fix(core): Attribute errors to the span they escaped - #23666
Conversation
size-limit report 📦
|
| */ | ||
| const escapedSpanTraceContexts = new WeakMap<object, TraceContext>(); | ||
|
|
||
| function toKey(error: unknown): object | undefined { |
There was a problem hiding this comment.
l: maybe add a comment here why we do this
There was a problem hiding this comment.
without a bit more context, from the outside it seems as if we're converting the error to something else, which I was confused about initially 😅
There was a problem hiding this comment.
I renamed the fn to be clearer and added a comment
| /** | ||
| * The trace context of the span an error escaped, keyed by the error itself. | ||
| * | ||
| * We store the plain trace context rather than the span, so that an error object cannot keep a |
There was a problem hiding this comment.
l/m: is this necessary? Since this is a weakmap, should this be garbage collected anyhow?
There was a problem hiding this comment.
actually, I think the implementation is fine but I would change the comment - it is not necessary to keep this as trace context for gc reasons, but we apply the trace context directly later so this is fine.
There was a problem hiding this comment.
I re-worded it a bit, initially I was only storing the span id but noticed for the whole thing to be correct-ish I needed the rest of the trace context.
| trace: { | ||
| ...eventTraceContext, | ||
| span_id: traceContext.span_id, | ||
| parent_span_id: traceContext.parent_span_id, |
There was a problem hiding this comment.
m: IMHO we should invert this, if a span is already set on this, likely we should not overwrite it I think? 🤔 but this also makes the logic a bit trickier, because we cannot just do:
trace: {
span_id: traceContext.span_id,
parent_span_id: traceContext.parent_span_id
...eventTraceContext
}
because that could lead to a case where eventTraceContext has a span_id but no parent_span_id and then they would be incorrectly in sync.
I guess we generally do have a traceContext here already set, right, even if we do not actually have the span? 🤔
Would it work if we move the invocation of applyEscapedErrorSpanToEvent to prepareEvent.ts like this:
if (span) {
applySpanToEvent(prepared, span);
} else {
applyEscapedErrorSpanToEvent(prepared, hint);
}
or something along these lines? 🤔
There was a problem hiding this comment.
So there are three cases here:
- The span id is wrong: this is the main fix here because we cannot trust the span id already set because we know for a fact it is wrong, so we have to overwrite. The concern here would be, could the already set span id be more accurate than the span id that caught and re-thrown the error?
- The span id matches the span the error escaped from: We write the same values so it is idempotent here.
- No trace/span_id: This is the tricky part, if we change placement we won't have a trace_id to compare against and so we could have a mismatching dsc.
I clarified these in a comment just now if it makes sense, WDYT?
99fac5c to
07c56d2
Compare
There was a problem hiding this comment.
Q: When we call recordEscapedErrorSpan, could we instead of the weakmap approach, throw a _span non-enumberable property onto the error object? Which we'd later on pick up and adjust the trace context accordingly...
Just wondering if there's smaller alternative to the weakmap.
(fwiw, I'm probably biased because my idea a few years to keep scope data was basically this: Throw the scope onto the error ein every startSpan/withScope callback and merge scopes. Never tried this though so I'm probably missing reasons, so don't feel forced to rewrite!)
I thought we generally preferred weakmaps in recent PRs I observed. Doesn't seem like a big difference to me other than it avoids any funny business with runtimes that serialize stuff like cloudlfare (not saying they do in this case, but they were sneakily serializing tracing channel payloads). Happy to switch over since it's not a strong opinion, what do you think? |
|
👋 @mydea — Please review this PR when you get a chance! |
I think this makes sense to me, then we can possibly just pick this up if it exists in the code where we set the trace context based on the active span? 🤔 |
|
👋 @mydea — Please review this PR when you get a chance! |
Adds failing coverage for #16206: an error is attributed to whatever span is active at captureException time rather than the span it was thrown in. Three cases fail on develop (sync nested span, concurrent group, deepest escaped span). The fourth asserts the cross-trace bail-out, which passes today and must keep passing.
An error was attributed to whichever span happened to be active when captureException ran, not to the span that actually failed. Record the span's trace context in a WeakMap keyed on the error as it unwinds, and prefer that when building the error event. The first (deepest) span wins, non-recording spans are skipped, and the attribution only applies within the error's own trace so the envelope header and body can never name different traces.
The error thrown in a hapi route handler escapes the router span, so it is now attributed to that span rather than to the request span. Assert the new relationship (error span is a child of the transaction's span, and is the router span) instead of the old identity.
Rename `toKey` to `toWeakMapKey` and explain that thrown primitives cannot be keyed. The previous WeakMap comment claimed a GC reason that does not hold.
… merge The trace context of an event captured with no active span only exists as of that merge, and without its trace id we cannot check the recorded span belongs to the same trace.
07c56d2 to
b1d8b5d
Compare
@mydea which approach do u mean? I moved the lookup to |
Attribution ran once the event was fully assembled, so scope and client event processors, along with the `postprocessEvent` hook, still saw the span that happened to be active at capture time. Move the lookup next to `applySpanToEvent` in `prepareEvent`. The same trace guard stays, but the trace id now comes from the scope when the event has no trace context of its own, which is what pushed the call downstream in the first place. The stored context is applied whole rather than spliced in: at this point the event may have no trace context at all, and the merge in `_prepareEvent` lets an existing one win outright, so a partial context would never get its trace id filled in.
`recordEscapedErrorSpan` skipped spans that were not recording, which bundles two conditions: the span is sampled, and it has not ended. Only sampling is relevant, since an unsampled span is never sent and its id would point at nothing. `handleCallbackErrors` runs its error handler after the callback's own `finally` block, and `startSpanManual` leaves ending the span to the caller. So the usual manual span idiom, ending the span in a `finally` and letting the error propagate, ended the span before the error unwound past it and was skipped entirely, despite being sampled and sent. Check `spanIsSampled` instead.
| () => callback(), | ||
| () => { | ||
| error => { | ||
| recordEscapedErrorSpan(error, span); |
There was a problem hiding this comment.
Bug: The error span attribution fix is incomplete. Next.js route handlers call handleCallbackErrors directly, bypassing the startSpan logic, so errors are not correctly attributed to the handler's span.
Severity: MEDIUM
Suggested Fix
To ensure consistent error attribution, wrap the route handler execution in wrapRouteHandlerWithSentry.ts with a startSpan call. This will ensure that runCallback is executed, which in turn calls recordEscapedErrorSpan upon an error, correctly associating the error with the active span before it's captured. This pattern should be applied to any other framework integrations that currently call handleCallbackErrors directly.
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/core/src/tracing/trace.ts#L675
Potential issue: The fix for error span attribution is incomplete. In certain framework
integrations, such as the Next.js route handler wrapper
(`wrapRouteHandlerWithSentry.ts`), errors are handled by calling `handleCallbackErrors`
directly, without being wrapped in a `startSpan`. The new attribution logic, which uses
`recordEscapedErrorSpan`, is only called from within the `runCallback` function used by
`startSpan`. As a result, errors thrown from these specific handlers will not be
correctly associated with their originating span, reverting to the old behavior where
attribution depends on the active span at the time of capture.
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.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4099fde. Configure here.
| const eventTraceContext = event.contexts?.trace; | ||
| const eventTraceId = eventTraceContext?.trace_id ?? (scope && getTraceContextFromScope(scope).trace_id); | ||
|
|
||
| if (eventTraceId !== traceContext.trace_id) { |
There was a problem hiding this comment.
Trace check uses the wrong scope
Medium Severity
The same-trace fallback reads finalScope, but the later merge and DSC still come from currentScope. Passing a Scope as captureContext can skip attribution, or write the escaped span's trace_id onto an event whose envelope DSC names a different trace.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4099fde. Configure here.
Errors are now attributed to the span they were thrown in, not whichever span happened to be active when
captureExceptionran.startSpanrecords the escaping error's span in aWeakMapand we prefer that when building the event.It only applies inside the error's own trace, this was flagged as an unlikely edge case by the clanker so I decided to add a sanity check for it regardless of how unlikely it is.
We seemed to have tests asserting the old behavior, so those needed to change to match the new one.
closes #16206