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}
| form-data "^4.0.5" | ||
| proxy-from-env "^2.1.0" | ||
|
|
||
| axios@^0.26.1: |
There was a problem hiding this comment.
Medium severity vulnerability introduced by a package you're using:
Line 11806 lists a dependency (axios) with a known Medium severity vulnerability. Fixing requires upgrading or replacing the dependency.
ℹ️ Why this matters
Affected versions of axios are vulnerable to Improper Neutralization of CRLF Sequences in HTTP Headers ('HTTP Request/Response Splitting') / Inconsistent Interpretation of HTTP Requests ('HTTP Request/Response Smuggling') / Server-Side Request Forgery (SSRF). Axios can be used as a gadget for header injection: if another dependency enables prototype pollution, polluted properties can be merged into Axios request headers and written without CRLF sanitization, allowing request smuggling/SSRF that can reach internal services such as AWS IMDSv2 and potentially lead to credential theft or broader compromise.
To resolve this comment:
Upgrade this dependency to at least version 0.31.0 at yarn.lock.
💬 Ignore this finding
To ignore this, reply with:
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
You can view more details on this finding in the Semgrep AppSec Platform here.
| form-data "^4.0.5" | ||
| proxy-from-env "^2.1.0" | ||
|
|
||
| axios@^0.26.1: |
There was a problem hiding this comment.
Medium severity vulnerability may affect your project—review required:
Line 11806 lists a dependency (axios) with a known Medium severity vulnerability.
ℹ️ Why this matters
Affected versions of axios are vulnerable to Server-Side Request Forgery (SSRF) / Unintended Proxy or Intermediary ('Confused Deputy'). Axios does not normalize hostnames before applying NO_PROXY, so requests to loopback or internal hosts such as localhost. or [::1] can be sent through a configured proxy instead of bypassing it. If an attacker can influence request URLs, they may force local/internal Axios traffic through an attacker-controlled proxy, undermining SSRF protections and exposing sensitive responses.
To resolve this comment:
Check if you have NO_PROXY configured in your environment.
- If you're affected, upgrade this dependency to at least version 0.31.0 at yarn.lock.
- If you're not affected, comment
/fp we don't use this [condition]
💬 Ignore this finding
To ignore this, reply with:
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
You can view more details on this finding in the Semgrep AppSec Platform here.
![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 |
| }); | ||
| debug('module.register() called for @apm-js-collab/tracing-hooks/hook.mjs'); | ||
|
|
||
| // ALSO patch `Module.prototype._compile` for the CJS side: when an ESM file |
There was a problem hiding this comment.
This is not the case when using synchronous module.registerHooks. In that case, the same hooks cover all CJS and ESM, even internal to CJS packages.
Once apm-js-collab/tracing-hooks#27 lands, we can feature detect like:
import { initialize, resolve, load } from '@apm-js-collab/tracing-hooks/hook-sync.mjs';
import Module from 'node:module';
if (runtime) {
// ...
} else {
// detection to decide module loader hooks to use
// registerHooks was present but not stable until 24.13 and 25.1
const version = (process.versions.node ?? '0.0.0')
.split('.')
.map(n => parseInt(n, 10));
const stableSyncHooks = version[0] > 25 ||
version[0] === 25 && version[1] >= 1 ||
version[0] === 24 && version[1] >= 13;
if (typeof Module.registerHooks === 'function' && stableSyncHooks) {
initialize({ instrumentations });
Module.registerHooks({ resolve, load });
} else if (typeof Module.register === 'function') {
// what you have here already, Module.register() and the cjs patch
} else {
throw new Error('No available API to apply module load hooks');
}
}There was a problem hiding this comment.
updated and applied this change, seems to work well! 🎉
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
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 5d957bf. Configure here.
| code: SPAN_STATUS_ERROR, | ||
| message: ctx.error instanceof Error ? ctx.error.message : 'unknown_error', | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Missing captureException call in error handling paths
Low Severity
The startInactiveSpan call's error paths (both the channel error handler and the streamable Query's 'error' event listener) set span status to SPAN_STATUS_ERROR but never call captureException. Per the review rules, when using any startSpan API (including startInactiveSpan), error cases need to be checked and it may make sense to call captureException. This may be intentional for DB query errors (to avoid noise), but worth flagging per the rules.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 5d957bf. Configure here.
| // resolves it via Node's module resolution against the installed package. | ||
|
|
||
| import { createRequire } from 'node:module'; | ||
| import { initialize, resolve, load } from '@apm-js-collab/tracing-hooks/hook-sync.mjs'; |
There was a problem hiding this comment.
Static import of sync hooks prevents fallback path
Medium Severity
The top-level static import of @apm-js-collab/tracing-hooks/hook-sync.mjs executes unconditionally on every Node version, but initialize, resolve, and load are only used inside the Module.registerHooks branch (Node ≥24.13/25.1). If hook-sync.mjs internally depends on APIs absent on older Node versions, the import throws before the runtime fallback to Module.register is ever reached. A dynamic import() inside the branch would make the fallback path reliable.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5d957bf. Configure here.
2 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.