fix(node): Detect + warn when the orchestrion runtime hook is bundled - #23675
fix(node): Detect + warn when the orchestrion runtime hook is bundled#23675mydea wants to merge 11 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
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 1540609. Configure here.
mydea
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| ## Troubleshoot | ||
|
|
||
| If your server-side auto-instrumentation stops recording spans after bundling (e.g. certain Nitro |
There was a problem hiding this comment.
Should this also be added to the Nitro SDK readme?
There was a problem hiding this comment.
i'd look into a follow up here overall to try to fix this in nitro, if possible!
| // instrumentation was already injected at build time and the runtime hook is redundant — this is | ||
| // an expected, supported setup, so stay quiet (debug-only). Otherwise nothing is instrumented, so | ||
| // surface an always-on, actionable warning. | ||
| if (marker.bundler instanceof Set) { |
There was a problem hiding this comment.
We already have some bundler instanceof Set guards in some files. Should we create a helper named bundlerPluginRun() or something? Then it's also more understandable what it does.
There was a problem hiding this comment.
hmm I think most other places I've seen (or found at least) are more type-guarding this which seems a bit different of a use case then this?
There was a problem hiding this comment.
👀
hm, yes then a method actually does not really make sense. disregard this :D
| debug.warn( | ||
| 'Failed to register diagnostics-channel injection hooks; channel-based integrations will not record spans.', | ||
| error, |
There was a problem hiding this comment.
This was a debug.warn before (gated with the debug flag). Is this on purpose, that this should now always be printed?
There was a problem hiding this comment.
I think this should be a proper console warn, this means nothing will work really so users should know 😅
`@sentry/node`'s `init()` installs a runtime module-transform hook from `@sentry/server-utils/orchestrion/register`, which drives a vendored code transformer (meriyah/astring/source-map) and is designed to run from `node_modules`. If a downstream bundler inlines and tree-shakes `@sentry/server-utils`, that transformer is stripped to empty objects, so at runtime `parse`/`generate` are `undefined` and the first module the hook tries to transform throws `TypeError: parse is not a function` — deep in the loader, once per module, and only when `debug: true` (otherwise it fails silently). Detect this once, up front: run a throwaway in-memory transform over a synthetic snippet before installing any hook. A healthy build returns normally; a tree-shaken one throws a `TypeError`. On detection, emit a single, always-on, actionable warning (via `consoleSandbox`, deduped on a global marker) and skip installing hooks that can't work, instead of letting the cryptic per-module error surface. The existing registration `catch` is likewise upgraded to an always-on warning. All of this lives inside `registerDiagnosticsChannelInjection`, so it tree-shakes away with the whole block when `bundleSizeOptimizations.excludeChannelInjection` sets `__SENTRY_CHANNEL_INJECTION__` to `false`. Ref #23664 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n plugin The runtime hook (reached via `@sentry/node`) must stay external so it resolves from `node_modules`; bundling it strips the transformer and breaks the `Module.register` self-reference. `@sentry/node` is a different package from the `@sentry/server-utils` barrel the plugin force-bundles (`ssr.noExternal`), so the vite plugin now also adds `@sentry/node` to `ssr.external`. Explicit `ssr.external` entries win over `noExternal`, so this holds even against a preset that sets `ssr.noExternal: true` — verified with a real vite SSR build. This covers the vite-based frameworks (SvelteKit, Astro, React Router, TanStack); the nitro/rollup frameworks (Nuxt, SolidStart) rely on the runtime warning above, with a nitro-level externalization guard as a follow-up. Ref #23664 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a "Bundling your server" note to the Node README (and a Nuxt troubleshoot note) explaining that the runtime instrumentation hook must stay external, and pointing to the build-time bundler-plugin instrumentation as the alternative. Ref #23664 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Forcing `@sentry/node` into `ssr.external` broke Cloudflare/worker builds: the shared vite orchestrion plugin also runs under `@cloudflare/vite-plugin` (and frameworks deploying to workerd), where `@sentry/node` is unused and setting `resolve.external` on a worker environment is rejected outright — and the worker environment is even named `ssr`, so there's no reliable node-vs-worker discriminator in the `config()` hook. Vite already externalizes `@sentry/node` for node SSR by default anyway, and the runtime probe in `orchestrion/register` covers the cases where it does get bundled, so drop the forced externalization. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… a bundled hook If `@sentry/server-utils` was bundled AND the build-time bundler plugin ran (a defined `__SENTRY_ORCHESTRION__.bundler` Set), instrumentation is already injected at build time and the runtime hook is redundant — a supported setup. In that case downgrade the "bundled" message to a debug log instead of an always-on warning. The always-on warning now fires only when nothing instrumented the app (bundled and no build-time plugin). Also corrects the Node README: bundling doesn't disable auto-instrumentation when the build-time plugin is used. Ref #23664 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1bb710e to
da0de0d
Compare
…nto a dedicated package The runtime diagnostics-channel injection (`register`/`hook`/`import-hook` + the vendored transformer chain meriyah/esquery/astring/source-map/tracing-hooks) must stay EXTERNAL when apps bundle their server, while the rest of `@sentry/server-utils` (barrel + config + bundler plugins) is meant to be bundled. Sharing one package made externalization fiddly. This moves the runtime injection into a new package, `@sentry/server-runtime-injection`, so "keep this external" is a clean package boundary. - New package holds `register`/`hook`/`import-hook` (subpaths `/register`, `/hook`, `/import-hook`) and vendors the transformer chain; it depends on `@sentry/server-utils` and imports `SENTRY_INSTRUMENTATIONS` from `./orchestrion/config` (config stays put). - `@sentry/server-utils` drops the runtime dir/exports and the runtime-only vendored deps; the lone `config/index.ts` bundler re-export moves to a new `./orchestrion/bundler-transforms` subpath so importing config stays transformer-free (bun updated). - References updated: node SDK + test mock, deno import, the shared `--import` template + `makeOrchestrionLoader` guard, Next.js externalization (`ORCHESTRION_RUNTIME_EXTERNAL_PACKAGES`, nextjs-anchored require-ability check, forwarder generation) + tests, `.size-limit.js`. - Dep added to runtime-injection consumers (node, nextjs, deno, aws/gcp-serverless, astro, tanstackstart-react); package registered in workspaces, `.craft.yml`, CODEOWNERS, aws e2e. Stacked on the detect/warn PR (#23675). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| .getTransformer('@sentry/orchestrion-probe', '0.0.0', 'probe.js') | ||
| ?.transform('class C { async m(x) { return x; } }', 'esm'); | ||
| return false; |
There was a problem hiding this comment.
Bug: The tree-shaking detection can fail silently. If getTransformer() returns undefined, the optional chaining on .transform() will cause the check to pass incorrectly, leading to broken hooks being installed.
Severity: HIGH
Suggested Fix
Remove the optional chaining from the .transform() call. Instead, explicitly check if the value returned by getTransformer() is undefined. If it is, treat it as a failure case by throwing an error. This ensures that a non-matching probe configuration correctly signals a problem with the transformer.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/server-utils/src/orchestrion/runtime/register.ts#L54-L56
Potential issue: The tree-shaking detection mechanism uses optional chaining on a
`.transform()` call. The preceding function, `getTransformer()`, can return `undefined`
if the synthetic probe configuration does not match. In this scenario, the optional
chaining (`?.`) will cause the `.transform()` call to be skipped silently, without
throwing an error. The function will then incorrectly determine that the transformer is
healthy and proceed to install broken runtime hooks. This defeats the purpose of the
check and will lead to the original `TypeError: parse is not a function` error during
module loading.
…nto a dedicated package The runtime diagnostics-channel injection (`register`/`hook`/`import-hook` + the vendored transformer chain meriyah/esquery/astring/source-map/tracing-hooks) must stay EXTERNAL when apps bundle their server, while the rest of `@sentry/server-utils` (barrel + config + bundler plugins) is meant to be bundled. Sharing one package made externalization fiddly. This moves the runtime injection into a new package, `@sentry/server-runtime-injection`, so "keep this external" is a clean package boundary. - New package holds `register`/`hook`/`import-hook` (subpaths `/register`, `/hook`, `/import-hook`) and vendors the transformer chain; it depends on `@sentry/server-utils` and imports `SENTRY_INSTRUMENTATIONS` from `./orchestrion/config` (config stays put). - `@sentry/server-utils` drops the runtime dir/exports and the runtime-only vendored deps; the lone `config/index.ts` bundler re-export moves to a new `./orchestrion/bundler-transforms` subpath so importing config stays transformer-free (bun updated). - References updated: node SDK + test mock, deno import, the shared `--import` template + `makeOrchestrionLoader` guard, Next.js externalization (`ORCHESTRION_RUNTIME_EXTERNAL_PACKAGES`, nextjs-anchored require-ability check, forwarder generation) + tests, `.size-limit.js`. - Dep added to runtime-injection consumers (node, nextjs, deno, aws/gcp-serverless, astro, tanstackstart-react); package registered in workspaces, `.craft.yml`, CODEOWNERS, aws e2e. Stacked on the detect/warn PR (#23675). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Raised what I think is a pretty significant false-alarm issue, but the intent is sound and the scoping decision, to keep the hook external rather than make a bundled copy work, definitely matches the constraints. The core detection works: I reproduced it end-to-end against an esbuild bundle and it fires correctly.
| // instrumentation was already injected at build time and the runtime hook is redundant — this is | ||
| // an expected, supported setup, so stay quiet (debug-only). Otherwise nothing is instrumented, so | ||
| // surface an always-on, actionable warning. | ||
| if (marker.bundler instanceof Set) { |
There was a problem hiding this comment.
There's an interesting issue here that'll lead to false alarms, because the esbuild can fail to add the banner, and then print a warning telling the user to add the bundler plugin they're already using.
The root cause is that the upstream entryOutputPaths/configuredEntryPoints in node_modules/@apm-js-collab/code-transformer-bundler-plugins/dist/esm/esbuild.mjs match metafile.outputs[].entryPoint against the configured entry point paths. @sentry/bundler-plugins/esbuild debug-id injection rewrites the entry, so eg a.mjs becomes a.mjs?sentryDebugIdProxy=true, which never matches, and the banner is skipped.
This is a pre-existing gap, but as of this PR, we're depending on it for the user-visible alarm, so it becomes a tangible problem that we should fix, imo. It wasn't a problem before, because nothing read the banner, we always did stuff like (g.bundler ??= new Set()).add(module), so the point where it was created didn't ever matter before.
We have some tests in node-integration-tests that could've caught this, but didn't check for this situation specifically. Applying this patch adds a test that reproduces the issue: https://gist.github.com/isaacs/0f0633e3f8af0b078a973448abcf0955
Also, ESM import hoisting will effectively put the banner after the call to Sentry.init() in a code-split build, so we'll get chunks like:
;(function(){…g.bundler=g.bundler||new Set();})();import "./chunk-cafebad0.js";We can address both of these, though, by removing the probe, and then moving the check out of this function and down into the setDiagnosticsHook error callback function on line 128, which also reduces out a layer.
That callback fires when a module actually fails to transform, by which point every banner has run, so the ordering problem disappears. It also drops the init cost, and can name the failing module. The one behavior change is that there's no warning if the transformer is stripped but no instrumented module ever loads. But I think that's the correct outcome, since nothing was lost in that case.
These two patches would implement the test and suggested fix: https://gist.github.com/isaacs/8f58c5e1f9bac3479be2e9f2c6ee578d or pull the top two commits from isaacs/review-23675
| // runs both at `--import` time and (synchronously) inside `Sentry.init()`, | ||
| // so an unguarded throw would either abort startup or make `init()` throw. | ||
| // On any failure (e.g. dep resolution, `require(esm)` / Node-compat | ||
| // incompatibility) we warn (DEBUG only) and continue without channel | ||
| // injection |
There was a problem hiding this comment.
low/question: I think this comment might be stale now?
3 participants
@sentry/node'sinit()installs a runtime module-transform hook from@sentry/server-utils/orchestrion/register, which drives a vendored code transformer (meriyah/astring/source-map) and is designed to run fromnode_modules(external). Issue #23664 reported that when a downstream bundler inlines and tree-shakes@sentry/server-utils, that transformer is stripped and instrumentation breaks.Root cause & why this approach
The runtime hook is broken two ways when bundled: (1) tree-shaking empties meriyah/astring/source-map, and (2)
Module.register('@sentry/server-utils/orchestrion/hook', { parentURL: <own __filename> })is a self-reference that only resolves from the real on-disk path. So bundling the hook is fundamentally unsupported — the fix is to keep it external (or instrument at build time), not to make the bundled copy work. The defaultnode --importsetup and well-configured frameworks (Next.js already guards this) are unaffected. Today it also fails silently (debug.warnis gated behinddebug: true).An earlier attempt (draft PR #23668) instead co-located the vendored chunk so the bundled transformer survived tree-shaking — but that added ~61 kB to every bundle that inlines
@sentry/server-utils, only partly fixed the problem, and optimised a discouraged config. That PR is closed in favour of this one.What this does
register.ts): a cheap in-memory probe runs the transformer over a synthetic snippet before installing any hook. When it detects the transformer was tree-shaken:__SENTRY_ORCHESTRION__.bundlermarker) → instrumentation is already injected at build time, so the runtime hook is redundant and we stay quiet (debug log only) — a supported setup;TypeError.Verified end-to-end against tree-shaken esbuild bundles (warns without build-time, silent with it).
registerDiagnosticsChannelInjection, sobundleSizeOptimizations.excludeChannelInjection(__SENTRY_CHANNEL_INJECTION__ = false) strips it and the transformer entirely — verified.Not included: forcing
@sentry/nodeinto vitessr.externalwas explored but dropped — the shared vite plugin also runs under@cloudflare/vite-plugin, which forbidsresolve.externalin worker environments (where@sentry/nodeis unused), and there's no reliable node-vs-worker discriminator in theconfig()hook. Vite already externalizes@sentry/nodefor node SSR by default, and the runtime detection covers the rest.No size-limit impact:
@sentry/nodestays at 119.76 kB (limit 123 kB); no +61 kB.Follow-ups (not in this PR)
node-orchestrion-webpackapp only greps a config string that survives tree-shaking).Fixes #23664
🤖 Generated with Claude Code