chore(gha): run the telemetry delivery e2e on every build - #414
Conversation
jsteinich
left a comment
There was a problem hiding this comment.
Reviewed as part of the full tele/s1–tele/s6 stack. The e2e itself is the best part of this whole stack — asserting on a real envelope reaching a real sink from the real esbuild bundle is a much stronger oracle than the mocked unit tests, and the leak assertions are exactly the right set: stack names, resource ids, private-registry hosts, local provider paths, and the SENTRY_ENVIRONMENT / SENTRY_TRACE / SENTRY_BAGGAGE values a user or CI might export. Matching "sentry.environment":{"value":"production" as a key/value adjacency rather than a bare production is a nice touch.
Three things.
1. The title undersells a production code change — please rename
The PR is titled chore(gha): run the telemetry delivery e2e on every build, but it also changes how the shipped CLI resolves its handlers, in packages/cdktn-cli/src/bin/cmds/helper/utilities.ts:
- const filePath = path.join(__dirname, "..", "handlers.js");
+ const filePath = path.join(__dirname, "cmds", "handlers.js");In the bundle, utilities.ts is inlined into bin/cdktn.js, so __dirname is bundle/bin — meaning the old first branch resolved to bundle/handlers.js, never matched, and every invocation fell through to the projectRootPath() lookup. The new path does match. That's a real behaviour change on the hot path of every command, and it's the right change, but it isn't a chore(gha).
The commit itself is labelled honestly (feat(cli): resolve bundled handlers next to the entrypoint and honour CDKTN_BUNDLE_OUTDIR) — the problem is only the PR title, and this repo allows squash-merge with PR_TITLE as the commit subject. So as it stands this lands as a chore and never appears in the changelog.
Suggest just renaming the PR to cover both, e.g. feat(cli): resolve bundled handlers next to the entrypoint and run the telemetry e2e in CI.
2. Run the e2e as its own job rather than appending to build-and-package
build-and-package is the critical-path job — every other workflow gates on it. This step adds a second full esbuild, four CLI invocations (one of which is cdktn convert, which shells out to init), and a poll loop that waits up to 10 s on a local HTTP sink. A flake or a timeout in any of that now fails the build for everyone, including PRs that touch nothing near telemetry.
A separate job depending on build-and-package would isolate the blast radius and make the failure legible when it does fire. It also fits the existing ci/skip-* label convention, which this step currently sits outside of.
3. The script has an undeclared dependency on pnpm package
cdktn convert resolves dist via pkg.version === "0.0.0" ? dist : undefined, and in-repo the version is 0.0.0 — so the convert trigger needs dist/ to exist. That holds in CI only because the step is appended after pnpm run package in the same job. Run tools/validate-sentry-e2e.sh on a clean checkout locally and it fails somewhere deep inside convert instead of saying what's missing.
Worth an explicit guard near the top:
[ -d "$ROOT/dist" ] || { echo "FAIL: run 'pnpm package' first" >&2; exit 1; }This becomes more important, not less, if #2 moves it to a separate job where the ordering is no longer implicit.
b67e335 to
ee69f72
Compare
ee69f72 to
e76083f
Compare
Part 6 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 metricschore(gha): run the telemetry delivery e2e on every build(this PR)Related issue
Part of #48
Description
Unit tests can prove what the CLI intends to emit. Only a real bundle, run as a real process against a real endpoint, proves that anything is actually delivered before the process exits, and that nothing unintended rides along. This slice makes that check a step in the build workflow, running on every build.
What changes:
tools/sentry-sink.mjs: a local envelope sink that records what arrives.tools/validate-sentry-e2e.sh: builds a throwaway bundle with the sink's DSN baked in, picks a free port, runs the trigger commands, and polls the sink for the expected items instead of sleeping. It asserts the success-path flush, the entrypoint failure metric and the per-stack metrics arrive; it greps the real bundle artifact forcheckpoint-api.hashicorp.com; and it asserts the deliberateLEAK-*markers (hostname, username, cwd, temp project directory,SENTRY_ENVIRONMENT/SENTRY_TRACE/SENTRY_BAGGAGE) are absent from everything the sink received. It fails on a missing metric rather than on a timeout, and needs noSENTRY_DSNsecret..github/workflows/build.yml: one step running that script. The script unsets the job-levelCHECKPOINT_DISABLE=1for its own child processes so the metrics flow. Nothing leaves the runner, and the packaged bundle is untouched.packages/cdktn-cli/build.tsandpackages/cdktn-cli/.gitignore: an outdir override (CDKTN_BUNDLE_OUTDIR, default./bundle) so the E2E can build a throwaway bundle next to the shipped one instead of rewriting and restoring it, with the scratch directory ignored.One production behaviour change, deliberately in this slice
requireHandlersinpackages/cdktn-cli/src/bin/cmds/helper/utilities.tsprobed__dirname/../handlers.js, a path no bundle layout actually has, so every run fell through to the shipped bundle at the project root. It now probes__dirname/cmds/handlers.jsfirst, which is where the bundle actually lays the file down, and keeps the project-root fallback.This is a production change, not test-only. It is what lets a bundle built into any other directory load its own handlers, which is exactly what the E2E needs, and it means a bundle run from a non-default location now uses its own handlers rather than silently borrowing the installed one's. It is in this slice because the E2E is the thing that exposed it and the thing that covers it.
Start with that
requireHandlerschange, since it is the only production line in the slice. Then readtools/validate-sentry-e2e.shtop to bottom; it is the whole of the rest.Reading order for this slice:
packages/cdktn-cli/src/bin/cmds/helper/utilities.ts(the resolution change)tools/validate-sentry-e2e.sh(the scratch bundle, the free port, the triggers, the assertions)tools/sentry-sink.mjspackages/cdktn-cli/build.ts,packages/cdktn-cli/.gitignoreand the.github/workflows/build.ymlstepTest plan
@cdktn/commons,@cdktn/cli-coreandcdktn-clion this slice against S5tools/validate-sentry-e2e.shgreen locally: scratch bundle, free port, polling, all expected items observedERR_UNHANDLED_REJECTIONand noPromiseRejectionHandledWarningprintedLEAK-*markers for hostname, username, cwd, temp project directory andSENTRY_*values asserted absentcheckpoint-api.hashicorp.comreferences in the real artifactpnpm prettier --check .cleanReview threads from #62 answered here
tools/validate-sentry-e2e.shruns as a build-workflow step instead, on every build. What makes that safe to run unconditionally is the throwaway bundle with the sink DSN baked in, the free port, the polling instead of sleeping, and needing noSENTRY_DSNsecret; and it fails where the signal is actually missing rather than on a timeout.Follow-ups (documented, not in this PR)
parseMetricItemsinpackages/@cdktn/commons/src/telemetry.test.ts,recordEnvelopeintools/sentry-sink.mjs, and the local parser inpackages/cdktn-cli/src/bin/__tests__/error-handling.integration.test.ts. An envelope-format change has to be fixed in three places; extract one shared test-support helper. Commons cannot depend on CLI test code, so it likely belongs in a small test-support module.Checklist