Repository navigation
fix(node): stop esbuild failing when @opentelemetry/api is not installed - #5422
posthog[bot] merged 2 commits into
Conversation
Read the active span from the global registry of @opentelemetry/api v1. Do not import the optional peer. A literal import made esbuild fail the build when the peer was not installed. A variable import makes webpack warn and drop the module. Add a test that bundles the built ESM and CJS entrypoints with esbuild when @opentelemetry/api cannot resolve. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 0d603b98-9825-4f1c-8261-e50c33c8a122
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-adding the stamphog label gives the same result unless the changed files change.
stamphog can't approve this pull request automatically, and the refusal is final. The deny-list gate flagged it for the deps_toolchain category, most likely because it adds esbuild to packages/node/package.json and changes pnpm-lock.yaml. The tier gate also classified it as T2-never (55 lines across 5 files, touching two areas), so it needs a human reviewer. The size gate passed, so splitting is not required. The author can ask a maintainer to review it. Moving the dependency and lockfile change into its own pull request might also make the remaining change easier to review.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: deps_toolchain |
| size | ✓ | 19L, 3F substantive, 55L/5F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (55L, 5F, two-areas, fix) |
| stamphog 2.6.1 | .stamphog/policy.yml @ unknown · reviewed head 8e04f6a |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Coherence4 spec problems · 0 failing chokepoints · 2 spec gaps · 0 guard failures 44 components, 7 bullets (5 invariants, 2 requirements), 4 problems Spec problems (spec --check)
Chokepoints: none declared. Spec gaps: 2 entrances with outside or unknown trust and no traced control, and 10 more held by the adoption baseline
Close one with |
Node SDK compliance v2Commit: 4a0232c Capture v0 — acceptance✅ 28 passed / ❌ 3 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: Harness exit: 1; report validation exit: 1. Capture v1 — acceptance✅ 28 passed / ❌ 3 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: Harness exit: 1; report validation exit: 1. Capture v0 — migration✅ 55 passed / ❌ 1 non-passing / Drill-down: run artifacts → ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. Capture v1 — migration✅ 120 passed / ❌ 1 non-passing / Drill-down: run artifacts → ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. |
…iling-on-52ac87 Generated-By: PostHog Desktop Task-Id: a33bb94c-53b3-481d-bb7a-146daa4a51fe
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-adding the stamphog label gives the same result unless the changed files change.
stamphog refused this pull request at two gates. The deny-list gate flagged it for deps_toolchain, which here is the new esbuild dev dependency in packages/node/package.json and the matching pnpm-lock.yaml change. The tier gate classified it as T2-never, because it touches two areas (the trace-context.ts fix plus the dependency and toolchain changes), and that tier is never auto-approved. The size gate passed, so splitting the PR for size isn't needed.
The next step is to ask a human reviewer to look at it. The author could also split it so the trace-context.ts change and its test are separate from the dependency change, though a human would still need to review both parts.
- pauldambra reviewed the current head.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: deps_toolchain |
| size | ✓ | 19L, 3F substantive, 55L/5F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (55L, 5F, two-areas, fix) |
| stamphog 2.6.1 | .stamphog/policy.yml @ unknown · reviewed head 4a0232c |
Problem
posthog-node5.56.1 or later with esbuild get a failed build when they do not install the optional peer@opentelemetry/api. This blocks deploys after a routine version bump (AWS CDKNodejsFunction, SST, Serverless esbuild, tsup withnoExternal).trace-context.tsused a literalimport('@opentelemetry/api'). The runtimetry/catchcannot help, because esbuild resolves a literal specifier at bundle time.Origin
Changes
getActiveTracePropertiesnow reads the active span from the global registry of@opentelemetry/apiv1 (Symbol.for('opentelemetry.js.api.1')) and the span context key (Symbol.for('OpenTelemetry Context Key SPAN')). The package does not import OpenTelemetry anymore, so no bundler tries to resolve it.esbuild-bundle.spec.tsbundles the built ESM and CJS entrypoints with esbuild while@opentelemetry/*cannot resolve. It fails onmainand passes with this change.test:unitalready depends onbuildin turbo, so the test readsdist.esbuildis now a dev dependency ofposthog-node(same^0.25.0range as other packages, version already in the lockfile).import()(before)import(variable)node_modulesis not deployedWarning
The global key is how all copies of
@opentelemetry/apiv1 share one context manager, but it is not public API. A future@opentelemetry/apiv2 would use a new key. Our peer range is^1.2.0, so v2 is out of range today. If the key changes, the only effect is that exceptions get no$trace_id/$span_id. Capture still works.@opentelemetry/apiimports, but in a newposthog-node/otelentrypoint. The new test covers the main entrypoints only, so feat(node): add request context to OpenTelemetry spans #5314 should confirm that the main entrypoints do not import that module.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Fully autonomous
/* webpackIgnore: true */. rslib (rspack) removes webpack and Turbopack magic comments fromdist, also withimportDynamic: false(rslib already sets it). A local webpack 5 build ofimport(variable)gave the "Critical dependency" warning, so I rejected that approach (see the table).exception-trace-context.spec.tstests use the real@opentelemetry/apiwithAsyncLocalStorageContextManager, so they now verify the global-registry read path.posthog-nodelint,check-types,test:unit(all pass),test:edge-compat,oxfmt --check,test:dependency-cooldown. I did not do a manual test in a real CDK or SST project.Created with PostHog Desktop from this inbox report, addressing #5420.
Closes #5420
🤖 Generated with Claude Code