fix(stalls): Missing measurements when using new .end() span API (#3737)

CHANGELOG.md

@@ -12,6 +12,7 @@
 - Do not enable NativeFramesTracking when native is not available ([#3705](https://github.com/getsentry/sentry-react-native/pull/3705))
 - Do not initialize the SDK during `expo-router` static routes generation ([#3730](https://github.com/getsentry/sentry-react-native/pull/3730))
 - Cancel spans in background doesn't crash in environments without AppState ([#3727](https://github.com/getsentry/sentry-react-native/pull/3727))
+- Fix missing Stall measurements when using new `.end()` span API ([#3737](https://github.com/getsentry/sentry-react-native/pull/3737))
 
 ### Dependencies
 

src/js/tracing/addTracingExtensions.ts

@@ -72,11 +72,22 @@ const _patchStartTransaction = (originalStartTransaction: StartTransactionFuncti
 
       transaction.finish = (endTimestamp: number | undefined) => {
         if (reactNativeTracing) {
-          reactNativeTracing.onTransactionFinish(transaction);
+          reactNativeTracing.onTransactionFinish(transaction, endTimestamp);
         }
 
         return originalFinish.apply(transaction, [endTimestamp]);
       };
+
+      // eslint-disable-next-line @typescript-eslint/unbound-method
+      const originalEnd = transaction.end;
+
+      transaction.end = (endTimestamp: number | undefined) => {
+        if (reactNativeTracing) {
+          reactNativeTracing.onTransactionFinish(transaction, endTimestamp);
+        }
+
+        return originalEnd.apply(transaction, [endTimestamp]);
+      };
     }
 
     return transaction;

src/js/tracing/stalltracking.ts

@@ -122,6 +122,19 @@ export class StallTrackingInstrumentation {
             this._markSpanFinish(transaction, span.endTimestamp);
           }
         };
+
+        // eslint-disable-next-line @typescript-eslint/unbound-method
+        const originalSpanEnd = span.end;
+
+        span.end = (endTimestamp?: number) => {
+          // We let the span determine its own end timestamp as well in case anything gets changed upstream
+          originalSpanEnd.apply(span, [endTimestamp]);
+
+          // The span should set a timestamp, so this would be defined.
+          if (span.endTimestamp) {
+            this._markSpanFinish(transaction, span.endTimestamp);
+          }
+        };
       };
     }
   }

test/mocks/client.ts

@@ -13,6 +13,8 @@ import { resolvedSyncPromise } from '@sentry/utils';
 
 export function getDefaultTestClientOptions(options: Partial<TestClientOptions> = {}): TestClientOptions {
   return {
+    dsn: 'https://[email protected]/4505526893805568',
+    enabled: true,
     integrations: [],
     sendClientReports: true,
     transport: () =>
@@ -39,6 +41,7 @@ export class TestClient extends BaseClient<TestClientOptions> {
   public static sendEventCalled?: (event: Event) => void;
 
   public event?: Event;
+  public eventQueue: Array<Event> = [];
   public hint?: EventHint;
   public session?: Session;
 
@@ -74,6 +77,7 @@ export class TestClient extends BaseClient<TestClientOptions> {
 
   public sendEvent(event: Event, hint?: EventHint): void {
     this.event = event;
+    this.eventQueue.push(event);
     this.hint = hint;
 
     // In real life, this will get deleted as part of envelope creation.

test/tracing/reactnavigation.stalltracking.test.ts

@@ -0,0 +1,69 @@
+import {
+  addGlobalEventProcessor,
+  getCurrentHub,
+  getCurrentScope,
+  getGlobalScope,
+  getIsolationScope,
+  setCurrentClient,
+  startSpanManual,
+} from '@sentry/core';
+
+import { ReactNativeTracing, ReactNavigationInstrumentation } from '../../src/js';
+import { _addTracingExtensions } from '../../src/js/tracing/addTracingExtensions';
+import { RN_GLOBAL_OBJ } from '../../src/js/utils/worldwide';
+import { getDefaultTestClientOptions, TestClient } from '../mocks/client';
+import { createMockNavigationAndAttachTo } from './reactnavigationutils';
+import { expectStallMeasurements } from './stalltrackingutils';
+
+jest.useFakeTimers({ advanceTimers: true });
+
+describe('StallTracking with ReactNavigation', () => {
+  let client: TestClient;
+  let mockNavigation: ReturnType<typeof createMockNavigationAndAttachTo>;
+
+  beforeEach(() => {
+    RN_GLOBAL_OBJ.__sentry_rn_v5_registered = false;
+    _addTracingExtensions();
+
+    getCurrentScope().clear();
+    getIsolationScope().clear();
+    getGlobalScope().clear();
+
+    const rnavigation = new ReactNavigationInstrumentation();
+    mockNavigation = createMockNavigationAndAttachTo(rnavigation);
+
+    const rnTracing = new ReactNativeTracing({
+      routingInstrumentation: rnavigation,
+      enableStallTracking: true,
+      enableNativeFramesTracking: false,
+      enableAppStartTracking: false,
+    });
+
+    const options = getDefaultTestClientOptions({
+      tracesSampleRate: 1.0,
+      integrations: [rnTracing],
+    });
+    client = new TestClient(options);
+    setCurrentClient(client);
+    client.init();
+
+    // We have to call this manually as setupOnce is executed once per runtime (global var check)
+    rnTracing.setupOnce(addGlobalEventProcessor, getCurrentHub);
+  });
+
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it('Stall tracking supports idleTransaction with unfinished spans', async () => {
+    jest.runOnlyPendingTimers(); // Flush app start transaction
+    mockNavigation.navigateToNewScreen();
+    startSpanManual({ name: 'This child span will never finish' }, () => {});
+
+    jest.runOnlyPendingTimers(); // Flush new screen transaction
+
+    await client.flush();
+
+    expectStallMeasurements(client.event?.measurements);
+  });
+});

test/tracing/reactnavigation.ttid.test.tsx

@@ -14,14 +14,14 @@ import TestRenderer from 'react-test-renderer';
 import * as Sentry from '../../src/js';
 import { ReactNavigationInstrumentation } from '../../src/js';
 import { TimeToFullDisplay, TimeToInitialDisplay } from '../../src/js/tracing';
-import type { NavigationRoute } from '../../src/js/tracing/reactnavigation';
 import { isHermesEnabled, notWeb } from '../../src/js/utils/environment';
 import { createSentryEventEmitter } from '../../src/js/utils/sentryeventemitter';
 import { RN_GLOBAL_OBJ } from '../../src/js/utils/worldwide';
 import { MOCK_DSN } from '../mockDsn';
 import { secondInFutureTimestampMs } from '../testutils';
 import type { MockedSentryEventEmitter } from '../utils/mockedSentryeventemitter';
 import { emitNativeFullDisplayEvent, emitNativeInitialDisplayEvent } from './mockedtimetodisplaynative';
+import { createMockNavigationAndAttachTo } from './reactnavigationutils';
 
 describe('React Navigation - TTID', () => {
   let mockedEventEmitter: MockedSentryEventEmitter;
@@ -514,78 +514,12 @@ describe('React Navigation - TTID', () => {
     return sut;
   }
 
-  function createMockNavigationAndAttachTo(sut: ReactNavigationInstrumentation) {
-    const mockedNavigationContained = mockNavigationContainer();
-    const mockedNavigation = {
-      navigateToNewScreen: () => {
-        mockedNavigationContained.listeners['__unsafe_action__']({
-          // this object is not used by the instrumentation
-        });
-        mockedNavigationContained.currentRoute = {
-          key: 'new_screen',
-          name: 'New Screen',
-        };
-        mockedNavigationContained.listeners['state']({
-          // this object is not used by the instrumentation
-        });
-      },
-      navigateToInitialScreen: () => {
-        mockedNavigationContained.listeners['__unsafe_action__']({
-          // this object is not used by the instrumentation
-        });
-        mockedNavigationContained.currentRoute = {
-          key: 'initial_screen',
-          name: 'Initial Screen',
-        };
-        mockedNavigationContained.listeners['state']({
-          // this object is not used by the instrumentation
-        });
-      },
-      finishAppStartNavigation: () => {
-        mockedNavigationContained.currentRoute = {
-          key: 'initial_screen',
-          name: 'Initial Screen',
-        };
-        mockedNavigationContained.listeners['state']({
-          // this object is not used by the instrumentation
-        });
-      },
-    };
-    sut.registerNavigationContainer(mockRef(mockedNavigationContained));
-
-    return mockedNavigation;
-  }
-
-  function mockNavigationContainer(): MockNavigationContainer {
-    return new MockNavigationContainer();
-  }
-
-  function mockRef<T>(wat: T): { current: T } {
-    return {
-      current: wat,
-    };
-  }
-
   function getLastTransaction(mockedTransportSend: jest.Mock): TransactionEvent {
     // Until https://github.com/getsentry/sentry-javascript/blob/a7097d9ba2a74b2cb323da0ef22988a383782ffb/packages/types/src/event.ts#L93
     return JSON.parse(JSON.stringify(mockedTransportSend.mock.lastCall[0][1][0][1]));
   }
 });
 
-class MockNavigationContainer {
-  currentRoute: NavigationRoute = {
-    key: 'initial_screen',
-    name: 'Initial Screen',
-  };
-  listeners: Record<string, (e: any) => void> = {};
-  addListener: any = jest.fn((eventType: string, listener: (e: any) => void): void => {
-    this.listeners[eventType] = listener;
-  });
-  getCurrentRoute(): NavigationRoute | undefined {
-    return this.currentRoute;
-  }
-}
-
 function initSentry(sut: ReactNavigationInstrumentation): {
   transportSendMock: jest.Mock<ReturnType<Transport['send']>, Parameters<Transport['send']>>;
 } {

test/tracing/reactnavigationutils.ts

@@ -0,0 +1,67 @@
+import type { NavigationRoute, ReactNavigationInstrumentation } from '../../src/js/tracing/reactnavigation';
+
+export function createMockNavigationAndAttachTo(sut: ReactNavigationInstrumentation) {
+  const mockedNavigationContained = mockNavigationContainer();
+  const mockedNavigation = {
+    navigateToNewScreen: () => {
+      mockedNavigationContained.listeners['__unsafe_action__']({
+        // this object is not used by the instrumentation
+      });
+      mockedNavigationContained.currentRoute = {
+        key: 'new_screen',
+        name: 'New Screen',
+      };
+      mockedNavigationContained.listeners['state']({
+        // this object is not used by the instrumentation
+      });
+    },
+    navigateToInitialScreen: () => {
+      mockedNavigationContained.listeners['__unsafe_action__']({
+        // this object is not used by the instrumentation
+      });
+      mockedNavigationContained.currentRoute = {
+        key: 'initial_screen',
+        name: 'Initial Screen',
+      };
+      mockedNavigationContained.listeners['state']({
+        // this object is not used by the instrumentation
+      });
+    },
+    finishAppStartNavigation: () => {
+      mockedNavigationContained.currentRoute = {
+        key: 'initial_screen',
+        name: 'Initial Screen',
+      };
+      mockedNavigationContained.listeners['state']({
+        // this object is not used by the instrumentation
+      });
+    },
+  };
+  sut.registerNavigationContainer(mockRef(mockedNavigationContained));
+
+  return mockedNavigation;
+}
+
+function mockRef<T>(wat: T): { current: T } {
+  return {
+    current: wat,
+  };
+}
+
+function mockNavigationContainer(): MockNavigationContainer {
+  return new MockNavigationContainer();
+}
+
+export class MockNavigationContainer {
+  currentRoute: NavigationRoute = {
+    key: 'initial_screen',
+    name: 'Initial Screen',
+  };
+  listeners: Record<string, (e: any) => void> = {};
+  addListener: any = jest.fn((eventType: string, listener: (e: any) => void): void => {
+    this.listeners[eventType] = listener;
+  });
+  getCurrentRoute(): NavigationRoute | undefined {
+    return this.currentRoute;
+  }
+}