Skip to content

chore(deps): upgrade @sentry/node to 10.x with unchanged reporting behaviour - #409

Open
so0k wants to merge 5 commits into
mainfrom
tele/s1-sentry-sdk-10
Open

chore(deps): upgrade @sentry/node to 10.x with unchanged reporting behaviour#409
so0k wants to merge 5 commits into
mainfrom
tele/s1-sentry-sdk-10

Conversation

@so0k

@so0k so0k commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Part 1 of 6 in a stack; review order S1 → S6; base is the previous slice (this one is based on main).

  1. chore(deps): upgrade @sentry/node to 10.x with unchanged reporting behaviour (this PR)
  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
  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

Sentry sunset the 7.x custom-metrics product server side on 2024-10-07, so on @sentry/node 7.120.4 any metric emission is a silent no-op. The usage metrics that replace the HashiCorp checkpoint transport (S3 onwards) therefore need the v10 metrics API. This slice is that upgrade on its own, with error and crash reporting behaviour deliberately unchanged, so the SDK bump can be reviewed and bisected apart from the telemetry work.

What changes:

  • packages/@cdktn/commons/package.json, packages/@cdktn/cli-core/package.json, packages/cdktn-cli/package.json: @sentry/node moves to ^10.73.0 in all three at once, so the bundle keeps a single shared SDK instance. pnpm-lock.yaml is regenerated; only @sentry/* and its transitives move.
  • packages/@cdktn/cli-core/src/lib/error-reporting.ts: Sentry.configureScope becomes getCurrentScope(), and the Sentry.init option block is rebuilt for v10. Options removed by the SDK are dropped, tracesSampleRate: 0 is pinned (metrics deliver independently of trace sampling), environment is pinned to production so SENTRY_ENVIRONMENT is never read, a fresh propagation context is set so an inherited SENTRY_TRACE / SENTRY_BAGGAGE is discarded, and serverName is fixed to cdktn-cli.
  • packages/@cdktn/commons/src/errors.ts: the same scope-API change inside setScope. The factories themselves are untouched here.
  • packages/@cdktn/cli-core/src/test/error-reporting.test.ts: the init-options assertion (release, tracesSampleRate: 0, serverName, environment) plus the mock-factory shape it needs.

The one visible effect on today's behaviour is a privacy improvement that falls out of the fixed serverName: crash reports no longer carry the machine hostname, which the SDK attached by default. Nothing else about what is reported, or when, changes in this slice; there is still no usage telemetry of any kind here.

Start with the Sentry.init option block in packages/@cdktn/cli-core/src/lib/error-reporting.ts, then the matching assertion in packages/@cdktn/cli-core/src/test/error-reporting.test.ts, then the lockfile delta.

Reading order for this slice:

  1. packages/@cdktn/cli-core/src/lib/error-reporting.ts
  2. packages/@cdktn/commons/src/errors.ts
  3. packages/@cdktn/cli-core/src/test/error-reporting.test.ts
  4. the three package.json files and pnpm-lock.yaml

Test plan

  • Unit tests green across @cdktn/commons, @cdktn/cli-core and cdktn-cli on this slice against main
  • Init-options assertion covers release, tracesSampleRate: 0, serverName: "cdktn-cli" and environment: "production"
  • Crash-reporting behaviour unchanged: the existing shouldReportCrash and captureException tests pass untouched
  • pnpm install lockfile delta inspected: only @sentry/* and its transitive dependencies move
  • Lint and pnpm prettier --check . clean

Follow-ups (documented, not in this PR)

  • @sentry/node is a devDependency of cdktn-cli (it reaches users through the bundle and through @cdktn/commons). Worth confirming that is intended rather than incidental.

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/s1 sentry sdk 10 chore(deps): upgrade @sentry/node to 10.x with unchanged reporting behaviour 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. This one is a clean, self-contained upgrade and is mergeable independently. Two things before it lands.

Pin the version, and set enableMetrics explicitly

^10.73.0 breaks this repo's exact-pinning convention — every other dependency in these three package.json files is pinned exactly (ci-info: 4.4.0, chalk: 4.1.2, and the 7.120.4 this replaces).

The caret is riskier than usual here because metrics delivery in the later PRs depends on an undeclared default. In @sentry/core@10.73.0, metrics/internal.js:

const metricsEnabled = enableMetrics ?? _experiments?.enableMetrics ?? true;

Nothing in Sentry.init sets enableMetrics, so we're relying on that ?? true fallback. A minor bump that flips the default silently kills all usage telemetry, with no test failure anywhere in the stack — the unit tests mock @sentry/node, and the only oracle that would catch it is the e2e in #414.

Suggest pinning to 10.73.0 and passing enableMetrics: true explicitly in the Sentry.init options, so the behaviour is stated rather than inherited.

Test describe is mislabelled

packages/@cdktn/cli-core/src/test/error-reporting.test.ts:42 reads describe("consent gating (initializErrorReporting)", ...), but neither test covers consent gating — one asserts the fresh trace seeding, the other the pinned init options. Something like "Sentry init hardening" would match the contents. (Consent gating does get covered in #411, which is presumably where the name came from.)

@so0k
so0k force-pushed the tele/s1-sentry-sdk-10 branch from 88ab178 to 84c9f09 Compare September 10, 2026 04:07
@so0k

so0k commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Both done. @sentry/node is pinned to exactly 10.73.0 in the three package.json files, and Sentry.init now passes enableMetrics: true with a note that the SDK default would otherwise be inherited, so the behaviour the later slices depend on is stated rather than assumed; the init-options test asserts it. The describe is renamed to "Sentry init hardening". Also rebased onto main after #406 (package.json and lockfile conflicts resolved, lockfile regenerated from main's), with the rest of the stack rebased on top. Not merging yet.

@sakul-learning

Copy link
Copy Markdown
Contributor

Approved ✅

Verified the Sentry v7 → v10 migration against the installed @sentry/core@10.73.0 type declarations:

  • Sentry.configureScope(...)Sentry.getCurrentScope() in packages/@cdktn/cli-core/src/lib/error-reporting.ts and packages/@cdktn/commons/src/errors.ts is the correct v10 migration; setTransactionName and setPropagationContext both exist on Scope.
  • The Sentry.init options added in error-reporting.tstracesSampleRate: 0, environment: "production", serverName: "cdktn-cli" — all exist in v10. autoSessionTracking (removed here) no longer exists in v10, so dropping it is correct.
  • The pnpm-lock.yaml delta moves only @sentry/* and its transitives (the v10 package split plus OpenTelemetry/instrumentation deps), removing the v7-only localforage / immediate / lie.

Checks run:

  • pnpm exec nx test @cdktn/cli-core --runInBand — 168 passed (including the new error-reporting.test.ts); 1 suite skipped (missing dist files)
  • pnpm exec nx test @cdktn/commons --runInBand — 84 passed
  • pnpm exec nx build cdktn — clean
  • pnpm exec nx build cdktn-clitsc --noEmit clean
  • pnpm exec nx test cdktn-cli --runInBand — 34 passed

No blockers.

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.

3 participants