feat(node)!: Drop custom startNewTrace implementation - #22715
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 aff9af4. Configure here.
| if (acs.startNewTrace) { | ||
| return acs.startNewTrace(callback); | ||
| } | ||
|
|
There was a problem hiding this comment.
OTEL spans ignore startNewTrace
High Severity
Removing the OpenTelemetry startNewTrace override drops remote span injection into OTEL context. In Node, Sentry.startInactiveSpan / startSpan root creation still goes through SentryTracer, which calls startNewTrace again per root instead of reusing the callback’s propagation context, so spans inside one startNewTrace can get different traceIds and won’t match the scope set by the API.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit aff9af4. Configure here.
There was a problem hiding this comment.
Verified that this bb finding is valid. (Will provide more details, writing up, don't merge for now please.)
| }); | ||
| }); | ||
|
|
||
| describe('startNewTrace', () => { |
There was a problem hiding this comment.
Removed OTEL startNewTrace tests
Medium Severity
This feat change deletes the entire OpenTelemetry startNewTrace test suite without adding integration, E2E, or equivalent unit coverage elsewhere. Per PR review guidelines, feature work should include tests that assert the new behavior, especially for Node/OTEL trace propagation.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit aff9af4. Configure here.
size-limit report 📦
|
|
I think this PR is not feasible, though it does showcase a shortcoming in our tests that would've made that more clear. In packages/opentelemetry/src/tracer.ts, there's this, when there's no parent span and no remote parent: return startNewTrace(() =>
_INTERNAL_startInactiveSpan({
...sentryOptions,
parentSpan: hasExplicitContext ? null : undefined,
}),
);Every parentless root span created through OTEL calls core The OTEL override avoided this by injecting an The propagator in // If we have a remote span, the spanId should be considered as the parentSpanId, not spanId itself
// Instead, we use a virtual (generated) spanId for propagation
if (span?.spanContext().isRemote) {So, on Node, spans created inside In digging into this, I created a test that can be applied on I tried to see if there was a way to detect that we're in a remote span and not re-enter the |
2 participants
All tests pass without this, it seems to work fine with the base implementation.