WIP: POC to use orchestrion-js for instrumentation#20900
Conversation
![semgrep-code-getsentry[bot]](https://avatars.githubusercontent.com/in/1153895?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
|
Note: dependency warning stuff should be addressed when this is merged/released: apm-js-collab/code-transformer-bundler-plugins#2 |
mydea
force-pushed
the
experiment/orchestrionjs-auto-instrumentation
branch
from
b1b6ed6 to
9e8b070
Compare
mydea
force-pushed
the
experiment/orchestrionjs-auto-instrumentation
branch
from
9e8b070 to
26ccdf4
Compare
mydea
force-pushed
the
experiment/orchestrionjs-auto-instrumentation
branch
from
26ccdf4 to
c8be420
Compare
mydea
force-pushed
the
experiment/orchestrionjs-auto-instrumentation
branch
from
a38481b to
c095626
Compare
mydea
force-pushed
the
experiment/orchestrionjs-auto-instrumentation
branch
from
5d957bf to
12ac21c
Compare
|
I also added an e2e test app using the vite plugin, however this is failing, will need to wait on v0.2.0 of the plugins as this updates to the latest version of the code transformer, which we need here. |
mydea
force-pushed
the
experiment/orchestrionjs-auto-instrumentation
branch
from
7d6f1f7 to
3de78da
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
This is looking very good!
My only concern/suggestion is that it should really not be node-specific. Deno and Bun are both supported by orchestrion now, so it'd be good to move this logic into a shared location so they can all benefit from it as we add instrumentations.
I'm not sure if it's best to (a) land this first, and then refactor this and the other @sentry/core/server stuff into @sentry-internal/server-utils, or (b) if we should bite that bullet first and then move this implementation on top of it. If we go with (a), then the move would be to land this now, and do server-utils as a second step. If (b), then it'd be good to get that done, and port this on top of it.
jup, eventually this should go into a new server-utils package or similar I guess - we can move this up any time! |
Also, fix the `import ... from 'node:module'` in the import-hook.mjs, which was broken.
The test was loading the import hook via `import`, which would get converted to a `require()` when testing CommonJS. This, obviously, would break, because the import loader is ESM only. Update the test to register the import loader in the traditional way, with `--import`, so that we can verify it covers both CJS and ESM. Update the Orchestrion Plan so that the agents don't get confused when the require hook is missing.
Also, disable the `Module.register()` path on Deno and Bun, because the function is present but no-opped on those platforms.
Older versions of TS do not find this from package.json submodule exports.
The AI agents reviewing the code are concerned that we're not warning in production, but that would be a bad idea in this case, so put a comment to make the intent clearer.
isaacs
force-pushed
the
experiment/orchestrionjs-auto-instrumentation
branch
from
7b69e3b to
d7addf6
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Polish the the orchestrion.js-based auto-instrumentation so the Node SDK
presents a minimal API surface that does not expose the internal
"orchestrion" term on the public API surface.
A single opt-in option now does the entire Orchestrion setup.
```ts
Sentry.init({
dsn: '__DSN__',
experimentalDiagnosticsChannelInjection: true,
})
```
Assuming that this is run in a user's local `--import` module (or,
failing that, before the instrumented modules are loaded), then the
appropriate hooks will be synchronously added, and Orchestrion
diagnostics_channel-based integrations will be used instead of the
legacy OTel integrations.
The previous three-step setup (`_experimentalUseOrchestrion` flag,
`--import @sentry/node/orchestrion`, plus a separate
`_experimentalSetupOrchestrion()` method call) is removed.
The channel-injection hooks are registered synchronously (mirroring
`esmLoader.ts`: `Module.register(...)` + `ModulePatch` on Node <24.13,
`Module.registerHooks` on newer / Deno 2.8+), so they are in place
before the app's own `import`s resolve. A dynamic `import()` would have
raced module loading.
The `@sentry/node/import` loader hook script injects the channels
**unconditionally**, as the presence of the channels is harmless when
they are not being used. They are only *subscribed* to if the opt-in
flag is set.
The version detection is moved to a single source of truth in
`server-utils/orchestrion/runtime/register.ts`, so that it can be
initialized synchronously as part of `Sentry.init()`, or in the
`import-hook.mjs` loader.
All references to `orchestrion` are removed from the public API surface.
The term remains in `server-utils`, of course, as this is an internal
package designed to house shared implementation details across
server-side JS platforms.
The `@sentry/node/orchestrion/vite` subpath export is removed, as that
really isn't used by anything, and was just a pass-through for
`@sentry/server-utils/orchestrion/vite` anyway. The Bun and Edge
computing SDKs will use this directly to instrument using Orchestrion.
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 e27b2e9. Configure here.
| '`node --import ./instrument.mjs app.js`), or inject the channels with ' + | ||
| '`node --import @sentry/node/import app.js`.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Late init misreports injection success
Medium Severity
After registerDiagnosticsChannelInjection() succeeds, __SENTRY_ORCHESTRION__.runtime is set even when instrumented modules such as mysql were already loaded, so hooks never transform them. The DEBUG-only warning only runs when that marker is missing, so the late-init case described in the function comment never triggers and MySQL spans can fail silently.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e27b2e9. Configure here.
| // possible (before the app imports its instrumented modules) when opted in. | ||
| if (options.experimentalDiagnosticsChannelInjection) { | ||
| installDiagnosticsChannelInjection(); | ||
| } |
There was a problem hiding this comment.
Channel mysql ignores tracing disabled
Medium Severity
With experimentalDiagnosticsChannelInjection: true, the SDK always installs channel injection hooks and adds the channel-based Mysql integration, even when hasSpansEnabled is false. The default OTel Mysql integration is only registered when span recording is enabled, so this path can instrument queries when tracing was intentionally turned off.
Reviewed by Cursor Bugbot for commit e27b2e9. Configure here.
| '`node --import ./instrument.mjs app.js`), or inject the channels with ' + | ||
| '`node --import @sentry/node/import app.js`.', | ||
| ); |
There was a problem hiding this comment.
Bug: The warning in installDiagnosticsChannelInjection() for late Sentry.init() calls is broken. It incorrectly checks for registration success instead of timing, so it never fires when modules are imported before initialization.
Severity: MEDIUM
Suggested Fix
The warning logic should be changed to detect if modules were already loaded before the diagnostics channel hooks were registered, rather than just checking if the registration function succeeded. This might involve setting a flag earlier in the application lifecycle (e.g., via --import) and checking its state within Sentry.init() to determine if initialization is happening too late.
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/node/src/sdk/diagnosticsChannelInjection.ts#L52-L54
Potential issue: The warning logic in `installDiagnosticsChannelInjection()` is flawed.
It is intended to alert developers if `Sentry.init()` is called after instrumented
modules have already been imported, which would cause instrumentation to fail. However,
the check `!marker?.runtime && !marker?.bundler` will never trigger in this scenario.
This is because `registerDiagnosticsChannelInjection()` sets `marker.runtime = true`
upon successful hook registration, regardless of whether it was too late. As a result,
developers who misconfigure their application by initializing Sentry after importing a
module like `mysql` will not receive the intended debug warning, leading to silent
instrumentation failure.
Also affects:
packages/server-utils/src/orchestrion/runtime/register.ts:98~98
4 participants
This is a WIP POC trying out usage of orchestrion-js for node SDK instrumentation.
Honestly it seems pretty straightforward... Usage for this POC is:
And then
This will disable the otel instrumentation that is already converted to orchestrion (in this PR, only Mysql) and add the respective orchestrion-based integrations instead. The exact API here is WIP and really just geared towards experimentation, so could change, and it's easy to see how this would be easier in v11 with this being the default.
Some general benefits of this approach:
--importscript only registers the mappings for orchestrion, all actual code registering stuff etc. happens inSentry.init(). This makes a bunch of things easier...--import. This also works when deploying to e.g. cloudflare etc. as long as one of the bundler plugins is used.