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! |
isaacs
force-pushed
the
experiment/orchestrionjs-auto-instrumentation
branch
from
0157d09 to
25bfdaa
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 25bfdaa. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
isaacs
force-pushed
the
experiment/orchestrionjs-auto-instrumentation
branch
from
25bfdaa to
7a8b730
Compare
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
7a8b730 to
7b69e3b
Compare
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.