feat(cli): replace HashiCorp checkpoint telemetry with Sentry usage metrics and consent - #411
feat(cli): replace HashiCorp checkpoint telemetry with Sentry usage metrics and consent#411so0k wants to merge 11 commits into
Conversation
jsteinich
left a comment
There was a problem hiding this comment.
Reviewed as part of the full tele/s1–tele/s6 stack. The consent model here is the part I scrutinised hardest and I think it's right:
initializErrorReportinginitialises Sentry when either flag is on, andbeforeSendreturnsnullwhen only usage telemetry is enabled — so metrics flow without a single error event leaking to a user who never opted into crash reporting. That's the correct decomposition and it's easy to get wrong.- The
hasCapturedUsageTelemetryDecision()guard genuinely works:convertcallsinitializErrorReporting()athandlers.ts:96before theprocess.chdirinto the throwaway project, so the decision is captured against the user's real cwd and the nestedinitcorrectly declines to overwrite it. I traced both the TypeScript (needsProject === false) and non-TypeScript paths. - Fixing
shouldReportCrashto returnundefinedfor an absent key — rather than silentlyfalse— is the right call even though it means existing users see a new prompt on upgrade. Good that it's called out in the description.
Two things.
sendUsageTelemetry is typed boolean but rendered as a string
The templates emit "sendUsageTelemetry": "{{sendUsageTelemetry}}", so a freshly inited project has the string "true" / "false" on disk, while ConfigBase declares:
readonly sendUsageTelemetry?: boolean;getUsageTelemetryConsent handles the string explicitly (and the comment explains why), so nothing is broken today. But the declared type is now a lie about what's actually in the file, and the trap is asymmetric: any future if (config.sendUsageTelemetry) through the typed path reads the string "false" as true — i.e. it fails open, silently enabling telemetry for a user who opted out.
This mirrors the pre-existing sendCrashReports trap rather than introducing a new one, so I don't think it blocks. But since this PR is the one adding both fields to ConfigBase, it's the natural place to normalise them in parseConfig and let the rest of the codebase trust the type.
Heads-up for #413: this sets up an inconsistency in cli.command.invoked
Not actionable in this PR, recording it here because the divergence starts with the paths introduced here. synth-stack.ts:181 and :203 emit cli.command.error and then hard-exit, so a failed synth never emits cli.command.invoked. In #413, finishStackRun sends telemetry before throwing, so a failed deploy emits both. Details and a suggested fix are on #413.
897fb06 to
e3f9914
Compare
Review: usage-consent and failed-command telemetry gapsReviewed at head 1.
|
… and terraform checks
…e across the bundle's module copies
…ry store correctly
|
@sakul-learning both gaps are fixed on the branch, one commit each.
@jsteinich on the string-typed flags: On the |
Re-review at
|
Part 3 of 6 in a stack; review order S1 → S6; base is the previous slice.
chore(deps): upgrade @sentry/node to 10.x with unchanged reporting behaviourfix(cli): stop the top-level error handler racing yargsfeat(cli): replace HashiCorp checkpoint telemetry with Sentry usage metrics and consent(this PR)feat(cli): report the installed binary, target versions and platform in usage metricsfeat(cli): report per-stack, per-provider and per-command usage metricschore(gha): run the telemetry delivery e2e on every buildRelated issue
Resolves #48
Description
This is the slice that answers the issue: the CLI stops sending anything to
checkpoint-api.hashicorp.com, and usage analytics go to the project's own Sentry instead, behind their own consent flag. S4 and S5 add attributes and per-item metrics on top of the contract established here; nothing in this slice depends on them.What changes:
packages/@cdktn/commons/src/checkpoint.tsandpackages/@cdktn/cli-core/src/test/checkpoint.test.tsare removed, andnockleavespackages/@cdktn/cli-core/package.json(that test was its only user), so the lockfile moves again. The preservedgetUserId/getProjectIdrelocate topackages/@cdktn/commons/src/identity.ts.packages/@cdktn/commons/src/telemetry.ts: the consent reader, the decision captured at command start plushasCapturedUsageTelemetryDecision,isUsageTelemetryEnabled, the single boundedflushTelemetry,sendErrorTelemetry,startCommandTelemetryemittingcli.command.invokedat the start of a run, andsendTelemetryemittingcli.command.completed,cli.command.errorandcli.synth.duration, all with attributes{command, ci, language}. The rule those three command metrics follow is written out under One emission rule for the whole stack below.packages/@cdktn/cli-core/src/lib/error-reporting.ts: a per-flag interactive prompt (TTY, not CI), persistence back tocdktf.json, aprojectPathparameter so a project directory that does not exist yet is handled, and thesetUsageTelemetryEnabledcapture.packages/@cdktn/commons/src/config.tsgainssendUsageTelemetryalongsidesendCrashReportsonConfigBase, and the six templates'cdktf.jsoncarry the flag. Because the templates render the flags as"{{sendUsageTelemetry}}", an on-disk value may be the string"true"or"false";parseConfignow normalises both consent flags to booleans, and drops any other value as unset with a debug log, so the declaredbooleantype is true of everything the rest of the codebase reads.packages/@cdktn/commons/src/errors.ts(factories callsendErrorTelemetry(type, scope)at call time, plusErrors.getScope()),packages/@cdktn/cli-core/src/lib/synth-stack.ts(command metric and duration, and failure paths that emit the error metric, flush, then exit),packages/@cdktn/cli-core/src/lib/cdktf-project.ts,packages/cdktn-cli/src/bin/cmds/handlers.ts,packages/cdktn-cli/src/bin/cmds/ui/get.ts, and the init path (packages/@cdktn/cli-core/src/lib/init.ts,packages/cdktn-cli/src/bin/cmds/init.ts,packages/cdktn-cli/src/bin/cmds/helper/init.ts,packages/cdktn-cli/src/bin/cmds/helper/error-reporting.ts).convertcollects consent like every other interactive handler:convert()passesaskForCrashReportingConsentandaskForUsageTelemetryConsent, and does so before it builds its temporary project and before anyprocess.chdir, so the decision is captured against the user's real cwd andhasCapturedUsageTelemetryDecisionkeeps the nestedinitfrom overwriting it. A handler-level test covers the prompt, the persistence, the no-re-prompt case and the non-interactive path.--check-code-maker-outputguard, thewatch --auto-approveguard and the Terraform availability/version check inpackages/cdktn-cli/src/bin/cmds/helper/terraform-check.tsnowthrow Errors.Usage(...)rather than printing and callingprocess.exit(1), so each of them reaches the single failure path: one line of output, onecli.command.error{error_type: Usage}, one bounded flush, one exit. The direct exits that remain all run before reporting is initialised (an unknown command, and init's empty-directory, template-download and login checks), and the two self-exiting synth paths emit and flush themselves.@cdktn/commons, so module-level state is per bundle rather than per process. The command scope, the captured consent decision, the target attributes and the started-command record all live in a store keyed bySymbol.for, which makes them one value per run whichever copy reads them. This is what used to make an error raised inside@cdktn/hcl2cdkcountcli.errorwithcommand: "unknown"; the delivery e2e never caught it because it only asserted metric names, and it now asserts attribute values as well.packages/cdktn-cli/src/bin/error-handling.ts,exit-flush.tsandcdktn.tsdrop their directSentry.flush/Sentry.closecalls in favour offlushTelemetry(), andreportFailuregains asendCommandErrorTelemetrydep and thecommandErrorTypecomputation.Start with
packages/@cdktn/commons/src/telemetry.ts. It is the whole contract in one file: the consent gate, what is emitted, and the only flush. Then readpackages/@cdktn/cli-core/src/lib/error-reporting.tsfor how consent is asked and stored.Reading order for this slice:
packages/@cdktn/commons/src/telemetry.tspackages/@cdktn/cli-core/src/lib/error-reporting.tspackages/@cdktn/commons/src/errors.tsandidentity.tspackages/@cdktn/cli-core/src/lib/synth-stack.tsandcdktf-project.tspackages/cdktn-cli/src/bin/error-handling.ts,exit-flush.ts,cdktn.ts(the flush reroute and the failure metric)packages/@cdktn/commons/src/telemetry.test.tsandpackages/@cdktn/cli-core/src/test/error-reporting.test.tsThings worth calling out
CdktfConfig.sendUsageTelemetrygetter was removed rather than kept. It had zero production callers and parsed the flag differently from the reader that actually gates emission, throwing on a malformed value instead of treating it as unset. There is now exactly one, forgiving, reader, inpackages/@cdktn/commons/src/telemetry.ts.ConfigBasekeeps the field socdktf.json's schema is still documented.cli.command.erroris emitted at most once per failed run. Synth paths that exit inside cli-core count themselves and then exit; a synth error that is rethrown instead is counted by the entrypoint. A failed run is therefore never counted twice, and never counted zero times when it reaches the reporter. The tests pin both halves of that rule.One emission rule for the whole stack
This is the rule for S3 through S6, stated here because the metrics are defined here.
cli.command.invokedis emitted exactly once per run, at command start, insideinitializErrorReportingright after Sentry is initialised and under the command scope. It counts attempts, not successes.cli.command.completedis emitted once at the end of a successful run, and is the metric that carries the scalars only known at the end: the convert stats, the init template, the get counts, and in S5 the synth origin.cli.command.erroris emitted once per failed run.error / invokedis therefore a true failure rate for every command, andinvokedno longer means one thing for synth and another for deploy. Consequences worth knowing:login,debug,provider listandprovider upgrade.provider addinitialises reporting, so it counts as invoked without a matching completed.watchstart-event metric is gone.invokedat command start already records exactly that.cdktn deploycounts as onedeployinvoked and onedeployerror, because deploy calls synth non-gracefully.What is collected
Everything below is gated by
isUsageTelemetryEnabled()(CHECKPOINT_DISABLE> the decision captured at command start >sendUsageTelemetryincdktf.json> default on) and a Sentry DSN in the build. Crash reports are a separate consent (sendCrashReports) and are unchanged. S4 and S5 extend this set; this is what the slice emits on its own.Base attributes on every metric below:
command;ci(the CI provider's name, orfalse);language(one of the supported languages, else omitted).languageis read fromcdktf.jsonat command start forcli.command.invokedand from the run's own payload forcli.command.completedandcli.command.error, which is why a run outside a project carries nolanguageon any of them.cli.command.invokedcli.command.completedcli.command.errorerror_type:Usage,External,Internal,unexpectedcli.synth.durationcli.errorErrorsfactoriestype(Usage,External,Internal),command(theErrorsscope, read at call time)Stamped by the Sentry SDK itself: a random, persistent user id kept in
~/.cdktf/config.json(delete the file and a new one is generated); the releasecdktn-cli-<version>, which is how the CLI version is known; a fixed environmentproduction; the SDK name and version; the fixedserver.addresscdktn-cli; a random per-process trace id and the emission timestamp; and one session record per process carrying the Node major version. Crash reports additionally carry theprojectIdfromcdktf.json.What is never sent
Stack names; resource ids and addresses; file paths and the working directory; the machine hostname or username; error messages and error context; any user code; the values of
SENTRY_*environment variables. Unit tests assert these against the raw envelope bytes.Migration notes (user-facing)
sendUsageTelemetryflag incdktf.json, independent ofsendCrashReports. Everything it covers is listed above.cdktf.jsonlackssendUsageTelemetryis asked once and the answer persisted. You may also be asked about crash reporting ifsendCrashReportsis missing: that prompt existed before but never fired due to a bug (a missing key was silently treated as "no"), and it now works as originally intended.CHECKPOINT_DISABLEkeeps working. It still disables usage telemetry everywhere and still does not affect crash reporting. Telemetry is fully inert when the build has no Sentry DSN, for example in self-built binaries.sendUsageTelemetryunset, interactivesendUsageTelemetryunset, CI or non-interactiveCHECKPOINT_DISABLEsetsendCrashReportsmissing, interactiveserverName: cdktn-cli, landed in S1){type, message, context}posted to HashiCorpTest plan
@cdktn/commons,@cdktn/cli-coreandcdktn-clion this slice against S2cli.command.invokedcli.command.invokedper run at start, onecli.command.completedon success, onecli.command.erroron failure; a nested operation (a deploy's synth, init's get, convert's throwaway init) adds no second runconvertconsent at the handler level: a missingsendUsageTelemetryunder an interactive terminal invokes the usage-consent callback and persists the answer, a flag already set is not re-prompted, and the non-interactive path stays prompt-freewatch --auto-approveguard and the Terraform availability check each yield onecli.command.error{error_type: Usage}and one flush before the exitparseConfignormalisation rows: booleans pass through,"true"/"false"become booleans, any other value is dropped as unset with a debug log, for both consent flagsCHECKPOINT_DISABLE, no-project, CI, the tri-state reader, persistence, and the init-time capture of the decision for a project directory that does not exist yetcli.errorcases including the call-time scope read, so a latersetScopeis honoredcli.command.errorcounted exactly once per failed run: self-exiting synth paths and the rethrow-to-entrypoint path both coveredflushTelemetrybehaviourCHECKPOINT_DISABLEcases moved into their own describe, leaving the Sentry init hardening cases from S1 in theirspnpm prettier --check .cleanReview threads from #62 answered here
no-hashicorp-egress.test.ts: agreed, and this stack never introduces it. A source scan for a string that no longer exists in the tree asserts what a build failure already asserts. The grep over the real bundle artifact lands in S6, which is the only place that assertion can still be wrong.no-hashicorp-runtime-egress.test.ts: agreed, also never introduced here. The nock canary only re-proved that Sentry's own transport posts to the DSN host, which is not a fact about this change. That is whynockis dropped fromcli-corein this slice, where it was the only user.beforeExit, yargs failure and self-exiting synth paths all route throughflushTelemetry(). Nothing outsidepackages/@cdktn/commons/src/telemetry.tscallsSentry.flushorSentry.close, so the timeout lives in exactly one place.cli.errorwith the error type and the command scope, read at call time. The message and the context stay out, and it goes through the usage-telemetry gate like every other metric, so it is a counter rather than the old fire-and-forget POST of error text.Follow-ups (documented, not in this PR)
cdktn debugprints neither consent flag. BothsendUsageTelemetryandsendCrashReportsshould show up there so a user can see what a given project sends without readingcdktf.json.main.Checklist