feat(server-runtime-injection): Split runtime orchestrion injection into a dedicated package - #23685
feat(server-runtime-injection): Split runtime orchestrion injection into a dedicated package#23685mydea wants to merge 7 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
24c382c to
2a2a5e3
Compare
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
There's definitely more opportunities for clean-up and a few things that should probably be addressed to avoid excess disk usage, but splitting these parts into a separate package is exactly the right approach, imo 👍
| @@ -1,20 +1,15 @@ | |||
| import { builtinModules } from 'node:module'; | |||
There was a problem hiding this comment.
The rollup changes here, and splitting into two packages, results in getting two copies of @apm-js-collab/code-transformer. We can avoid that by re-exporting the transformer from server-utils, so that it's only written to disk once, and save 6.69MB of install space on disk. (Not bundle size, just disk install size and file count.)
Here's what that could look like:
https://gist.github.com/isaacs/6162f2c166207d564f962ba5c581d1f4
I'm not necessarily married to that approach, definitely adds some redirection complexity, but it is kind of a lot of extra install weight for cf users otherwise.
| * build-time snippet's `@sentry/server-utils` import is handled separately by the code-transform. | ||
| */ | ||
| export const ORCHESTRION_RUNTIME_EXTERNAL_PACKAGES = ['@sentry/server-utils']; | ||
| export const ORCHESTRION_RUNTIME_EXTERNAL_PACKAGES = ['@sentry/server-runtime-injection']; |
There was a problem hiding this comment.
I understand why this is changing, but it does have one interesting ramification.
The orchestrion config now gets loaded twice; once from the server chunk, and once from node_modules via the external register.js in the new package. It's pure data, and guaranteed to be identical, so there's no correctness problem today, but the minute anything stateful gets in there, it's going to be an issue. I'm not sure the best fix there, but that does feel a bit brittle.
| * so `bundler` is set (to an empty `Set`) from boot, which gates the SDK's channel-integration setup | ||
| * at `init()`. | ||
| */ | ||
| export function sentryOrchestrionPlugin(): UnknownPlugin { |
There was a problem hiding this comment.
It's a little bit weird that this method doesn't take a PluginOptions option, since it's siblings all do. Maybe it should take a PluginOptions argument, and at least respect buildTimeInstrumentation: false?
2a2a5e3 to
082ec3f
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>
…-plugins dependency `@sentry/bun/plugin` now only re-exports from `@sentry/server-utils/orchestrion/bun` (which vendors the transformer), so the direct import is gone. Remove the leftover runtime dependency so installs don't pull an unused package. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
082ec3f to
24e43de
Compare
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 043aca6. Configure here.
| // Bun's builder to its `setup`. | ||
| const transformer = codeTransformer( | ||
| orchestrionTransformOptions(options, { injectDiagnostics: false }), | ||
| ) as unknown as { |
There was a problem hiding this comment.
Bun skips custom instrumented packages
Medium Severity
The Bun plugin now accepts extra instrumentations and forwards them to the transformer, but withoutInstrumentedExternals and the blanket-external warning still use the default INSTRUMENTED_MODULE_NAMES. Custom packages left in external never pass through onLoad, and Bun has no runtime fallback, so those libraries load uninstrumented.
Reviewed by Cursor Bugbot for commit 043aca6. Configure here.
2 participants
Stacked on #23675 (the detect/warn work) — base this on
fix/orchestrion-detect-bundling; rebase ontodeveloponce #23675 merges.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 (Next.js forwarders, the vite plugin couldn't cleanly separate them). This moves the runtime injection into a new package,@sentry/server-runtime-injection, so "keep this external" is a clean package boundary.What moves / stays
@sentry/server-runtime-injection:register/hook/import-hook(clean subpaths/register,/hook,/import-hook) + the vendored transformer chain. It depends on@sentry/server-utilsand importsSENTRY_INSTRUMENTATIONSfrom./orchestrion/config— config stays put (pure data; resolves fromnode_modules, one-way dep, no cycle), so@sentry/server-utilsremains fully bundleable.@sentry/server-utils: drops the runtime dir/exports and the runtime-only vendored dep (@apm-js-collab/tracing-hooks); keeps the barrel + config + bundler plugins.@sentry/server-utilsas./orchestrion/bun, alongside the vite/rollup/webpack/esbuild plugins;@sentry/bun/pluginnow just re-exports it assentryBunPlugin. This letsconfig/index.tsdrop its bundler re-export (it only existed so@sentry/buncould compose the upstream plugin itself), keeping./orchestrion/configtransformer-free without needing a separatebundler-transformssubpath.References updated
sdk/index.ts+ test mock), Deno (import.mjs), the shared--importtemplate +makeOrchestrionLoaderguard (one source for every framework's generated hook).ORCHESTRION_RUNTIME_EXTERNAL_PACKAGES→ the new package; the require-ability check is re-anchored at@sentry/nextjs(so it works under pnpm, where a server-utils-anchored resolver couldn't see the new package); forwarder generation iterates the new package's exports; tests updated..size-limit.jshook path; the@sentry/server-runtime-injectiondep added to node/nextjs/deno/aws-serverless/google-cloud-serverless/astro/tanstackstart-react..craft.yml(base tier, after server-utils), CODEOWNERS, the aws-serverless e2epackagesToLink.Verification
New package builds (config external, transformer vendored, own THIRD-PARTY-LICENSES) and its tests pass;
server-utils(353),node(362),bun(50), and the Next.js config tests (122) pass; lint clean across all changed packages. Smoke-testedregisterin ESM + CJS (healthy → hooks install, no warning), the relocated bun plugin (banner + force-bundle instrumented deps), and confirmed the detection probe still fires on a tree-shaken bundle (warns once, exit 0). size-limit unchanged —@sentry/node119.79 kB, hook entry 79.78 kB, all pass.Follow-ups
@sentry/server-utilsdep fromgoogle-cloud-serverless(kept for safety).🤖 Generated with Claude Code