[CFX-5205][CFX-5207][CFX-5208] first pass at wiring up Amplitude telemetry#403
Conversation
Add ldflags variables for telemetry configuration: - AmplitudeAPIKey: injected via GitHub Actions secret at build time - InstallMethod: set to 'release' for official builds, 'source' for dev Update release workflow to include AMPLITUDE_API_KEY secret.
Add GetUserID() function to fetch user ID from DataRobot API (GET /api/v2/userinfo/) for telemetry common properties.
- Add --disable-telemetry persistent flag bound to Viper - Initialize telemetry client in PersistentPreRunE - Flush telemetry events in PersistentPostRun with 3s timeout
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Update comments to reflect that UserID is now a placeholder, simplify GetUserID call in CollectCommonProperties, and remove unused imports. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
ajalon1
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Add missing 'encoding/json' and 'time' imports to internal/drapi/get.go - Fix undefined type error by changing 'types.Client' to 'amplitude.Client' in telemetry.go Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Amplitude SDK requires UserID or DeviceID as top-level fields on the event struct, not just in EventProperties. Without this, events are silently rejected by Amplitude's HTTP API with 'missing required field' errors. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
| MACOS_NOTARY_ISSUER_ID: ${{ secrets.MACOS_NOTARY_ISSUER_ID }} | ||
| MACOS_NOTARY_KEY_ID: ${{ secrets.MACOS_NOTARY_KEY_ID }} | ||
| MACOS_NOTARY_KEY: ${{ secrets.MACOS_NOTARY_KEY }} | ||
| AMPLITUDE_API_KEY: ${{ secrets.AMPLITUDE_API_KEY }} |
There was a problem hiding this comment.
There is currently no GH secret for this, so it will be blank
cmd/root.go
Outdated
| PersistentPostRun: func(cmd *cobra.Command, _ []string) { | ||
| // Flush telemetry events before exit | ||
| if client, ok := cmd.Context().Value(telemetryClientKey{}).(*telemetry.Client); ok { | ||
| client.Flush(3 * time.Second) | ||
| } |
There was a problem hiding this comment.
I actually need to check to see if this is executed on error, not just success.
There was a problem hiding this comment.
Seeing these generated for me, I wonder if it make sense to have a single event type (NewCLICommandEvent) and then a property for the actual event. That would, I think, decrease our Amplitude costs. Or to top-level command events like dr_plugin, dr_start, dr_dotenv ...
| Environment string // US, EU, JP, or custom — from endpoint URL | ||
| DataRobotInstance string // Base URL of configured DataRobot instance |
There was a problem hiding this comment.
I think Environment is extra information, and DataRobotInstance is sufficient.
| if c.amp == nil { | ||
| log.Debug("Telemetry event (dry-run)", "type", event.EventType, "properties", event.EventProperties) | ||
| return | ||
| } |
There was a problem hiding this comment.
This is how I can test without actually wiring up to our test Amplitude account.
|
I'm going to merge this into the feature branch now. |
| -X "github.com/datarobot/cli/internal/version.Version={{ .Tag }}" | ||
| -X "github.com/datarobot/cli/internal/version.GitCommit={{ .ShortCommit }}" | ||
| -X "github.com/datarobot/cli/internal/version.BuildDate={{ .CommitDate }}" | ||
| -X "github.com/datarobot/cli/internal/telemetry.AmplitudeAPIKey={{ .Env.AMPLITUDE_API_KEY }}" |
There was a problem hiding this comment.
GoReleaser build breaks without AMPLITUDE_API_KEY env var
Medium Severity
The ldflags template {{ .Env.AMPLITUDE_API_KEY }} requires the environment variable to exist at GoReleaser execution time. In CI, ${{ secrets.AMPLITUDE_API_KEY }} resolves to an empty string when the secret doesn't exist (and the reviewer confirmed the secret doesn't exist yet), so the env var is set but empty. However, any local goreleaser release invocation without explicitly exporting AMPLITUDE_API_KEY will fail with a template error because .Env requires the variable to be present. A default value via GoReleaser's env section or envOr template function would prevent this.
Additional Locations (1)
…metry (datarobot-oss#403) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
1 participant
RATIONALE
On discussion with @victorborshak I am working off of a feature branch for now. Soon enough I will be merging said feature branch into main.
CHANGES
add amplitude-go as a direct dependency
Create a new module
internal/telemetry, which contains:Wire up the
disable-telemetryopt-out flagIn the root command's PersistentPreRunE hook, collect common event properties and store them in telemetry context
In the root command's PersistentPostRun hook, flush telemetry events before exit
add the AMPLITUDE_API_KEY and InstallMethod as ldflags in goreleaser configuration; this will be used to enable Amplitude writes (duh) and to add context to Amplitude events about how the CLI was installed
add to the internal/http client the ability to get UserInfo, which will be added as UserID in event context
add AMPLITUDE_API_KEY as a GH Release secret, so that the API key will be baked into the release binary but NOT in the source code itself
PR Automation
Comment-Commands: Trigger CI by commenting on the PR:
/trigger-smoke-testor/trigger-test-smoke- Run smoke tests/trigger-install-testor/trigger-test-install- Run installation testsLabels: Apply labels to trigger workflows:
run-smoke-testsorgo- Run smoke tests on demand (only works for non-forked PRs)Important
For Forked PRs: If you're an external contributor, the
run-smoke-testslabel won't work. Only maintainers can trigger smoke tests on forked PRs by applying theapproved-for-smoke-testslabel after security review. Please comment requesting maintainer review if you need smoke tests to run.Note
Medium Risk
Introduces new telemetry collection code and release-time API-key injection, which affects CLI startup/exit hooks and adds a third-party analytics dependency; failures are intended to be no-ops but could impact runtime behavior if misconfigured.
Overview
Adds first-pass Amplitude telemetry plumbing to the CLI: a new
internal/telemetrypackage provides an Amplitude-backed client (with no-op/dry-run behavior when disabled), common property collection, and typed event constructors.Wires telemetry lifecycle into
cmd/root.goby initializing a client inPersistentPreRunE, exposing an opt-out--disable-telemetryflag (bound via Viper), and flushing events on exit inPersistentPostRunE.Updates release/build configuration to inject
AMPLITUDE_API_KEYandInstallMethodvia GoReleaserldflags, and passes the secret through the GitHub release workflow; adds the Amplitude Go SDK dependency and a placeholderdrapi.GetUserID()for future user identification.Written by Cursor Bugbot for commit e629e2b. This will update automatically on new commits. Configure here.