diff --git a/CHANGELOG.md b/CHANGELOG.md index 45b532758e..1c96054688 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/js/tracing/reactnavigationv4.ts b/src/js/tracing/reactnavigationv4.ts index 44bc802d6c..efada563ee 100644 --- a/src/js/tracing/reactnavigationv4.ts +++ b/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,8 +92,10 @@ 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; + } } } @@ -80,10 +103,22 @@ class ReactNavigationV4Instrumentation extends RoutingInstrumentation { * 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 }; diff --git a/src/js/tracing/reactnavigationv5.ts b/src/js/tracing/reactnavigationv5.ts index b76ba038e0..f48a2a2314 100644 --- a/src/js/tracing/reactnavigationv5.ts +++ b/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: { diff --git a/test/tracing/reactnavigationv4.test.ts b/test/tracing/reactnavigationv4.test.ts index 3b67a57768..d437569dbe 100644 --- a/test/tracing/reactnavigationv4.test.ts +++ b/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( diff --git a/test/tracing/reactnavigationv5.test.ts b/test/tracing/reactnavigationv5.test.ts index bc7246c5fe..a477d70b0b 100644 --- a/test/tracing/reactnavigationv5.test.ts +++ b/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();