fix(core): Attribute errors to the span they escaped - #23666
Open
logaretm wants to merge 3 commits into
Open
Conversation
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.
Contributor
size-limit report 📦
|
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.
logaretm
marked this pull request as ready for review
August 27, 2026 02:47
mydea
reviewed
Aug 27, 2026
| */ | ||
| const escapedSpanTraceContexts = new WeakMap<object, TraceContext>(); | ||
|
|
||
| function toKey(error: unknown): object | undefined { |
Member
There was a problem hiding this comment.
l: maybe add a comment here why we do this
Member
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 😅
mydea
reviewed
Aug 27, 2026
| /** | ||
| * 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 |
Member
There was a problem hiding this comment.
l/m: is this necessary? Since this is a weakmap, should this be garbage collected anyhow?
Member
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.
mydea
reviewed
Aug 27, 2026
| trace: { | ||
| ...eventTraceContext, | ||
| span_id: traceContext.span_id, | ||
| parent_span_id: traceContext.parent_span_id, |
Member
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? 🤔
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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