From 41b58d966e1327103ac817273ad876ef23786d82 Mon Sep 17 00:00:00 2001 From: jennmueng Date: Mon, 1 Feb 2021 01:36:20 +0700 Subject: [PATCH 1/4] feat: Update V4 to start transaction before navigator mount --- src/js/tracing/reactnavigationv4.ts | 82 ++++++++++++++++++++++---- src/js/tracing/reactnavigationv5.ts | 8 ++- test/tracing/reactnavigationv4.test.ts | 56 +++++++++++------- test/tracing/reactnavigationv5.test.ts | 12 ++-- 4 files changed, 117 insertions(+), 41 deletions(-) diff --git a/src/js/tracing/reactnavigationv4.ts b/src/js/tracing/reactnavigationv4.ts index 44bc802d6c..764130f1e4 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( + BLANK_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 BLANK_TRANSACTION_CONTEXT_V4 = { + name: "Route Change", + op: "navigation", + tags: { + "routing.instrumentation": + ReactNavigationV4Instrumentation.instrumentationName, + }, + data: {}, +}; + +export { ReactNavigationV4Instrumentation, BLANK_TRANSACTION_CONTEXT_V4 }; diff --git a/src/js/tracing/reactnavigationv5.ts b/src/js/tracing/reactnavigationv5.ts index b76ba038e0..4d8a68c724 100644 --- a/src/js/tracing/reactnavigationv5.ts +++ b/src/js/tracing/reactnavigationv5.ts @@ -72,7 +72,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 +95,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 +178,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..a6f5522707 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, + BLANK_TRANSACTION_CONTEXT_V4, NavigationRouteV4, NavigationStateV4, ReactNavigationV4Instrumentation, @@ -14,6 +17,14 @@ const initialRoute = { }, }; +const getMockTransaction = () => { + const transaction = new Transaction(BLANK_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( + BLANK_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(); From 8435210c49678ac5dead059c233db261f00e2457 Mon Sep 17 00:00:00 2001 From: jennmueng Date: Mon, 1 Feb 2021 01:55:53 +0700 Subject: [PATCH 2/4] feat: Update V5 to start transaction before navigator mount --- src/js/tracing/reactnavigationv5.ts | 33 +++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/src/js/tracing/reactnavigationv5.ts b/src/js/tracing/reactnavigationv5.ts index 4d8a68c724..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; } /** From 85fc8992a7f7f6289fd6cde970e28aff9337c4eb Mon Sep 17 00:00:00 2001 From: jennmueng Date: Mon, 1 Feb 2021 20:07:56 +0700 Subject: [PATCH 3/4] ref: Rename initial transaction on V4 to 'App Launch' --- src/js/tracing/reactnavigationv4.ts | 8 ++++---- test/tracing/reactnavigationv4.test.ts | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/js/tracing/reactnavigationv4.ts b/src/js/tracing/reactnavigationv4.ts index 764130f1e4..efada563ee 100644 --- a/src/js/tracing/reactnavigationv4.ts +++ b/src/js/tracing/reactnavigationv4.ts @@ -104,7 +104,7 @@ class ReactNavigationV4Instrumentation extends RoutingInstrumentation { */ private _handleInitialState(): void { this._latestTransaction = this.onRouteWillChange( - BLANK_TRANSACTION_CONTEXT_V4 + INITIAL_TRANSACTION_CONTEXT_V4 ); // We set this to true so when registerAppContainer is called, the transaction gets updated with the actual route data @@ -261,8 +261,8 @@ class ReactNavigationV4Instrumentation extends RoutingInstrumentation { }; } -const BLANK_TRANSACTION_CONTEXT_V4 = { - name: "Route Change", +const INITIAL_TRANSACTION_CONTEXT_V4 = { + name: "App Launch", op: "navigation", tags: { "routing.instrumentation": @@ -271,4 +271,4 @@ const BLANK_TRANSACTION_CONTEXT_V4 = { data: {}, }; -export { ReactNavigationV4Instrumentation, BLANK_TRANSACTION_CONTEXT_V4 }; +export { ReactNavigationV4Instrumentation, INITIAL_TRANSACTION_CONTEXT_V4 }; diff --git a/test/tracing/reactnavigationv4.test.ts b/test/tracing/reactnavigationv4.test.ts index a6f5522707..d437569dbe 100644 --- a/test/tracing/reactnavigationv4.test.ts +++ b/test/tracing/reactnavigationv4.test.ts @@ -3,7 +3,7 @@ import { Transaction } from "@sentry/tracing"; import { AppContainerInstance, - BLANK_TRANSACTION_CONTEXT_V4, + INITIAL_TRANSACTION_CONTEXT_V4, NavigationRouteV4, NavigationStateV4, ReactNavigationV4Instrumentation, @@ -18,7 +18,7 @@ const initialRoute = { }; const getMockTransaction = () => { - const transaction = new Transaction(BLANK_TRANSACTION_CONTEXT_V4); + const transaction = new Transaction(INITIAL_TRANSACTION_CONTEXT_V4); transaction.sampled = false; @@ -106,7 +106,7 @@ describe("ReactNavigationV4Instrumentation", () => { // eslint-disable-next-line @typescript-eslint/unbound-method expect(instrumentation.onRouteWillChange).toHaveBeenLastCalledWith( - BLANK_TRANSACTION_CONTEXT_V4 + INITIAL_TRANSACTION_CONTEXT_V4 ); expect(mockTransaction.name).toBe(firstRoute.routeName); From 907b743580b5789385ae6e3b6a08153f1bce7c92 Mon Sep 17 00:00:00 2001 From: jennmueng Date: Mon, 1 Feb 2021 20:30:29 +0700 Subject: [PATCH 4/4] meta: Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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