diff --git a/CHANGELOG.md b/CHANGELOG.md index 9148147679..ab8f63a6d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ ### Fixes - Preserves interaction span context during app restart to allow proper replay capture ([#5386](https://github.com/getsentry/sentry-react-native/pull/5386)) +- Discard empty Route Change transactions ([#5387](https://github.com/getsentry/sentry-react-native/issues/5387)) ### Dependencies diff --git a/packages/core/src/js/tracing/onSpanEndUtils.ts b/packages/core/src/js/tracing/onSpanEndUtils.ts index d35d9e7379..07c68ae3fc 100644 --- a/packages/core/src/js/tracing/onSpanEndUtils.ts +++ b/packages/core/src/js/tracing/onSpanEndUtils.ts @@ -43,7 +43,28 @@ export const adjustTransactionDuration = (client: Client, span: Span, maxDuratio }); }; -export const ignoreEmptyBackNavigation = (client: Client | undefined, span: Span | undefined): void => { +/** + * Helper function to filter out auto-instrumentation child spans. + */ +function getMeaningfulChildSpans(span: Span): Span[] { + const children = getSpanDescendants(span); + return children.filter( + child => + child.spanContext().spanId !== span.spanContext().spanId && + spanToJSON(child).op !== 'ui.load.initial_display' && + spanToJSON(child).op !== 'navigation.processing', + ); +} + +/** + * Generic helper to discard empty navigation spans based on a condition. + */ +function discardEmptyNavigationSpan( + client: Client | undefined, + span: Span | undefined, + shouldDiscardFn: (span: Span) => boolean, + onDiscardFn: (span: Span) => void, +): void { if (!client) { debug.warn('Could not hook on spanEnd event because client is not defined.'); return; @@ -55,7 +76,7 @@ export const ignoreEmptyBackNavigation = (client: Client | undefined, span: Span } if (!isRootSpan(span) || !isSentrySpan(span)) { - debug.warn('Not sampling empty back spans only works for Sentry Transactions (Root Spans).'); + debug.warn('Not sampling empty navigation spans only works for Sentry Transactions (Root Spans).'); return; } @@ -64,27 +85,66 @@ export const ignoreEmptyBackNavigation = (client: Client | undefined, span: Span return; } - if (!spanToJSON(span).data?.['route.has_been_seen']) { + if (!shouldDiscardFn(span)) { return; } - const children = getSpanDescendants(span); - const filtered = children.filter( - child => - child.spanContext().spanId !== span.spanContext().spanId && - spanToJSON(child).op !== 'ui.load.initial_display' && - spanToJSON(child).op !== 'navigation.processing', - ); - - if (filtered.length <= 0) { - // filter children must include at least one span not created by the navigation automatic instrumentation - debug.log( - 'Not sampling transaction as route has been seen before. Pass ignoreEmptyBackNavigationTransactions = false to disable this feature.', - ); - // Route has been seen before and has no child spans. + const meaningfulChildren = getMeaningfulChildSpans(span); + if (meaningfulChildren.length <= 0) { + onDiscardFn(span); span['_sampled'] = false; } }); +} + +export const ignoreEmptyBackNavigation = (client: Client | undefined, span: Span | undefined): void => { + discardEmptyNavigationSpan( + client, + span, + // Only discard if route has been seen before + span => spanToJSON(span).data?.['route.has_been_seen'] === true, + // Log message when discarding + () => { + debug.log( + 'Not sampling transaction as route has been seen before. Pass ignoreEmptyBackNavigationTransactions = false to disable this feature.', + ); + }, + ); +}; + +/** + * Discards empty "Route Change" transactions that never received route information. + * This happens when navigation library emits a route change event but getCurrentRoute() returns undefined. + * Such transactions don't contain any useful information and should not be sent to Sentry. + * + * This function must be called with a reference tracker function that can check if the span + * was cleared from the integration's tracking (indicating it went through the state listener). + */ +export const ignoreEmptyRouteChangeTransactions = ( + client: Client | undefined, + span: Span | undefined, + defaultNavigationSpanName: string, + isSpanStillTracked: () => boolean, +): void => { + discardEmptyNavigationSpan( + client, + span, + // Only discard if: + // 1. Still has default name + // 2. No route information was set + // 3. Still being tracked (state listener never called) + span => { + const spanJSON = spanToJSON(span); + return ( + spanJSON.description === defaultNavigationSpanName && !spanJSON.data?.['route.name'] && isSpanStillTracked() + ); + }, + // Log and record dropped event + _span => { + debug.log(`Discarding empty "${defaultNavigationSpanName}" transaction that never received route information.`); + client?.recordDroppedEvent('sample_rate', 'transaction'); + }, + ); }; /** diff --git a/packages/core/src/js/tracing/reactnativenavigation.ts b/packages/core/src/js/tracing/reactnativenavigation.ts index 33a3275fb0..c35f22922c 100644 --- a/packages/core/src/js/tracing/reactnativenavigation.ts +++ b/packages/core/src/js/tracing/reactnativenavigation.ts @@ -9,7 +9,7 @@ import { } from '@sentry/core'; import type { EmitterSubscription } from '../utils/rnlibrariesinterface'; import { isSentrySpan } from '../utils/span'; -import { ignoreEmptyBackNavigation } from './onSpanEndUtils'; +import { ignoreEmptyBackNavigation, ignoreEmptyRouteChangeTransactions } from './onSpanEndUtils'; import { SPAN_ORIGIN_AUTO_NAVIGATION_REACT_NATIVE_NAVIGATION } from './origin'; import type { ReactNativeTracingIntegration } from './reactnativetracing'; import { getReactNativeTracingIntegration } from './reactnativetracing'; @@ -140,6 +140,14 @@ export const reactNativeNavigationIntegration = ({ if (ignoreEmptyBackNavigationTransactions) { ignoreEmptyBackNavigation(getClient(), latestNavigationSpan); } + // Always discard transactions that never receive route information + const spanToCheck = latestNavigationSpan; + ignoreEmptyRouteChangeTransactions( + getClient(), + spanToCheck, + DEFAULT_NAVIGATION_SPAN_NAME, + () => latestNavigationSpan === spanToCheck, + ); stateChangeTimeout = setTimeout(discardLatestNavigationSpan.bind(this), routeChangeTimeoutMs); }; diff --git a/packages/core/src/js/tracing/reactnavigation.ts b/packages/core/src/js/tracing/reactnavigation.ts index 98ffdff22f..da4232e3fc 100644 --- a/packages/core/src/js/tracing/reactnavigation.ts +++ b/packages/core/src/js/tracing/reactnavigation.ts @@ -17,7 +17,7 @@ import { isSentrySpan } from '../utils/span'; import { RN_GLOBAL_OBJ } from '../utils/worldwide'; import type { UnsafeAction } from '../vendor/react-navigation/types'; import { NATIVE } from '../wrapper'; -import { ignoreEmptyBackNavigation } from './onSpanEndUtils'; +import { ignoreEmptyBackNavigation, ignoreEmptyRouteChangeTransactions } from './onSpanEndUtils'; import { SPAN_ORIGIN_AUTO_NAVIGATION_REACT_NAVIGATION } from './origin'; import type { ReactNativeTracingIntegration } from './reactnativetracing'; import { getReactNativeTracingIntegration } from './reactnativetracing'; @@ -255,6 +255,14 @@ export const reactNavigationIntegration = ({ if (ignoreEmptyBackNavigationTransactions) { ignoreEmptyBackNavigation(getClient(), latestNavigationSpan); } + // Always discard transactions that never receive route information + const spanToCheck = latestNavigationSpan; + ignoreEmptyRouteChangeTransactions( + getClient(), + spanToCheck, + DEFAULT_NAVIGATION_SPAN_NAME, + () => latestNavigationSpan === spanToCheck, + ); if (enableTimeToInitialDisplay && latestNavigationSpan) { NATIVE.setActiveSpanId(latestNavigationSpan.spanContext().spanId); diff --git a/packages/core/test/tracing/reactnavigation.test.ts b/packages/core/test/tracing/reactnavigation.test.ts index 0b7ce0a1aa..8f810e3a1b 100644 --- a/packages/core/test/tracing/reactnavigation.test.ts +++ b/packages/core/test/tracing/reactnavigation.test.ts @@ -329,6 +329,62 @@ describe('ReactNavigationInstrumentation', () => { ); }); + test('empty Route Change transaction is not sent when route is undefined', async () => { + setupTestClient(); + jest.runOnlyPendingTimers(); // Flush the init transaction + + mockNavigation.emitNavigationWithUndefinedRoute(); + jest.runOnlyPendingTimers(); // Flush the empty route change transaction + + await client.flush(); + + // Only the initial transaction should be sent + expect(client.eventQueue.length).toBe(1); + expect(client.event).toEqual( + expect.objectContaining({ + type: 'transaction', + transaction: 'Initial Screen', + }), + ); + }); + + test('empty Route Change transaction is recorded as dropped', async () => { + const mockRecordDroppedEvent = jest.fn(); + setupTestClient(); + client.recordDroppedEvent = mockRecordDroppedEvent; + jest.runOnlyPendingTimers(); // Flush the init transaction + + mockNavigation.emitNavigationWithUndefinedRoute(); + jest.runOnlyPendingTimers(); // Flush the empty route change transaction + + await client.flush(); + + // Should have recorded a dropped transaction + expect(mockRecordDroppedEvent).toHaveBeenCalledWith('sample_rate', 'transaction'); + }); + + test('empty Route Change transaction is not sent after multiple undefined routes', async () => { + setupTestClient(); + jest.runOnlyPendingTimers(); // Flush the init transaction + + mockNavigation.emitNavigationWithUndefinedRoute(); + jest.runOnlyPendingTimers(); // Flush the first empty route change transaction + + mockNavigation.emitNavigationWithUndefinedRoute(); + jest.runOnlyPendingTimers(); // Flush the second empty route change transaction + + await client.flush(); + + // Only the initial transaction should be sent + expect(client.eventQueue.length).toBe(1); + expect(client.event).toEqual( + expect.objectContaining({ + type: 'transaction', + transaction: 'Initial Screen', + }), + ); + }); + describe('navigation container registration', () => { test('registers navigation container object ref', () => { const instrumentation = reactNavigationIntegration(); diff --git a/packages/core/test/tracing/reactnavigationutils.ts b/packages/core/test/tracing/reactnavigationutils.ts index bbc4576859..aa164087f9 100644 --- a/packages/core/test/tracing/reactnavigationutils.ts +++ b/packages/core/test/tracing/reactnavigationutils.ts @@ -68,6 +68,13 @@ export function createMockNavigationAndAttachTo(sut: ReturnType { + mockedNavigationContained.listeners['__unsafe_action__'](navigationAction); + mockedNavigationContained.currentRoute = undefined as any; + mockedNavigationContained.listeners['state']({ + // this object is not used by the instrumentation + }); + }, }; sut.registerNavigationContainer(mockRef(mockedNavigationContained));