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?
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.
99fac5c to
07c56d2
Compare
![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 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 07c56d2. Configure here.
|
|
||
| if (!key || !span.isRecording() || escapedSpanTraceContexts.has(key)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Ended spans skip error attribution
Low Severity
recordEscapedErrorSpan bails out when span.isRecording() is false, and SentrySpan.isRecording() is also false after end(). A sampled span that already finished is still sent, so an error thrown in that callback after end() is not attributed to it and falls through to a parent or the capture-time span. The skip is meant for spans that are never sent, which does not apply here.
Reviewed by Cursor Bugbot for commit 07c56d2. Configure here.
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!)
3 participants
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