feat(cli): report per-stack, per-provider and per-command usage metrics - #413
feat(cli): report per-stack, per-provider and per-command usage metrics#413so0k wants to merge 3 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 privacy normalisers are the strongest part of this PR — normalizeProviderSource reducing non-public hosts to private-registry, classifyModuleSource collapsing anything path- or host-shaped, and groupSizes sending counts where the raw structure would carry resource ids. Collapsing four copies of the JSON.parse(stack.content)["//"].metadata incantation into SynthStack.telemetryPayload (with a try/catch, which the originals lacked) is a good catch too.
Four things.
1. cli.command.invoked means two different things depending on the command
finishStackRun (cdktf-project.ts:439) sends telemetry before throwing, so a failed deploy / diff / destroy emits both cli.command.invoked and cli.command.error. A failed synth exits inside synth-stack.ts (:181, :203, introduced in #411) and emits only cli.command.error, never invoked.
So invoked counts attempts for the stack commands and successes for synth. Any success-rate or funnel query across commands silently returns the wrong number, and it's the kind of thing that's very hard to notice once the data is in Sentry.
Worth picking one semantic and stating it in a comment next to the metric. Emitting invoked unconditionally at command start — and letting cli.command.error be the thing that varies — is the version I'd argue for: it makes error / invoked a real success rate, and it stops the meaning of invoked depending on which exit path a command happens to take.
2. Per-run metric volume is unbounded
sendStackTelemetry emits, per stack, one cli.stack + one cli.stack.override per resource type + one cli.stack.provider per provider; sendGetTelemetry emits one per generated binding. A 50-stack project with 10 providers each produces on the order of 550 metric events for a single cdktn deploy, each carrying ~12 attributes after #412's os / arch / binary / target_* additions. There's no cap anywhere.
Every individual attribute is bounded (that's the good part), but the number of emissions scales with project size, and Sentry bills per metric. Worth a sanity check against the plan's quota with a realistically large project before this ships — a cap, or aggregating the per-provider counts into the cli.stack metric rather than emitting one event each, would both bound it.
3. SYNTH_ORIGINS duplicates a type that could just move to commons
telemetry.ts:379:
// The synth origins the CLI passes through; see SynthOrigin in cli-core.
const SYNTH_ORIGINS = ["watch"];The comment acknowledges the drift risk, on the premise that commons can't import cli-core. But the dependency only needs to go the other way: @cdktn/cli-core already depends on @cdktn/commons, commons doesn't depend on cli-core, and SynthOrigin is not exported from cli-core's public lib/index.ts (only SynthesizedStack is). So it's internal to lib/ and can move for free.
Make commons the source of truth and derive the type from the runtime array, so the allow-list and the type physically cannot drift:
// packages/@cdktn/commons/src/telemetry.ts
export const SYNTH_ORIGINS = ["watch"] as const;
export type SynthOrigin = (typeof SYNTH_ORIGINS)[number];// packages/@cdktn/cli-core/src/lib/synth-stack.ts
export type { SynthOrigin } from "@cdktn/commons";That leaves the four existing SynthOrigin references in synth-stack.ts and cdktf-project.ts working unchanged. One adjustment needed: ScalarAttribute.values is string[] at telemetry.ts:387 and won't accept a readonly tuple — widen it to readonly string[].
WATCH_EVENTS is a different case and I'd leave it: watch.ts:184 passes a bare { event: "start" } literal with no named type, so there's nothing being duplicated. If you want the same guarantee there it means introducing a WatchEvent type in commons and typing the call site against it, which is new work rather than de-duplication.
4. getGeneratedProviderSources() defaults to process.cwd()
telemetry.ts:470, inside sendStackTelemetry. Everything else that reads project state is deliberately captured at command start precisely because convert chdirs — usageTelemetryEnabledState and projectTargetAttributes both go through setX/getX pairs for exactly this reason. This one reads the cwd at emission time.
Harmless today (the stack commands don't chdir), so this is a consistency point rather than a bug — but it's the one reader that would silently do the wrong thing if a future command adopts the convert pattern, and the surrounding code has already established the safer idiom.
995d31e to
f04c44c
Compare
f04c44c to
60fc03a
Compare
|
Item 1 is resolved in #411 rather than here, since that is where the divergence starts: |
Part 5 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 consentfeat(cli): report the installed binary, target versions and platform in usage metricsfeat(cli): report per-stack, per-provider and per-command usage metrics(this PR)chore(gha): run the telemetry delivery e2e on every buildRelated issue
Part of #48
Description
The old checkpoint transport posted a per-stack payload (backends, required providers, overrides) to HashiCorp. S3 removed the transport and, with it, that signal:
SynthStackwas handing a payload tosendTelemetrythat was then thrown away. This slice brings the signal back, reduced at emission into counted metrics with enumerated and validated values, so nothing carries a name, an address or free text.What changes:
packages/@cdktn/commons/src/telemetry.ts:sendStackTelemetryemittingcli.stack,cli.stack.override,cli.stack.providerandcli.stack.failed;sendGetTelemetryandsendInitTelemetry; a typedSCALAR_ATTRIBUTESallow-list per command, so nothing outside it is ever forwarded; and the validators the reduction runs on:normalizeProviderSource,classifyModuleSource,classifyProviderBinding,normalizeBackendKind,normalizeProviderConstraint, the bounded resource-type grammar and the identity caps.packages/@cdktn/cli-core/src/lib/synth-stack.ts(the telemetry payload and the stack reduction),packages/@cdktn/cli-core/src/lib/cdktf-project.ts(finishStackRun),packages/@cdktn/commons/src/construct-maker-target.ts,packages/cdktn-cli/src/bin/cmds/helper/init.ts(templateTelemetryNameand thetemplate/is_remote/addedProvidersmapping),packages/cdktn-cli/src/bin/cmds/ui/get.ts(target collection) andpackages/cdktn-cli/src/bin/cmds/handlers.ts(the convert stats mapping).Where the command metrics land
The emission rule is fixed in S3 and this slice follows it:
cli.command.invokedonce per run at command start,cli.command.completedonce at the end of a successful run,cli.command.erroronce per failed run, soerror / invokedis a true failure rate. Two consequences show up in this slice:finishStackRunon a failed run. The per-stack metrics still go out before the throw, so a deploy that fails partway still reports the stacks it did reach and thecli.stack.failedcount. What no longer goes out there is the command metric: the run was already counted as invoked at start, and the failure is counted once by the entrypoint ascli.command.error.cli.command.completed, not oninvoked, because they are only known once the command has finished.Start with
sendStackTelemetryand the normalizers directly above it inpackages/@cdktn/commons/src/telemetry.ts, because that is where every string is either reduced to an enumerated value or dropped. Then read the reduction inpackages/@cdktn/cli-core/src/lib/synth-stack.tsto see what the payload looks like before it gets there.The whole point of the design is that the reduction happens at emission, not at collection. A malformed
required_providersentry, a non-object stack, an over-long hand-written source: each is tolerated and reduced tootherrather than passed through or thrown on. Four privacy leaks found by deliberately trying to make the CLI leak were fixed this way, and the tests that found them ship with this slice asLEAK-*marker assertions on the raw envelope bytes.Reading order for this slice:
packages/@cdktn/commons/src/telemetry.ts: the normalizers and validators, thensendStackTelemetry, thenSCALAR_ATTRIBUTESpackages/@cdktn/cli-core/src/lib/synth-stack.tsandcdktf-project.tspackages/cdktn-cli/src/bin/cmds/helper/init.ts,ui/get.ts,handlers.tspackages/@cdktn/commons/src/telemetry.test.ts(the privacy invariant and the normalization tables), thencdktf-project-telemetry.test.ts,synth-stack.test.ts,construct-maker-target.test.tsKnown gap
HCL-output projects yield no
cli.stack.provider. For those the CLI reads only the separate metadata file, sorequired_providersis never in hand, and no provider metric can be emitted. The stack and override metrics are unaffected. Recorded as a follow-up rather than worked around, because the fix belongs in the synth output path, not in telemetry.What is collected
Added on top of the S3 and S4 sets. Everything still carries the base attributes (
command,ci,language,os,arch,binary,binary_version,target_terraform,target_opentofu,targets_declared,validate_installed_binary) and is still gated byisUsageTelemetryEnabled()and a Sentry DSN in the build.cli.stackbackend(an enumerated Terraform backend kind, elseother,unknownwhen absent),cloud(boolean),library_version(release only),override_count,import_count,moved_countcli.stack.overrideresource_type(Terraform type grammar, the stack-level keys, ormodule.<kind>; elseother; 64-char cap),override_countcli.stack.providerprovider(public-registrynamespace/typeonly; other hostsprivate-registry; malformedother),version_constraint(canonical, elseinvalid),binding(generatedorprebuilt)cli.stack.failedcli.get.provider/cli.get.moduleprovider/moduleidentity as abovecli.init.providerproviderPer-command scalars added to
cli.command.completed, allow-listed by command and nothing else forwarded:synth_originfor synth;template(a built-in template name, anything else the literalremote),is_remoteandprovider_countfor init;module_count,provider_countandconverted_linesfor convert;provider_countandmodule_countfor get. Theeventscalar thewatchstart event used to carry is dropped:cli.command.invokedis emitted at command start and already records exactly that.A
required_providersentry with no stringsource({ aws: { version: "~> 5.0" } }) is reported under its local name expanded to the default namespace,hashicorp/<local name>, the same value Terraform itself resolves it to. Public-registry identities are additionally bounded (each segment at most 64 characters, at most 128 in total), so an over-long hand-written source becomesotherinstead of carrying free text.What is never sent
Extending the S3 list. Stack names; resource ids and addresses (imports and moves are counts only); file paths and the working directory; the machine hostname or username; private-registry hosts and organizations; module URLs and paths (reduced to
local,git,private-registryorother); remote template URLs (reduced toremote); error messages and error context; any user code; the values ofSENTRY_*environment variables. Unit tests assert these against the raw envelope bytes using deliberateLEAK-*markers.Test plan
@cdktn/commons,@cdktn/cli-coreandcdktn-clion this slice against S4SENTRY_ENVIRONMENT/SENTRY_TRACE/SENTRY_BAGGAGEexported asLEAK-*markers, asserted absent from the raw envelope bytesrequired_providersentry with no stringsource, a non-object stack skipped rather than thrown oncli.command.completedcli.stack.failedbefore the throw, then exactly onecli.command.errorand nocli.command.completed, against the onecli.command.invokedemitted at startcli.stack.failedcounts stacks that did not complete, with no names and no messagespnpm prettier --check .cleanReview threads from #62 answered here
cli.stackper stack with the backend kind, the cloud flag, the library version and the override/import/moved counts; onecli.stack.overrideper overridden resource type; onecli.stack.providerper required provider with its constraint and whether the binding is generated or prebuilt; pluscli.stack.failed. Provider identities are reduced to public-registrynamespace/type, so private hosts and organizations becomeprivate-registryand nothing carries a name or an address. The target-versions half of the thread is answered in S4.Follow-ups (documented, not in this PR)
cli.stack.resource, an allow-listed per-resource-type count, is the one metric that would help the asset-pipeline decision (Proposal: scope and shape of the asset pipeline #380). Deferred deliberately.cli.stack.provider, as described above. Known gap.Checklist