fix(node): Build-time instrument the ESM build of graphql - #23669
Merged
Conversation
mydea
force-pushed
the
fn/graphql-esm-instrument
branch
3 times, most recently
from
August 27, 2026 08:29
8688a44 to
102018c
Compare
Contributor
size-limit report 📦
|
The graphql orchestrion config only listed graphql's CommonJS file paths (`language/parser.js`, ...). graphql ships dual CJS/ESM per-file builds and the orchestrion matcher compares `filePath` exactly, so bundlers resolving graphql's ESM build (webpack with `outputModule`, vite, rollup, rolldown) never matched and left graphql uninstrumented at build time — the app ran but produced no graphql spans. Node's native ESM resolver ignores the `module` field and loads graphql's CJS build, so the runtime `--import` path (and the node-integration-tests) were unaffected; this gap only hit the bundler plugin path. Mirror the existing dual-path pattern from the `openai` config: emit one entry per built file (`.js` for `require`, `.mjs` for `import`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mydea
force-pushed
the
fn/graphql-esm-instrument
branch
from
August 27, 2026 09:35
102018c to
df48e30
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit df48e30. Configure here.
| }, | ||
| module: { name: 'graphql', versionRange: '>=14.0.0 <17', filePath }, | ||
| functionQuery: { functionName: 'execute', kind: 'Auto' as const }, | ||
| })), |
There was a problem hiding this comment.
Fix PR lacks regression tests
Medium Severity
This fix PR adds .mjs matcher entries so bundlers that resolve graphql's ESM build emit spans, but the diff includes no unit, integration, or E2E test that would fail without those entries. Review guidelines for fix PRs require a regression test covering the repaired path. I flagged this because it was mentioned in the review rules. The description notes a stacked test PR; landing that coverage with this change would satisfy the requirement.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit df48e30. Configure here.
mydea
marked this pull request as ready for review
August 27, 2026 11:20
mydea
requested review from
msonnb and
stephanie-anderson
and removed request for
a team
August 27, 2026 11:20
msonnb
approved these changes
Aug 27, 2026
mydea
added a commit
that referenced
this pull request
Aug 28, 2026
…time (#23670) Turns the five node bundler apps (`node-webpack`, `node-vite`, `node-rollup`, `node-rolldown`, `node-esbuild`) from a static banner-grep into a real runtime test, in the spirit of [orchestrion-treeshake-repro](https://github.com/logaretm/orchestrion-treeshake-repro): actually run the bundled service against a real `graphql` package and verify what gets injected — across **both** the build-time (bundler plugin) and runtime (`--import` hook) instrumentation paths. Each app builds the same `graphql` workload four ways and runs each bundle: - `plain` — graphql inlined, no plugin, no `--import`: **no** graphql spans (negative control), - `plugin` — graphql inlined, Sentry bundler plugin, no `--import`: one set of spans, via build-time injection, - `plain-external` — graphql external, `--import`: one set, via the runtime hook, - `plugin-external` — graphql external, plugin, `--import`: one set — the plugin can't touch an external module, so the runtime hook is the sole injector and there's no double instrumentation. "One set" is defined relative to the build-time run, so the count stays correct across bundlers and graphql versions. This guards the fix beneath it (#23669) and catches the whole class of "bundle looks instrumented but emits no/double spans" regressions the static grep couldn't. ### Shared assertion helper The per-app `assert.mjs` was ~86 identical lines copied five times. It now lives once in `@sentry-internal/test-utils` as `assertBundlerInstrumentation('graphql')`, and each app's `assert.mjs` is a single call. The helper is parameterized by a small fixture descriptor (`{ moduleName, origin, sourceMarker, assertResult }`) rather than hardcoded to graphql, so a future library is one registry entry plus a `src/app.mjs` workload — everything else (the four-variant matrix, the "one set" logic, the shape checks) is generic. `entry.mjs` is likewise library-agnostic: it runs a conventional `runWorkload()` and ships whatever it returns back as `result`. ### Decisions - **Control inlining with Vite's SSR knob, not `rollupOptions.external`.** A Vite SSR build externalizes deps by default, so `rollupOptions.external` never governed whether graphql was bundled — the "inlined" variants were silently shipping an external graphql and not exercising the build-time path at all. The variants now set `ssr.noExternal` / `ssr.external` explicitly (the latter also wins over the plugin's own force-bundle, leaving graphql for the runtime hook). - **Assert the bundle _shape_, not just the span counts.** The counts can come out right by accident even when the externalization knob is a no-op, so each variant is also checked to actually be inlined (carries graphql's own source, no bare `graphql` import) or external (keeps the bare import, no inlined source). This scans every emitted chunk in the output dir, because bundlers split the entry's `await import('./app.mjs')` into a sibling chunk (webpack) where the marker lands rather than in `main.*`. - **Hand the result back through a file, not stdout.** `console.log(...)` + `process.exit()` can truncate or EPIPE when stdout is a pipe (the exit lands before the buffered write drains). The entry `writeFileSync`s its result to `SENTRY_E2E_RESULT_FILE` and the helper reads it back; a boot crash or a missing file surfaces as an explicit failure instead of being swallowed. - **Isolate the build-time path.** The entry sets `enableRuntimeChannelInjection: false` and the non-`--import` bundles run with a plain `node`, so the bundler plugin is the only possible injector — making the `plain` build a true negative rather than something the runtime hook could rescue. - **Capture via the `spanEnd` hook** (+ `spanToJSON`) rather than `beforeSendSpan`, so collection is independent of transport and trace lifecycle. Transport is a no-op and the DSN fake — nothing hits the network. - **Entry is an async function, not top-level await**, so the same source bundles to both ESM and esbuild's CJS node output. - **esbuild emits CJS**, the other four emit ESM. esbuild's ESM output can't perform the CJS `require('node:async_hooks')` that `@sentry/server-utils` does once inlined (an esbuild CJS-in-ESM interop limit, the "separate bug" the repro noted); CJS output is the normal esbuild node target and sidesteps it. This also means esbuild resolves graphql's CJS build, so the suite covers both the `.js` and `.mjs` orchestrion paths. - **Dropped the banner-grep** assertion: the runtime span check is a strictly stronger proof that build-time injection ran, and the banner's `new Set()` formatting varies by bundler. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: isaacs <i@izs.me>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Build-time instrumentation of
graphqlsilently produced no spans when a bundler resolved graphql's ESM build. The orchestrion transform ran, the app worked, but noauto.graphql.diagnostic_channelspans were emitted — the same failure class as orchestrion-treeshake-repro.Root cause
graphql ships dual CJS/ESM per-file builds (
language/parser.jsandlanguage/parser.mjs, etc.), and the orchestrion matcher comparesfilePathexactly. The graphql config only listed the CommonJS paths, so a bundler that resolves graphql's ESM build never matched:modulefield → loadindex.mjs→.mjsfiles → no match → uninstrumented.module(graphql has noexportsmap) →import 'graphql'loadsindex.js(CJS) →.jsfiles → matched. So the runtime--importpath and the existing node-integration-tests (CJS and ESM) were unaffected; this gap was bundler-path-only.This is why the issue went unnoticed: everything that goes through Node's loader used the CJS build.
Fix
Mirror the dual-path pattern already used by the
openaiconfig: emit one entry per built file (.jsforrequire,.mjsforimport). No behavior change for the CJS path.The stacked test PR on top exercises this across webpack/vite/rollup/rolldown (ESM) and esbuild (CJS).