fix(core): Attribute errors to the span they escaped

dev-packages/browser-integration-tests/suites/tracing/errorSpanAttribution/init.js

@@ -0,0 +1,10 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  traceLifecycle: 'static',
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [Sentry.browserTracingIntegration()],
+  tracesSampleRate: 1,
+});

dev-packages/browser-integration-tests/suites/tracing/errorSpanAttribution/test.ts

@@ -0,0 +1,103 @@
+import type { Page } from '@playwright/test';
+import { expect } from '@playwright/test';
+import type { Event } from '@sentry/core';
+import { sentryTest } from '../../../utils/fixtures';
+import {
+  envelopeRequestParser,
+  runScriptInSandbox,
+  shouldSkipTracingTest,
+  waitForErrorRequest,
+  waitForTransactionRequest,
+} from '../../../utils/helpers';
+
+// The browser SDK parents every span to the root span, so `outer` and `inner` are siblings under the
+// pageload span rather than a chain. Attribution still has to pick the span the error escaped.
+const waitForPageloadWithSpans = (page: Page) =>
+  waitForTransactionRequest(
+    page,
+    event => event.contexts?.trace?.op === 'pageload' && !!event.spans?.some(span => span.description === 'inner'),
+  );
+
+sentryTest(
+  'attributes an uncaught error to the span it escaped, not the pageload span active when it surfaces',
+  async ({ getLocalTestUrl, page, browserName }) => {
+    if (browserName === 'webkit') {
+      // Errors thrown from `runScriptInSandbox` are Script Errors on Webkit and skipped by Sentry
+      sentryTest.skip();
+    }
+
+    if (shouldSkipTracingTest()) {
+      sentryTest.skip();
+    }
+
+    const url = await getLocalTestUrl({ testDir: __dirname });
+
+    const errorRequestPromise = waitForErrorRequest(page);
+    const transactionRequestPromise = waitForPageloadWithSpans(page);
+
+    await page.goto(url);
+
+    await runScriptInSandbox(page, {
+      content: `
+      setTimeout(() => {
+        Sentry.startSpan({ name: 'outer' }, () => {
+          Sentry.startSpan({ name: 'inner' }, () => {
+            throw new Error('Escaped Error');
+          });
+        });
+      });
+      `,
+    });
+
+    const errorEvent = envelopeRequestParser<Event>(await errorRequestPromise);
+    const transactionEvent = envelopeRequestParser<Event>(await transactionRequestPromise);
+
+    const innerSpan = transactionEvent.spans?.find(span => span.description === 'inner');
+
+    expect(errorEvent.exception?.values?.[0]?.value).toBe('Escaped Error');
+    expect(errorEvent.contexts?.trace?.trace_id).toBe(transactionEvent.contexts?.trace?.trace_id);
+    expect(errorEvent.contexts?.trace?.span_id).toBe(innerSpan?.span_id);
+    expect(errorEvent.contexts?.trace?.span_id).not.toBe(transactionEvent.contexts?.trace?.span_id);
+  },
+);
+
+sentryTest(
+  'attributes a caught error to the span it escaped, not the span it was caught in',
+  async ({ getLocalTestUrl, page }) => {
+    if (shouldSkipTracingTest()) {
+      sentryTest.skip();
+    }
+
+    const url = await getLocalTestUrl({ testDir: __dirname });
+
+    const errorRequestPromise = waitForErrorRequest(page);
+    const transactionRequestPromise = waitForPageloadWithSpans(page);
+
+    await page.goto(url);
+
+    await runScriptInSandbox(page, {
+      content: `
+      Sentry.startSpan({ name: 'outer' }, () => {
+        try {
+          Sentry.startSpan({ name: 'inner' }, () => {
+            throw new Error('Caught Error');
+          });
+        } catch (error) {
+          Sentry.captureException(error);
+        }
+      });
+      `,
+    });
+
+    const errorEvent = envelopeRequestParser<Event>(await errorRequestPromise);
+    const transactionEvent = envelopeRequestParser<Event>(await transactionRequestPromise);
+
+    const innerSpan = transactionEvent.spans?.find(span => span.description === 'inner');
+    const outerSpan = transactionEvent.spans?.find(span => span.description === 'outer');
+
+    expect(errorEvent.exception?.values?.[0]?.value).toBe('Caught Error');
+    expect(errorEvent.contexts?.trace?.trace_id).toBe(transactionEvent.contexts?.trace?.trace_id);
+    expect(errorEvent.contexts?.trace?.span_id).toBe(innerSpan?.span_id);
+    expect(errorEvent.contexts?.trace?.span_id).not.toBe(outerSpan?.span_id);
+  },
+);

dev-packages/e2e-tests/test-applications/node-hapi/tests/errors.test.ts

@@ -1,23 +1,26 @@
 import { expect, test } from '@playwright/test';
-import { waitForError, waitForStreamedSpan } from '@sentry-internal/test-utils';
+import {
+  collectStreamedSpansUntilSegment,
+  getSpanOp,
+  waitForError,
+  waitForStreamedSpan,
+} from '@sentry-internal/test-utils';
 
 test('Sends thrown error to Sentry', async ({ baseURL }) => {
   const errorEventPromise = waitForError('node-hapi', errorEvent => {
     return errorEvent?.exception?.values?.[0]?.value === 'This is an error';
   });
 
-  const segmentEventPromise = waitForStreamedSpan(
-    'node-hapi',
-    segment => segment.is_segment && segment.name === 'GET /test-failure',
-  );
+  const spansPromise = collectStreamedSpansUntilSegment('node-hapi', 'GET /test-failure');
 
   await fetch(`${baseURL}/test-failure`);
 
   const errorEvent = await errorEventPromise;
-  const segmentEvent = await segmentEventPromise;
+  const spans = await spansPromise;
+  const segmentSpan = spans.find(span => span.is_segment);
 
-  expect(segmentEvent.name).toBe('GET /test-failure');
-  expect(segmentEvent).toMatchObject({
+  expect(segmentSpan?.name).toBe('GET /test-failure');
+  expect(segmentSpan).toMatchObject({
     trace_id: expect.stringMatching(/[a-f0-9]{32}/),
     span_id: expect.stringMatching(/[a-f0-9]{16}/),
   });
@@ -42,10 +45,15 @@ test('Sends thrown error to Sentry', async ({ baseURL }) => {
   expect(errorEvent.contexts?.trace).toEqual({
     trace_id: expect.stringMatching(/[a-f0-9]{32}/),
     span_id: expect.stringMatching(/[a-f0-9]{16}/),
+    parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
   });
 
-  expect(errorEvent.contexts?.trace?.trace_id).toBe(segmentEvent?.trace_id);
-  expect(errorEvent.contexts?.trace?.span_id).toBe(segmentEvent?.span_id);
+  // The error is attributed to the route handler span that threw, which is a child of the request
+  // span the segment is built from.
+  const routeHandlerSpan = spans.find(span => getSpanOp(span) === 'router');
+  expect(errorEvent.contexts?.trace?.trace_id).toBe(segmentSpan?.trace_id);
+  expect(errorEvent.contexts?.trace?.span_id).toBe(routeHandlerSpan?.span_id);
+  expect(errorEvent.contexts?.trace?.parent_span_id).toBe(segmentSpan?.span_id);
 });
 
 test('sends error with parameterized transaction name', async ({ baseURL }) => {

packages/core/src/tracing/trace.ts

@@ -14,6 +14,7 @@ import type { StartSpanOptions } from '../types/startSpanOptions';
 import { baggageHeaderToDynamicSamplingContext } from '../utils/baggage';
 import { debug } from '../utils/debug-logger';
 import { handleCallbackErrors } from '../utils/handleCallbackErrors';
+import { recordEscapedErrorSpan } from '../utils/errorSpanAttribution';
 import { hasSpansEnabled } from '../utils/hasSpansEnabled';
 import { shouldIgnoreSpan } from '../utils/should-ignore-span';
 import { hasSpanStreamingEnabled } from './spans/hasSpanStreamingEnabled';
@@ -670,7 +671,9 @@ function runCallback<T>(span: Span, makeSpanActive: boolean, callback: () => T,
   return wrapper(() =>
     handleCallbackErrors(
       () => callback(),
-      () => {
+      error => {
+        recordEscapedErrorSpan(error, span);
+
         // Only update the span status if it hasn't been changed yet, and the span is not yet finished
         const { status } = spanToStaticSpanJSON(span);
         if (span.isRecording() && status === 'ok') {

packages/core/src/utils/errorSpanAttribution.ts

@@ -0,0 +1,78 @@
+import { getTraceContextFromScope } from '../currentScopes';
+import type { Scope } from '../scope';
+import type { TraceContext } from '../types/context';
+import type { Event, EventHint } from '../types/event';
+import type { Span } from '../types/span';
+import { isPrimitive } from './is';
+import { spanIsSampled, spanToTraceContext } from './spanUtils';
+
+/**
+ * The trace context of the span an error escaped, keyed by the error itself.
+ *
+ * We store the trace context rather than the span because that is the shape we apply to the event
+ * later, and it snapshots the span as it failed instead of reading it back once it has ended.
+ */
+const escapedSpanTraceContexts = new WeakMap<object, TraceContext>();
+
+/**
+ * A `WeakMap` can only be keyed by an object, so an error thrown as a primitive (`throw 'boom'`)
+ * has nothing we can hang the span on and is left unattributed.
+ */
+function toWeakMapKey(error: unknown): object | undefined {
+  return isPrimitive(error) ? undefined : error;
+}
+
+/**
+ * Remember which span an error escaped, so a later `captureException` can attribute the error to
+ * the span that actually failed instead of whichever span happens to be active at capture time.
+ *
+ * The first span to see the error wins: as an error unwinds through nested spans, the innermost
+ * one is the one that failed. Unsampled spans are skipped because they are never sent, so their
+ * span id would point at a span that does not exist. Sampling rather than `isRecording()` is what
+ * matters here: a span ended before the error escaped it, which is the norm for `startSpanManual`,
+ * has stopped recording but is still sent.
+ */
+export function recordEscapedErrorSpan(error: unknown, span: Span): void {
+  const key = toWeakMapKey(error);
+
+  if (!key || !spanIsSampled(span) || escapedSpanTraceContexts.has(key)) {
+    return;
+  }
+
+  escapedSpanTraceContexts.set(key, spanToTraceContext(span));
+}
+
+/**
+ * Attribute an error event to the span the error escaped, if we recorded one.
+ *
+ * This only applies within the error's own trace. The stored span id is meaningless in another
+ * trace, and the event's dynamic sampling context (which the envelope header is built from) is
+ * derived from the root span of the trace the event is already on. Rewriting the trace id here
+ * would leave the envelope header and body naming different traces.
+ */
+export function applyEscapedErrorSpanToEvent(event: Event, hint: EventHint, scope: Scope | undefined): void {
+  const key = toWeakMapKey(hint.originalException);
+  const traceContext = key && escapedSpanTraceContexts.get(key);
+
+  if (!traceContext) {
+    return;
+  }
+
+  // An error captured with no active span has no trace context yet: the scope's is merged in
+  // further downstream. Resolve the trace the event will end up on the same way that merge does,
+  // so the check below still knows which trace we are on.
+  const eventTraceContext = event.contexts?.trace;
+  const eventTraceId = eventTraceContext?.trace_id ?? (scope && getTraceContextFromScope(scope).trace_id);
+
+  if (eventTraceId !== traceContext.trace_id) {
+    return;
+  }
+
+  event.contexts = {
+    ...event.contexts,
+    trace: {
+      ...eventTraceContext,
+      ...traceContext,
+    },
+  };
+}

packages/core/src/utils/prepareEvent.ts

@@ -8,6 +8,7 @@ import type { ClientOptions } from '../types/options';
 import type { StackParser } from '../types/stacktrace';
 import { getFilenameToDebugIdMap } from './debug-ids';
 import { getDataCategoryByType } from './envelope';
+import { applyEscapedErrorSpanToEvent } from './errorSpanAttribution';
 import { addExceptionMechanismToCapturedException, uuid4 } from './misc';
 import { normalize } from './normalize';
 import { applyScopeDataToEvent, applySpanToEvent, getCombinedScopeData } from './scopeData';
@@ -95,6 +96,11 @@ export function prepareEvent(
     applySpanToEvent(prepared, span);
   }
 
+  // After the active span, so an error that escaped a span is attributed to that span rather than
+  // to whichever one happened to be active at capture time. Done here rather than once the event is
+  // assembled so that event processors and the `postprocessEvent` hook see the corrected span id.
+  applyEscapedErrorSpanToEvent(prepared, hint, finalScope);
+
   const eventProcessors = [
     ...clientEventProcessors,
     // Run scope event processors _after_ all other processors