fix: React Navigation Instrumentation starts initial transaction before navigator mount

CHANGELOG.md

@@ -6,6 +6,7 @@
 - build(ios): Bump sentry-cocoa to 6.1.4 #1308
 - fix: Handle auto session tracking start on iOS #1308
 - feat: Use beforeNavigate in routing instrumentation to match behavior on JS #1313
+- fix: React Navigation Instrumentation starts initial transaction before navigator mount #1315
 
 ## 2.2.0-beta.0
 

src/js/tracing/reactnavigationv4.ts

@@ -1,6 +1,11 @@
+import { Transaction } from "@sentry/types";
 import { logger } from "@sentry/utils";
 
-import { RoutingInstrumentation } from "./routingInstrumentation";
+import { BeforeNavigate } from "./reactnativetracing";
+import {
+  RoutingInstrumentation,
+  TransactionCreator,
+} from "./routingInstrumentation";
 import { ReactNavigationTransactionContext } from "./types";
 
 export interface NavigationRouteV4 {
@@ -49,6 +54,22 @@ class ReactNavigationV4Instrumentation extends RoutingInstrumentation {
   private _prevRoute?: NavigationRouteV4;
   private _recentRouteKeys: string[] = [];
 
+  private _latestTransaction?: Transaction;
+  private _shouldUpdateLatestTransaction: boolean = false;
+
+  /**
+   * Extends by calling _handleInitialState at the end.
+   */
+  public registerRoutingInstrumentation(
+    listener: TransactionCreator,
+    beforeNavigate: BeforeNavigate
+  ): void {
+    super.registerRoutingInstrumentation(listener, beforeNavigate);
+
+    // Need to handle the initial state as the router patch will only attach transactions on subsequent route changes.
+    this._handleInitialState();
+  }
+
   /**
    * Pass the ref to the app container to register it to the instrumentation
    * @param appContainerRef Ref to an `AppContainer`
@@ -71,19 +92,33 @@ class ReactNavigationV4Instrumentation extends RoutingInstrumentation {
     } else {
       this._patchRouter();
 
-      // Need to handle the initial state as the router patch will only attach transactions on subsequent route changes.
-      this._handleInitialState();
+      if (this._shouldUpdateLatestTransaction) {
+        this._updateLatestTransaction();
+        this._shouldUpdateLatestTransaction = false;
+      }
     }
   }
 
   /**
    * Starts an idle transaction for the initial state which won't get called by the router listener.
    */
   private _handleInitialState(): void {
-    if (this._appContainerRef.current) {
-      const state = this._appContainerRef.current._navigation.state;
+    this._latestTransaction = this.onRouteWillChange(
+      INITIAL_TRANSACTION_CONTEXT_V4
+    );
 
-      this._onStateChange(state);
+    // We set this to true so when registerAppContainer is called, the transaction gets updated with the actual route data
+    this._shouldUpdateLatestTransaction = true;
+  }
+
+  /**
+   * Updates the latest transaction with the current state and calls beforeNavigate.
+   */
+  private _updateLatestTransaction(): void {
+    // We can assume the ref is present as this is called from registerAppContainer
+    if (this._appContainerRef.current && this._latestTransaction) {
+      const state = this._appContainerRef.current._navigation.state;
+      this._onStateChange(state, true);
     }
   }
 
@@ -112,7 +147,10 @@ class ReactNavigationV4Instrumentation extends RoutingInstrumentation {
   /**
    * To be called on navigation state changes and creates the transaction.
    */
-  private _onStateChange(state: NavigationStateV4): void {
+  private _onStateChange(
+    state: NavigationStateV4,
+    updateLatestTransaction: boolean = false
+  ): void {
     const currentRoute = this._getCurrentRouteFromState(state);
 
     // If the route is a different key, this is so we ignore actions that pertain to the same screen.
@@ -136,12 +174,15 @@ class ReactNavigationV4Instrumentation extends RoutingInstrumentation {
       }
 
       if (finalContext.sampled === false) {
-        logger.log(
-          `[ReactNavigationV4Instrumentation] Will not send transaction "${finalContext.name}" due to beforeNavigate.`
-        );
+        this._onBeforeNavigateNotSampled(finalContext.name);
       }
 
-      this.onRouteWillChange(finalContext);
+      if (updateLatestTransaction && this._latestTransaction) {
+        // Update the latest transaction instead of calling onRouteWillChange
+        this._latestTransaction.updateWithContext(finalContext);
+      } else {
+        this._latestTransaction = this.onRouteWillChange(finalContext);
+      }
 
       this._pushRecentRouteKey(currentRoute.key);
       this._prevRoute = currentRoute;
@@ -211,6 +252,23 @@ class ReactNavigationV4Instrumentation extends RoutingInstrumentation {
       );
     }
   };
+
+  /** Helper to log a transaction that was not sampled due to beforeNavigate */
+  private _onBeforeNavigateNotSampled = (transactionName: string): void => {
+    logger.log(
+      `[ReactNavigationV4Instrumentation] Will not send transaction "${transactionName}" due to beforeNavigate.`
+    );
+  };
 }
 
-export { ReactNavigationV4Instrumentation };
+const INITIAL_TRANSACTION_CONTEXT_V4 = {
+  name: "App Launch",
+  op: "navigation",
+  tags: {
+    "routing.instrumentation":
+      ReactNavigationV4Instrumentation.instrumentationName,
+  },
+  data: {},
+};
+
+export { ReactNavigationV4Instrumentation, INITIAL_TRANSACTION_CONTEXT_V4 };

src/js/tracing/reactnavigationv5.ts

@@ -1,7 +1,11 @@
 import { Transaction as TransactionType } from "@sentry/types";
 import { logger } from "@sentry/utils";
 
-import { RoutingInstrumentation } from "./routingInstrumentation";
+import { BeforeNavigate } from "./reactnativetracing";
+import {
+  RoutingInstrumentation,
+  TransactionCreator,
+} from "./routingInstrumentation";
 import { ReactNavigationTransactionContext } from "./types";
 
 export interface NavigationRouteV5 {
@@ -41,9 +45,23 @@ export class ReactNavigationV5Instrumentation extends RoutingInstrumentation {
 
   private _latestRoute?: NavigationRouteV5;
   private _latestTransaction?: TransactionType;
+  private _shouldUpdateLatestTransactionOnRef: boolean = false;
   private _stateChangeTimeout?: number | undefined;
   private _recentRouteKeys: string[] = [];
 
+  /**
+   * Extends by calling _handleInitialState at the end.
+   */
+  public registerRoutingInstrumentation(
+    listener: TransactionCreator,
+    beforeNavigate: BeforeNavigate
+  ): void {
+    super.registerRoutingInstrumentation(listener, beforeNavigate);
+
+    // Need to handle the initial state as the navigation container listeners will only start transactions on subsequent route changes.
+    this._handleInitialState();
+  }
+
   /**
    * Pass the ref to the navigation container to register it to the instrumentation
    * @param navigationContainerRef Ref to a `NavigationContainer`
@@ -61,9 +79,20 @@ export class ReactNavigationV5Instrumentation extends RoutingInstrumentation {
       this._onStateChange.bind(this)
     );
 
+    if (this._shouldUpdateLatestTransactionOnRef) {
+      this._onStateChange();
+      this._shouldUpdateLatestTransactionOnRef = false;
+    }
+  }
+
+  /**
+   *
+   */
+  private _handleInitialState(): void {
     // This will set a transaction for the initial screen.
     this._onDispatch();
-    this._onStateChange();
+
+    this._shouldUpdateLatestTransactionOnRef = true;
   }
 
   /**
@@ -72,7 +101,9 @@ export class ReactNavigationV5Instrumentation extends RoutingInstrumentation {
    * and gets the route information from there, @see _onStateChange
    */
   private _onDispatch(): void {
-    this._latestTransaction = this.onRouteWillChange(BLANK_TRANSACTION_CONTEXT);
+    this._latestTransaction = this.onRouteWillChange(
+      BLANK_TRANSACTION_CONTEXT_V5
+    );
 
     this._stateChangeTimeout = setTimeout(
       this._discardLatestTransaction.bind(this),
@@ -93,7 +124,7 @@ export class ReactNavigationV5Instrumentation extends RoutingInstrumentation {
         this._latestTransaction &&
         (!previousRoute || previousRoute.key !== route.key)
       ) {
-        const originalContext = this._latestTransaction.toContext() as typeof BLANK_TRANSACTION_CONTEXT;
+        const originalContext = this._latestTransaction.toContext() as typeof BLANK_TRANSACTION_CONTEXT_V5;
         const routeHasBeenSeen = this._recentRouteKeys.includes(route.key);
 
         const updatedContext: ReactNavigationTransactionContext = {
@@ -176,7 +207,7 @@ export class ReactNavigationV5Instrumentation extends RoutingInstrumentation {
   }
 }
 
-export const BLANK_TRANSACTION_CONTEXT = {
+export const BLANK_TRANSACTION_CONTEXT_V5 = {
   name: "Route Change",
   op: "navigation",
   tags: {

test/tracing/reactnavigationv4.test.ts

@@ -1,6 +1,9 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
+import { Transaction } from "@sentry/tracing";
+
 import {
   AppContainerInstance,
+  INITIAL_TRANSACTION_CONTEXT_V4,
   NavigationRouteV4,
   NavigationStateV4,
   ReactNavigationV4Instrumentation,
@@ -14,6 +17,14 @@ const initialRoute = {
   },
 };
 
+const getMockTransaction = () => {
+  const transaction = new Transaction(INITIAL_TRANSACTION_CONTEXT_V4);
+
+  transaction.sampled = false;
+
+  return transaction;
+};
+
 class MockAppContainer implements AppContainerInstance {
   _navigation: {
     state: NavigationStateV4;
@@ -72,7 +83,8 @@ describe("ReactNavigationV4Instrumentation", () => {
   test("transaction set on initialize", () => {
     const instrumentation = new ReactNavigationV4Instrumentation();
 
-    instrumentation.onRouteWillChange = jest.fn();
+    const mockTransaction = getMockTransaction();
+    instrumentation.onRouteWillChange = jest.fn(() => mockTransaction);
 
     const tracingListener = jest.fn();
     instrumentation.registerRoutingInstrumentation(
@@ -93,30 +105,32 @@ describe("ReactNavigationV4Instrumentation", () => {
     expect(instrumentation.onRouteWillChange).toHaveBeenCalledTimes(1);
 
     // eslint-disable-next-line @typescript-eslint/unbound-method
-    expect(instrumentation.onRouteWillChange).toHaveBeenLastCalledWith({
-      name: firstRoute.routeName,
-      op: "navigation",
-      tags: {
-        "routing.instrumentation":
-          ReactNavigationV4Instrumentation.instrumentationName,
-        "routing.route.name": firstRoute.routeName,
-      },
-      data: {
-        route: {
-          name: firstRoute.routeName,
-          key: firstRoute.key,
-          params: firstRoute.params,
-          hasBeenSeen: false,
-        },
-        previousRoute: null,
+    expect(instrumentation.onRouteWillChange).toHaveBeenLastCalledWith(
+      INITIAL_TRANSACTION_CONTEXT_V4
+    );
+
+    expect(mockTransaction.name).toBe(firstRoute.routeName);
+    expect(mockTransaction.tags).toStrictEqual({
+      "routing.instrumentation":
+        ReactNavigationV4Instrumentation.instrumentationName,
+      "routing.route.name": firstRoute.routeName,
+    });
+    expect(mockTransaction.data).toStrictEqual({
+      route: {
+        name: firstRoute.routeName,
+        key: firstRoute.key,
+        params: firstRoute.params,
+        hasBeenSeen: false,
       },
+      previousRoute: null,
     });
   });
 
   test("transaction sent on navigation", () => {
     const instrumentation = new ReactNavigationV4Instrumentation();
 
-    instrumentation.onRouteWillChange = jest.fn();
+    const mockTransaction = getMockTransaction();
+    instrumentation.onRouteWillChange = jest.fn(() => mockTransaction);
 
     const tracingListener = jest.fn();
     instrumentation.registerRoutingInstrumentation(
@@ -172,7 +186,8 @@ describe("ReactNavigationV4Instrumentation", () => {
   test("transaction context changed with beforeNavigate", () => {
     const instrumentation = new ReactNavigationV4Instrumentation();
 
-    const tracingListener = jest.fn();
+    const mockTransaction = getMockTransaction();
+    const tracingListener = jest.fn(() => mockTransaction);
     instrumentation.registerRoutingInstrumentation(
       tracingListener as any,
       (context) => {
@@ -231,7 +246,8 @@ describe("ReactNavigationV4Instrumentation", () => {
   test("transaction not attached on a cancelled navigation", () => {
     const instrumentation = new ReactNavigationV4Instrumentation();
 
-    instrumentation.onRouteWillChange = jest.fn();
+    const mockTransaction = getMockTransaction();
+    instrumentation.onRouteWillChange = jest.fn(() => mockTransaction);
 
     const tracingListener = jest.fn();
     instrumentation.registerRoutingInstrumentation(

test/tracing/reactnavigationv5.test.ts

@@ -2,7 +2,7 @@
 import { Transaction } from "@sentry/tracing";
 
 import {
-  BLANK_TRANSACTION_CONTEXT,
+  BLANK_TRANSACTION_CONTEXT_V5,
   NavigationRouteV5,
   ReactNavigationV5Instrumentation,
 } from "../../src/js/tracing/reactnavigationv5";
@@ -24,7 +24,7 @@ class MockNavigationContainer {
 }
 
 const getMockTransaction = () => {
-  const transaction = new Transaction(BLANK_TRANSACTION_CONTEXT);
+  const transaction = new Transaction(BLANK_TRANSACTION_CONTEXT_V5);
 
   transaction.sampled = false;
 
@@ -52,7 +52,7 @@ describe("ReactNavigationV5Instrumentation", () => {
 
     expect(mockTransaction.name).toBe(dummyRoute.name);
     expect(mockTransaction.tags).toStrictEqual({
-      ...BLANK_TRANSACTION_CONTEXT.tags,
+      ...BLANK_TRANSACTION_CONTEXT_V5.tags,
       "routing.route.name": dummyRoute.name,
     });
     expect(mockTransaction.data).toStrictEqual({
@@ -107,7 +107,7 @@ describe("ReactNavigationV5Instrumentation", () => {
 
         expect(mockTransaction.name).toBe(route.name);
         expect(mockTransaction.tags).toStrictEqual({
-          ...BLANK_TRANSACTION_CONTEXT.tags,
+          ...BLANK_TRANSACTION_CONTEXT_V5.tags,
           "routing.route.name": route.name,
         });
         expect(mockTransaction.data).toStrictEqual({
@@ -210,10 +210,10 @@ describe("ReactNavigationV5Instrumentation", () => {
       setTimeout(() => {
         expect(mockTransaction.sampled).toBe(false);
         expect(mockTransaction.name).toStrictEqual(
-          BLANK_TRANSACTION_CONTEXT.name
+          BLANK_TRANSACTION_CONTEXT_V5.name
         );
         expect(mockTransaction.tags).toStrictEqual(
-          BLANK_TRANSACTION_CONTEXT.tags
+          BLANK_TRANSACTION_CONTEXT_V5.tags
         );
         expect(mockTransaction.data).toStrictEqual({});
         resolve();