Skip to content

feat(bundlers): Add orchestrion bundler plugins#22124

Open
timfish wants to merge 8 commits into
developfrom
timfish/feat/orchestrion-bundler-plugins
Open

feat(bundlers): Add orchestrion bundler plugins#22124
timfish wants to merge 8 commits into
developfrom
timfish/feat/orchestrion-bundler-plugins

Conversation

@timfish

@timfish timfish commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

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-utils because there is already one in the Bun package and I can't see it being used anywhere else 🤷‍♂️

@timfish timfish marked this pull request as ready for review July 9, 2026 02:36
@timfish timfish requested a review from a team as a code owner July 9, 2026 02:36
@timfish timfish requested review from JPeer264, andreiborza, isaacs and mydea and removed request for a team July 9, 2026 02:36
Comment thread packages/server-utils/src/orchestrion/bundler/vite.ts
Comment thread packages/server-utils/src/orchestrion/bundler/webpack.ts Outdated
Comment thread packages/server-utils/src/orchestrion/bundler/esbuild.ts Outdated
Comment thread packages/server-utils/src/orchestrion/bundler/options.ts Outdated
Comment thread packages/server-utils/src/orchestrion/bundler/esbuild.ts Outdated
Comment thread packages/server-utils/src/orchestrion/bundler/rollup.ts

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

There are 5 total unresolved issues (including 3 from previous reviews).

Fix All in Cursor

❌ 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.

Comment thread packages/server-utils/src/orchestrion/bundler/vite.ts
Comment thread packages/server-utils/src/orchestrion/bundler/vite.ts

@isaacs isaacs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Need to keep @apm-js-collab/code-transformer as 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';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhhh. Part of me feel like these should be re-exported from both of the other packages!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would indeed be handy!

return {
instrumentations: [...SENTRY_INSTRUMENTATIONS, ...(options.instrumentations || [])],
injectDiagnostics: () => {
return '(globalThis.__SENTRY_ORCHESTRION__=globalThis.__SENTRY_ORCHESTRION__||{}).bundler=true;';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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's register.ts module

type UnknownPlugin = any;

import codeTransformer from '@apm-js-collab/code-transformer-bundler-plugins/vite';
import MagicString from 'magic-string';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants