Skip to content

feat(cli): report the installed binary, target versions and platform in usage metrics - #412

Open
so0k wants to merge 3 commits into
tele/s3-sentry-usage-metricsfrom
tele/s4-binary-target-attrs
Open

feat(cli): report the installed binary, target versions and platform in usage metrics#412
so0k wants to merge 3 commits into
tele/s3-sentry-usage-metricsfrom
tele/s4-binary-target-attrs

Conversation

@so0k

@so0k so0k commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Part 4 of 6 in a stack; review order S1 → S6; base is the previous slice.

  1. chore(deps): upgrade @sentry/node to 10.x with unchanged reporting behaviour
  2. fix(cli): stop the top-level error handler racing yargs
  3. feat(cli): replace HashiCorp checkpoint telemetry with Sentry usage metrics and consent
  4. feat(cli): report the installed binary, target versions and platform in usage metrics (this PR)
  5. feat(cli): report per-stack, per-provider and per-command usage metrics
  6. chore(gha): run the telemetry delivery e2e on every build

Related issue

Part of #48

Description

S3 emits usage metrics carrying only command, ci and language, 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 to unknown, plus a seam so tests can drive it without spawning anything. The probe result is shared across copies of commons in the bundle via globalThis. packages/@cdktn/commons/src/debug.ts moves onto the same probe.
  • packages/@cdktn/commons/src/telemetry.ts: getBinaryAttributes (bounded, release-only version, and a version only for a recognised product), getProjectTargetAttributes and setProjectTargetAttributes, os and arch, and semverRangeOrInvalid, which canonicalizes a declared range or reduces it to the literal invalid. The base attribute set grows accordingly.
  • packages/@cdktn/cli-core/src/lib/error-reporting.ts: one setProjectTargetAttributes(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.PATCH and dropped entirely for an unrecognised product, ranges are canonical or invalid, and nothing carries a path, a binary location or free text.

Start with getBinaryAttributes and getProjectTargetAttributes in packages/@cdktn/commons/src/telemetry.ts, then the probe in packages/@cdktn/commons/src/terraform.ts and its timeout handling. The exact-attribute-set test in telemetry.test.ts is the thing to check the result against: it pins the sorted key set of cli.command.invoked, so an accidental extra attribute fails.

Reading order for this slice:

  1. packages/@cdktn/commons/src/terraform.ts (the probe and its seam)
  2. packages/@cdktn/commons/src/telemetry.ts (the new attribute builders and the validators)
  3. packages/@cdktn/cli-core/src/lib/error-reporting.ts (the single capture site)
  4. packages/@cdktn/commons/src/terraform.test.ts, debug.test.ts and the exact-attribute-set and stamping cases in telemetry.test.ts

What is collected

Added to the base attribute set of every metric (the S3 set of command, ci and language is unchanged and still applies). Still gated by isUsageTelemetryEnabled() and a Sentry DSN in the build; the never-sent list from S3 is unchanged by this slice.

Attribute Value Source
os, arch process.platform / process.arch Node
binary terraform, opentofu, unknown, missing one lazy process-global probe, 1500 ms ceiling, timeout maps to unknown
binary_version MAJOR.MINOR.PATCH, only when binary is terraform or opentofu the probe; prerelease and build text dropped; an unrecognised product sends no version
target_terraform, target_opentofu canonical semver range (for example >=1.9.0), else invalid targetVersions in cdktf.json, else the CLI's DEFAULT_TARGET_VERSIONS; prerelease or build identifiers make it invalid
targets_declared boolean whether cdktf.json has a targetVersions block
validate_installed_binary boolean validateInstalledBinary === true in cdktf.json

No new metrics are added in this slice.

Test plan

  • Unit tests green across @cdktn/commons, @cdktn/cli-core and cdktn-cli on this slice against S3
  • Exact-attribute-set test: the sorted attribute key set of cli.command.invoked matches the documented set, so an extra attribute fails the build
  • Probe: runs at most once per process, honours the 1500 ms ceiling, a timeout yields binary: unknown, a missing binary yields missing, an unrecognised product yields no binary_version
  • Version and range validation: prerelease and build text dropped from versions; a non-canonical or prerelease range becomes invalid
  • Stamping test: the target attributes captured at init appear on metrics emitted later in the run
  • targets_declared and validate_installed_binary read from cdktf.json through the same forgiving reader as the consent flag
  • Lint and pnpm prettier --check . clean

Review threads from #62 answered here

  • "dropped stack payload loses useful information", the target-versions half of the thread (so0k's follow-up): the declared Terraform and OpenTofu ranges are on every command metric now, alongside the installed binary and its version. The per-stack half of that thread is answered in S5.

Checklist

  • I have updated the PR title to match CDKTN's style guide
  • I have run the linter on my code locally
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if applicable (follow-up in cdk-terrain-docs)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works if applicable
  • New and existing unit tests pass locally with my changes

@so0k
so0k added this pull request to stack #415 September 9, 2026 18:44
@so0k so0k changed the title tele/s4 binary target attrs feat(cli): report the installed binary, target versions and platform in usage metrics Sep 9, 2026
@so0k
so0k marked this pull request as ready for review September 9, 2026 18:45
@so0k
so0k requested a review from a team as a code owner September 9, 2026 18:45

@jsteinich jsteinich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed as part of the full tele/s1tele/s6 stack. Breaking the errors.tsterraform.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.)

@so0k
so0k force-pushed the tele/s4-binary-target-attrs branch from 86cb1d4 to 7adca39 Compare September 10, 2026 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants