ref(browser)!: Remove beforeSpanEnd callback in favor of beforeIdleSpanEnd hook (#22818)

#22554 introduced a
new `beforeIdleSpanEnd` client hook. We can use it instead of the
existing idle span's `beforeSpanEnd` callback.

Breaking change because `startIdleSpan` is publicly exported, but I
confirmed that we do not document this anywhere

Author: msonnb

packages/browser/src/tracing/browserTracingIntegration.ts

@@ -424,31 +424,6 @@ export const browserTracingIntegration = ((options: Partial<BrowserTracingOption
       childSpanTimeout,
       // should wait for finish signal if it's a pageload transaction
       disableAutoFinish: isPageloadSpan,
-      beforeSpanEnd: span => {
-        addPerformanceEntries(span, {
-          ignoreResourceSpans,
-          spanStreamingEnabled: hasSpanStreamingEnabled(client),
-        });
-        setActiveIdleSpan(client, undefined);
-
-        // A trace should stay consistent over the entire timespan of one route - even after the pageload/navigation ended.
-        // Only when another navigation happens, we want to create a new trace.
-        // This way, e.g. errors that occur after the pageload span ended are still associated to the pageload trace.
-        const scope = getCurrentScope();
-        const oldPropagationContext = scope.getPropagationContext();
-
-        scope.setPropagationContext({
-          ...oldPropagationContext,
-          traceId: idleSpan.spanContext().traceId,
-          sampled: spanIsSampled(idleSpan),
-          dsc: getDynamicSamplingContextFromSpan(span),
-        });
-
-        if (isPageloadSpan) {
-          // clean up the stored pageload span on the intergration.
-          _pageloadSpan = undefined;
-        }
-      },
       trimIdleSpanEndTimestamp: !enableReportPageLoaded,
     });
 
@@ -519,6 +494,39 @@ export const browserTracingIntegration = ((options: Partial<BrowserTracingOption
         }
       }
 
+      client.on('beforeIdleSpanEnd', span => {
+        // Interaction idle spans also flow through this hook, but the route bookkeeping below only
+        // applies to the pageload/navigation span. We identify it by reference rather than by op:
+        // only the route span is registered as the active idle span.
+        if (getActiveIdleSpan(client) !== span) {
+          return;
+        }
+
+        addPerformanceEntries(span, {
+          ignoreResourceSpans,
+          spanStreamingEnabled: hasSpanStreamingEnabled(client),
+        });
+        setActiveIdleSpan(client, undefined);
+
+        // A trace should stay consistent over the entire timespan of one route - even after the pageload/navigation ended.
+        // Only when another navigation happens, we want to create a new trace.
+        // This way, e.g. errors that occur after the pageload span ended are still associated to the pageload trace.
+        const scope = getCurrentScope();
+        const oldPropagationContext = scope.getPropagationContext();
+
+        scope.setPropagationContext({
+          ...oldPropagationContext,
+          traceId: span.spanContext().traceId,
+          sampled: spanIsSampled(span),
+          dsc: getDynamicSamplingContextFromSpan(span),
+        });
+
+        if (_pageloadSpan === span) {
+          // clean up the stored pageload span on the integration.
+          _pageloadSpan = undefined;
+        }
+      });
+
       client.on('startNavigationSpan', (startSpanOptions, navigationOptions) => {
         if (getClient() !== client) {
           return;

packages/core/src/tracing/idleSpan.ts

@@ -75,9 +75,6 @@ interface IdleSpanOptions {
    */
   disableAutoFinish?: boolean;
 
-  /** Allows to configure a hook that is called when the idle span is ended, before it is processed. */
-  beforeSpanEnd?: (span: Span) => void;
-
   /**
    * If set to `true`, the idle span will be trimmed to the latest span end timestamp of its children.
    *
@@ -114,7 +111,6 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
     idleTimeout = TRACING_DEFAULTS.idleTimeout,
     finalTimeout = TRACING_DEFAULTS.finalTimeout,
     childSpanTimeout = TRACING_DEFAULTS.childSpanTimeout,
-    beforeSpanEnd,
     trimIdleSpanEndTimestamp = true,
   } = options;
 
@@ -142,10 +138,6 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
     apply(target, thisArg, args: Parameters<Span['end']>) {
       client.emit('beforeIdleSpanEnd', span);
 
-      if (beforeSpanEnd) {
-        beforeSpanEnd(span);
-      }
-
       // If the span is non-recording, nothing more to do here...
       // This is the case if tracing is enabled but this specific span was not sampled
       if (spanIsNonRecordingSpan(thisArg)) {

packages/core/test/lib/tracing/idleSpan.test.ts

@@ -214,21 +214,23 @@ describe('startIdleSpan', () => {
     });
   });
 
-  it('calls beforeSpanEnd callback before finishing', () => {
-    const beforeSpanEnd = vi.fn();
-    const idleSpan = startIdleSpan({ name: 'foo' }, { beforeSpanEnd });
+  it('emits beforeIdleSpanEnd hook before finishing', () => {
+    const beforeIdleSpanEnd = vi.fn();
+    getClient()!.on('beforeIdleSpanEnd', beforeIdleSpanEnd);
+
+    const idleSpan = startIdleSpan({ name: 'foo' });
     expect(idleSpan).toBeDefined();
 
-    expect(beforeSpanEnd).not.toHaveBeenCalled();
+    expect(beforeIdleSpanEnd).not.toHaveBeenCalled();
 
     startSpan({ name: 'inner' }, () => {});
 
     vi.runOnlyPendingTimers();
-    expect(beforeSpanEnd).toHaveBeenCalledTimes(1);
-    expect(beforeSpanEnd).toHaveBeenLastCalledWith(idleSpan);
+    expect(beforeIdleSpanEnd).toHaveBeenCalledTimes(1);
+    expect(beforeIdleSpanEnd).toHaveBeenLastCalledWith(idleSpan);
   });
 
-  it('allows to mutate idle span in beforeSpanEnd before it is sent', () => {
+  it('allows to mutate idle span in beforeIdleSpanEnd before it is sent', () => {
     const transactions: Event[] = [];
     const beforeSendTransaction = vi.fn(event => {
       transactions.push(event);
@@ -246,16 +248,18 @@ describe('startIdleSpan', () => {
     // We want to accommodate a bit of drift there, so we ensure this starts earlier...
     const baseTimeInSeconds = Math.floor(Date.now() / 1000) - 9999;
 
-    const beforeSpanEnd = vi.fn((span: Span) => {
+    const beforeIdleSpanEnd = vi.fn((span: Span) => {
       span.setAttribute('foo', 'bar');
       // Try adding a child here - we do this in browser tracing...
-      const inner = startInactiveSpan({ name: 'from beforeSpanEnd', startTime: baseTimeInSeconds });
+      const inner = startInactiveSpan({ name: 'from beforeIdleSpanEnd', startTime: baseTimeInSeconds });
       inner.end(baseTimeInSeconds + 1);
     });
-    const idleSpan = startIdleSpan({ name: 'idle span', startTime: baseTimeInSeconds }, { beforeSpanEnd });
+    client.on('beforeIdleSpanEnd', beforeIdleSpanEnd);
+
+    const idleSpan = startIdleSpan({ name: 'idle span', startTime: baseTimeInSeconds });
     expect(idleSpan).toBeDefined();
 
-    expect(beforeSpanEnd).not.toHaveBeenCalled();
+    expect(beforeIdleSpanEnd).not.toHaveBeenCalled();
 
     vi.advanceTimersByTime(TRACING_DEFAULTS.idleTimeout + 1);
     vi.runOnlyPendingTimers();
@@ -270,7 +274,7 @@ describe('startIdleSpan', () => {
     const transaction = transactions[0]!;
 
     expect(transaction.start_timestamp).toBe(baseTimeInSeconds);
-    // It considers the end time of the span we added in beforeSpanEnd
+    // It considers the end time of the span we added in beforeIdleSpanEnd
     expect(transaction.timestamp).toBe(baseTimeInSeconds + 1);
 
     expect(transaction.contexts?.trace?.data).toEqual(
@@ -279,7 +283,7 @@ describe('startIdleSpan', () => {
       }),
     );
     expect(transaction.spans).toHaveLength(1);
-    expect(transaction.spans).toEqual([expect.objectContaining({ description: 'from beforeSpanEnd' })]);
+    expect(transaction.spans).toEqual([expect.objectContaining({ description: 'from beforeIdleSpanEnd' })]);
   });
 
   it('runs beforeIdleSpanEnd before trimming the idle span', () => {