feat(bundlers): Add orchestrion bundler plugins#22124
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 5 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b15cb00. Configure here.
…hestrion-bundler-plugins
isaacs
left a comment
There was a problem hiding this comment.
- Need to keep
@apm-js-collab/code-transformeras a dep, because we export the types (would break pnpm) - I think we should add a "things will be externalized and we can't prevent it" warning to the non-Vite plugins, similar to what was done for the bun plugin.
Otherwise, clearly a step in the right direction which we've been discussing. 👍
| * ```ts | ||
| * // vite.config.ts | ||
| * import { sentryOrchestrionPlugin } from '@sentry/node/orchestrion/vite'; | ||
| * import { sentryOrchestrionPlugin } from '@sentry/server-utils/orchestrion/vite'; |
There was a problem hiding this comment.
Good catch, missed that when we moved it into this location!
| "magic-string": "~0.30.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@apm-js-collab/code-transformer": "^0.15.0", |
There was a problem hiding this comment.
Demoting this to a devDep would break the published types, because we re-export the InstrumentationConfig from it. (Would still work in Yarn and npm via the transitive dep, but not pnpm.)
There was a problem hiding this comment.
ahhhh. Part of me feel like these should be re-exported from both of the other packages!
| return { | ||
| instrumentations: [...SENTRY_INSTRUMENTATIONS, ...(options.instrumentations || [])], | ||
| injectDiagnostics: () => { | ||
| return '(globalThis.__SENTRY_ORCHESTRION__=globalThis.__SENTRY_ORCHESTRION__||{}).bundler=true;'; |
There was a problem hiding this comment.
If this is now done unconditionally, couldn't that sometimes run into problems with the check in packages/server-utils/src/orchestrion/register.ts, in hybrid cases where a package remains external?
// Already injected (runtime --import hook or bundler plugin) — nothing to do.
if (g.runtime || g.bundler) {
return;
}If we're using a bundler like vite, where we an very reliably de-externalize anything orchestrion wants to instrument, but a case like rollup or esbuild with packages: 'external' or external: ['*']. I thought this was the reason for the comment in the webpack plugin in packages/server-utils/src/orchestrion/bundler/webpack.ts, saying it intentionally does not inject the .bundler flag because we can't guarantee things aren't externalized.
The approach I ended up taking in the Bun plugin, after some back and forth trying to find a way around it (and failing) was to just detect these blanket-external cases and warn about it at bundle time.
There was a problem hiding this comment.
This makes me wonder if we need a more comprehensive tracking of what actually gets instrumented by the bundler along the way, which can be injected into some kind of global registry, and then the runtime instrumentation can filter those out. Would be more complicated, but might close the gap without having to predict the future based on bundler type and precise configs 🤔
There was a problem hiding this comment.
Because something has been injected at bundler time, why should this disable runtime injection?
There is no risk of "double-wrapping" so I don't understand why we impose this limitation?
There was a problem hiding this comment.
(summarizing discussion from slack)
Yes, you're right, double-wrapping is not a concern, unless we bundle directly over the source file in the same location in node_modules, which would be... confusing.
I think for the time being, if you agree, this seems most prudent:
- let's keep the "force non-externalizing orchestrion things" in the bundlers where we can reliably
- warn at bundle time if we can't do that reliably
- write up the permutations so we can make a 👍/👎 bikeshed decision on each one
- remove the
|| g.bundler) {check from server-utils'sregister.tsmodule
| type UnknownPlugin = any; | ||
|
|
||
| import codeTransformer from '@apm-js-collab/code-transformer-bundler-plugins/vite'; | ||
| import MagicString from 'magic-string'; |
There was a problem hiding this comment.
Looks like we can remove this from packages/server-utils/package.json, yeah? (Ignore if it is going to be used again at some point in the follow-on PR.)

There was already a Vite bundler plugin for orchestrion that included our instrumentations but this PR adds the ability to include additional instrumentations and adds plugins for other bundlers.
I left the Bun plugin out of
@sentry/server-utilsbecause there is already one in the Bun package and I can't see it being used anywhere else 🤷♂️