feat(nextjs): Add opt-in for orchestrion instrumentation#22043
feat(nextjs): Add opt-in for orchestrion instrumentation#22043chargome wants to merge 12 commits into
Conversation
Two fixes for diagnostics-channel injection under Turbopack:
Externalize @apm-js-collab/tracing-hooks and @apm-js-collab/code-transformer
when the flag is on. Bundled, the code transformer's parser breaks
('a.parse is not a function'), so the runtime module hook silently returned
untransformed sources and externalized packages never produced spans.
Keep mysql in serverExternalPackages instead of force-bundling it. Turbopack
cannot bundle mysql 2.x correctly (the wire-protocol handshake fails with
'Received packet in the wrong sequence' even untransformed). External, it is
now instrumented by the working runtime hook instead of the build-time loader.
Also drop the bundler marker from the orchestrion webpack plugin: it made
registerDiagnosticsChannelInjection() skip the runtime hook, but the hybrid
setup (loader for bundled deps, runtime hook for external ones) needs both.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nto cg/nextjs-orchestrion-support
Instead of un-externalizing every instrumented package and forcing the Next-default-external ones through transpilePackages, only packages on an explicit bundle-safe allowlist (currently ioredis) are removed from Sentry's own serverExternalPackages defaults. Everything else — Next's defaults, the user's externals, the rest of Sentry's defaults — stays external and is instrumented by the runtime module hook on require, which works since the orchestrion machinery is externalized. This is safer: bundling a server package changes real behavior (mysql 2.x corrupts its wire protocol when bundled by Turbopack), and new upstream instrumentations (e.g. hapi) now default to the external/runtime-hook path instead of silently becoming bundled. It also removes the Next server-external-packages list parsing and the pg-native webpack workaround, both only needed to support force-bundling. Verified in the nextjs-16-orchestrion e2e: ioredis via the build-time loader, pg and mysql via the runtime hook. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t@13 Use createRequire(__filename) instead of aliasing the CJS require in the orchestrion webpack bundler module. Next.js 13 bundles @sentry/nextjs into the server build, and webpack flags the aliased require with 'Critical dependency: require function is used in a way in which dependencies cannot be statically extracted', which the nextjs-app-dir e2e treats as a failure. Add isDiagnosticsChannelInjectionEnabled to the consistent-exports ignore list: like its companions experimentalUseDiagnosticsChannelInjection and diagnosticsChannelInjectionIntegrations, the Node-runtime-only opt-in is not surfaced through the framework / serverless SDKs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The plugin previously ran for all server compilations, including the edge runtime, which diagnostics-channel injection does not target. Gate it on the already-derived runtime instead of isServer and add unit tests covering the server/edge/client/flag-off cases. Also trim down the orchestrion-related comments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a3a3595. Configure here.
| expect(externals).toContain('mysql'); | ||
| expect(externals).not.toContain('@apm-js-collab/tracing-hooks'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Feat lacks integration or E2E
Medium Severity
This feature PR adds unit and webpack config tests but no integration or E2E test in the diff, though the description references separate e2e verification. Review guidelines expect at least one integration or E2E test for feat changes.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit a3a3595. Configure here.
| // module don't emit a "Critical dependency" warning. | ||
| function getOrchestrionRequire(): ReturnType<typeof createRequire> { | ||
| let nodeRequire: ReturnType<typeof createRequire>; | ||
| /*! rollup-include-cjs-only */ |
chargome
requested review from
a team,
JPeer264,
andreiborza,
logaretm,
mydea,
nicohrubec,
s1gr1d and
timfish
and removed request for
a team,
JPeer264,
andreiborza and
s1gr1d
When the SDK is bundled into a Next.js server build, the runtime module hook's bare require of @apm-js-collab/tracing-hooks resolves relative to the emitted chunk, which fails under isolated installs (pnpm) where the package is not linked at the app root — the hook silently no-ops and externalized packages (pg, mysql) lose their spans. Resolve the package location in withSentryConfig, where the SDK is a real on-disk package, and inline it as a build-time env value that the runtime prefers over the bare specifier. Also construct nodeRequire via createRequire in both build flavors so bundlers don't statically trace the call (Turbopack otherwise tries to resolve the injected absolute path at build time). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
size-limit report 📦
|
|
I guess the Ideally we should avoid using config options to enable orchestrion mode because it requires all these hacks. I also think this will result in webpack not including the orchestrion code in a bundle even if you use it and result in a runtime error when it can't be loaded. For the Node SDK we have The only valid use I've seen for |
timfish
left a comment
timfish
left a comment
There was a problem hiding this comment.
The getTracingHooksSpecifier obfuscation will mean that no other bundler will be able to resolve and bundle @apm-js-collab/tracing-hooks/hook-sync.mjs. This will force us (and users) to make it external everywhere.
I think in the long term we need to solve the Turbopack issue (happy to take a look!) but for now to get this merged I would create a nextjs specific registerDiagnosticsChannelInjection() so only nextjs has this obfuscation!
registerDiagnosticsChannelInjection() now takes an optional tracingHooksDir and by default loads @apm-js-collab/tracing-hooks via its bare specifier again, keeping the require statically analyzable for bundlers. Only the Next.js SDK — whose server builds bundle the SDK and can't resolve the bare specifier under pnpm — passes the build-time-resolved package location (via a Next.js-specific experimentalUseDiagnosticsChannelInjection wrapper), where loading switches to an opaque createRequire. webpack ignore-comments were evaluated as an alternative: turbopackIgnore works on require(), but webpack only honors webpackIgnore on import() and compiles the call to a broken module stub, so createRequire it is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
I reworked the pr so we have a nextjs specific On magic comments: Once the transformer becomes bundle-safe, we can update the code and internalize all the modules for both bundlers. |
timfish
left a comment
timfish
left a comment
There was a problem hiding this comment.
Great bundler wrangling!
3 participants
Adds an experimental option for opting in to orchestrion instrumentation.
mysql) stay external and are instrumented by the runtime module hook, which works because we also externalize the@apm-js-collab/*transformer packages.transpilePackagesfor the ones that Next externalizes by default (e.g.pg) — removing them from serverExternalPackages isn't enough for Next's own defaults.Verified in e2e
closes #22009