feat(cli): report the installed binary, target versions and platform in usage metrics - #412
feat(cli): report the installed binary, target versions and platform in usage metrics#412so0k 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. Breaking the errors.ts → terraform.ts cycle so telemetry.ts can be imported from errors.ts is a nice bit of untangling, and the process-global Symbol.for probe key is the right answer to the "bundle carries several copies of this module" problem.
One real issue and one smaller one.
The 1500 ms bound doesn't bound what the docstring says it does
telemetry.ts:149:
* Binary attributes from the version probe, bounded so a hung binary never
* delays the command; a timed-out probe reports `binary: "unknown"`.The Promise.race bounds the attribute, not the process. exec in commons/src/util.ts uses spawn with no unref() and no kill-on-timeout, so a hung terraform version leaves a live child handle on the event loop: the race resolves at 1500 ms, the metric gets binary: "unknown", and then the CLI sits there until the child exits. beforeExit won't fire while that handle is active, so createBeforeExitFlush doesn't run either — the command hangs at exit rather than degrading gracefully.
Either kill the child when the timer fires, or unref() the probe's child so it can't hold the loop open. Worth fixing because "the version probe hangs" is exactly the scenario the timeout was written for.
The probe now runs on commands that never needed the binary
Related but separate: getBinaryAttributes() is awaited unconditionally from sendTelemetry (telemetry.ts:229), so init, get, convert and synth now spawn terraform version purely to populate a metric attribute. It's memoised per process via the Symbol.for key so it's one spawn, not many, and it's correctly behind the isUsageTelemetryEnabled() early-return — but it's still a new subprocess on commands that previously touched no binary at all, and on the failure path in reportFailure it can add up to 1500 ms before the process exits.
Not necessarily wrong, but worth a deliberate decision: is binary / binary_version valuable enough on an init or convert metric to justify spawning for it? Restricting the probe to the commands that already shell out would remove both this and most of the exposure above.
Minor: fabricated error prose as a success value
terraform.ts:71-77:
.catch(
(err) =>
`Error: Usage Error: Unknown: Error loading terraform version ${err}`,
);Resolving to a string that impersonates the old Errors.Usage output is a smell — the "Error: Usage Error: " prefix now exists only to make cdktn debug print something that looks like it used to. Since the only consumer is debug.ts:407 (which already does ?? null), returning undefined and letting debug.ts render the failure message would be clearer and wouldn't leave a fake error string floating around for someone to pattern-match on later.
(The Promise → function change itself is fine — all in-repo consumers are updated.)
fe64262 to
86cb1d4
Compare
86cb1d4 to
7adca39
Compare
Part 4 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 metrics(this PR)feat(cli): report per-stack, per-provider and per-command usage metricschore(gha): run the telemetry delivery e2e on every buildRelated issue
Part of #48
Description
S3 emits usage metrics carrying only
command,ciandlanguage, which does not answer the questions the metrics exist for: which Terraform or OpenTofu binary people actually run, which versions their projects declare they target, and on what platform. This slice adds that environment context to the base attribute set, so every metric from here on carries it.What changes:
packages/@cdktn/commons/src/terraform.ts: a lazy binary probe that runs at most once per process, with a 1500 ms ceiling and a timeout mapping tounknown, plus a seam so tests can drive it without spawning anything. The probe result is shared across copies of commons in the bundle viaglobalThis.packages/@cdktn/commons/src/debug.tsmoves onto the same probe.packages/@cdktn/commons/src/telemetry.ts:getBinaryAttributes(bounded, release-only version, and a version only for a recognised product),getProjectTargetAttributesandsetProjectTargetAttributes,osandarch, andsemverRangeOrInvalid, which canonicalizes a declared range or reduces it to the literalinvalid. The base attribute set grows accordingly.packages/@cdktn/cli-core/src/lib/error-reporting.ts: onesetProjectTargetAttributes(getProjectTargetAttributes(projectPath))capture at init, so the target attributes are read once from the project that is actually running.Every value here is validated before it becomes an attribute rather than forwarded: versions are reduced to
MAJOR.MINOR.PATCHand dropped entirely for an unrecognised product, ranges are canonical orinvalid, and nothing carries a path, a binary location or free text.Start with
getBinaryAttributesandgetProjectTargetAttributesinpackages/@cdktn/commons/src/telemetry.ts, then the probe inpackages/@cdktn/commons/src/terraform.tsand its timeout handling. The exact-attribute-set test intelemetry.test.tsis the thing to check the result against: it pins the sorted key set ofcli.command.invoked, so an accidental extra attribute fails.Reading order for this slice:
packages/@cdktn/commons/src/terraform.ts(the probe and its seam)packages/@cdktn/commons/src/telemetry.ts(the new attribute builders and the validators)packages/@cdktn/cli-core/src/lib/error-reporting.ts(the single capture site)packages/@cdktn/commons/src/terraform.test.ts,debug.test.tsand the exact-attribute-set and stamping cases intelemetry.test.tsWhat is collected
Added to the base attribute set of every metric (the S3 set of
command,ciandlanguageis unchanged and still applies). Still gated byisUsageTelemetryEnabled()and a Sentry DSN in the build; the never-sent list from S3 is unchanged by this slice.os,archprocess.platform/process.archbinaryterraform,opentofu,unknown,missingunknownbinary_versionMAJOR.MINOR.PATCH, only whenbinaryisterraformoropentofutarget_terraform,target_opentofu>=1.9.0), elseinvalidtargetVersionsincdktf.json, else the CLI'sDEFAULT_TARGET_VERSIONS; prerelease or build identifiers make itinvalidtargets_declaredcdktf.jsonhas atargetVersionsblockvalidate_installed_binaryvalidateInstalledBinary === trueincdktf.jsonNo new metrics are added in this slice.
Test plan
@cdktn/commons,@cdktn/cli-coreandcdktn-clion this slice against S3cli.command.invokedmatches the documented set, so an extra attribute fails the buildbinary: unknown, a missing binary yieldsmissing, an unrecognised product yields nobinary_versioninvalidtargets_declaredandvalidate_installed_binaryread fromcdktf.jsonthrough the same forgiving reader as the consent flagpnpm prettier --check .cleanReview threads from #62 answered here
Checklist