feat(telemetry): implement CLI usage tracking#6691
Conversation
There was a problem hiding this comment.
4 issues found across 14 files
Confidence score: 3/5
- In
packages/cli/lib/utils.ts, telemetry headers are computed at import time, so--no-telemetrycannot reliably suppress the device ID for normal CLI runs; merging as-is risks violating explicit user opt-out expectations — move header/device-id resolution to execution time after CLI flags are parsed. - In
packages/cli/lib/index.ts,commandTelemetryEventsomitsdeployandpull, which means two key workflows won’t be tracked and telemetry will be systematically incomplete after release — add explicit mappings for both commands before merging. - In
packages/cli/lib/services/telemetry.service.ts, the catch path swallows delivery errors entirely, so dropped events become invisible and troubleshooting telemetry regressions is harder — add lightweight failure logging/metrics while keeping best-effort behavior. - In
packages/server/lib/controllers/sync/deploy/postDeploy.ts, attachingdevice-idto team-authenticatedtrack()events can create high-cardinality and account-linking side effects in PostHog; merging now may add noisy analytics dimensions and governance concerns — remove, hash, or gate this property before rollout.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Added telemetry support for CLI commands to track usage events. - Introduced device ID generation and storage for anonymous tracking. - Updated CLI options to enable/disable telemetry. - Created a new endpoint for receiving telemetry data on the server. - Enhanced headers in HTTP requests to include device ID for tracking. This change aims to improve insights into CLI usage patterns while respecting user privacy preferences.
6f20dcd to
e292e88
Compare
| 'User-Agent': getUserAgent() | ||
| }; | ||
| if (!isTelemetryDisabled()) { | ||
| headers['Nango-CLI-Device-Id'] = getDeviceId(); |
There was a problem hiding this comment.
header is going to be attached on all requests. not just the one for telemetry. Is that on purpose?
There was a problem hiding this comment.
This is intended. Idea being we can correlate the commands with an actual team/user once they do an auth call to the BE (deploy / pull)
| } | ||
|
|
||
| export function isTelemetryDisabled(): boolean { | ||
| return process.env['TELEMETRY'] === 'false' || process.env['NANGO_CLI_TELEMETRY'] === 'false'; |
There was a problem hiding this comment.
can we have a conflict with another tool using TELEMETRY env var and disable it by accident?
There was a problem hiding this comment.
Removed. Considering adding support for DO_NOT_TRACK https://donottrack.sh/ , although adoption isnt widespread
| let deviceId = state.get('deviceId') as string | undefined; | ||
| if (!deviceId) { | ||
| deviceId = randomUUID(); | ||
| state.set('deviceId', deviceId); |
| 'cli:migrate_to_zero_yaml', | ||
| 'cli:deploy', | ||
| 'cli:pull' | ||
| ]), |
There was a problem hiding this comment.
the enum should be shared with CliTelemetryEvent types
| 'cli:deploy', | ||
| 'cli:pull' | ||
| ]), | ||
| properties: z.record(z.string(), z.union([z.string(), z.number(), z.boolean()])).optional() |
There was a problem hiding this comment.
should properties be constrained?
There was a problem hiding this comment.
Removed them. Original plan was to add additional information to track, but not currently used, so lets add it if we need it later down the line
| // The `satisfies` above rejects entries that aren't valid `CliTelemetryEvent`s; | ||
| // the assertion below rejects any `CliTelemetryEvent` missing from this array. | ||
| // Together they keep the two lists in sync. | ||
| true satisfies [Exclude<CliTelemetryEvent, (typeof cliTelemetryEvents)[number]>] extends [never] ? true : never; |
There was a problem hiding this comment.
why is this file in utils package and not in server?
This change aims to improve insights into CLI usage patterns while respecting user privacy preferences.