Skip to content

Commit 8c195b6

Browse files
antonisantonis
authored
Merge branch 'main' into antonis/replay-beforeErrorSampling
2 parents 9eac259 + c198191 commit 8c195b6

File tree

6 files changed

+159
-19
lines changed

6 files changed

+159
-19
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
### Fixes
1919

2020
- Preserves interaction span context during app restart to allow proper replay capture ([#5386](https://github.com/getsentry/sentry-react-native/pull/5386))
21+
- Discard empty Route Change transactions ([#5387](https://github.com/getsentry/sentry-react-native/issues/5387))
2122

2223
### Dependencies
2324

‎packages/core/src/js/tracing/onSpanEndUtils.ts‎

Lines changed: 77 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,28 @@ export const adjustTransactionDuration = (client: Client, span: Span, maxDuratio
4343
});
4444
};
4545

46-
export const ignoreEmptyBackNavigation = (client: Client | undefined, span: Span | undefined): void => {
46+
/**
47+
* Helper function to filter out auto-instrumentation child spans.
48+
*/
49+
function getMeaningfulChildSpans(span: Span): Span[] {
50+
const children = getSpanDescendants(span);
51+
return children.filter(
52+
child =>
53+
child.spanContext().spanId !== span.spanContext().spanId &&
54+
spanToJSON(child).op !== 'ui.load.initial_display' &&
55+
spanToJSON(child).op !== 'navigation.processing',
56+
);
57+
}
58+
59+
/**
60+
* Generic helper to discard empty navigation spans based on a condition.
61+
*/
62+
function discardEmptyNavigationSpan(
63+
client: Client | undefined,
64+
span: Span | undefined,
65+
shouldDiscardFn: (span: Span) => boolean,
66+
onDiscardFn: (span: Span) => void,
67+
): void {
4768
if (!client) {
4869
debug.warn('Could not hook on spanEnd event because client is not defined.');
4970
return;
@@ -55,7 +76,7 @@ export const ignoreEmptyBackNavigation = (client: Client | undefined, span: Span
5576
}
5677

5778
if (!isRootSpan(span) || !isSentrySpan(span)) {
58-
debug.warn('Not sampling empty back spans only works for Sentry Transactions (Root Spans).');
79+
debug.warn('Not sampling empty navigation spans only works for Sentry Transactions (Root Spans).');
5980
return;
6081
}
6182

@@ -64,27 +85,66 @@ export const ignoreEmptyBackNavigation = (client: Client | undefined, span: Span
6485
return;
6586
}
6687

67-
if (!spanToJSON(span).data?.['route.has_been_seen']) {
88+
if (!shouldDiscardFn(span)) {
6889
return;
6990
}
7091

71-
const children = getSpanDescendants(span);
72-
const filtered = children.filter(
73-
child =>
74-
child.spanContext().spanId !== span.spanContext().spanId &&
75-
spanToJSON(child).op !== 'ui.load.initial_display' &&
76-
spanToJSON(child).op !== 'navigation.processing',
77-
);
78-
79-
if (filtered.length <= 0) {
80-
// filter children must include at least one span not created by the navigation automatic instrumentation
81-
debug.log(
82-
'Not sampling transaction as route has been seen before. Pass ignoreEmptyBackNavigationTransactions = false to disable this feature.',
83-
);
84-
// Route has been seen before and has no child spans.
92+
const meaningfulChildren = getMeaningfulChildSpans(span);
93+
if (meaningfulChildren.length <= 0) {
94+
onDiscardFn(span);
8595
span['_sampled'] = false;
8696
}
8797
});
98+
}
99+
100+
export const ignoreEmptyBackNavigation = (client: Client | undefined, span: Span | undefined): void => {
101+
discardEmptyNavigationSpan(
102+
client,
103+
span,
104+
// Only discard if route has been seen before
105+
span => spanToJSON(span).data?.['route.has_been_seen'] === true,
106+
// Log message when discarding
107+
() => {
108+
debug.log(
109+
'Not sampling transaction as route has been seen before. Pass ignoreEmptyBackNavigationTransactions = false to disable this feature.',
110+
);
111+
},
112+
);
113+
};
114+
115+
/**
116+
* Discards empty "Route Change" transactions that never received route information.
117+
* This happens when navigation library emits a route change event but getCurrentRoute() returns undefined.
118+
* Such transactions don't contain any useful information and should not be sent to Sentry.
119+
*
120+
* This function must be called with a reference tracker function that can check if the span
121+
* was cleared from the integration's tracking (indicating it went through the state listener).
122+
*/
123+
export const ignoreEmptyRouteChangeTransactions = (
124+
client: Client | undefined,
125+
span: Span | undefined,
126+
defaultNavigationSpanName: string,
127+
isSpanStillTracked: () => boolean,
128+
): void => {
129+
discardEmptyNavigationSpan(
130+
client,
131+
span,
132+
// Only discard if:
133+
// 1. Still has default name
134+
// 2. No route information was set
135+
// 3. Still being tracked (state listener never called)
136+
span => {
137+
const spanJSON = spanToJSON(span);
138+
return (
139+
spanJSON.description === defaultNavigationSpanName && !spanJSON.data?.['route.name'] && isSpanStillTracked()
140+
);
141+
},
142+
// Log and record dropped event
143+
_span => {
144+
debug.log(`Discarding empty "${defaultNavigationSpanName}" transaction that never received route information.`);
145+
client?.recordDroppedEvent('sample_rate', 'transaction');
146+
},
147+
);
88148
};
89149

90150
/**

‎packages/core/src/js/tracing/reactnativenavigation.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
} from '@sentry/core';
1010
import type { EmitterSubscription } from '../utils/rnlibrariesinterface';
1111
import { isSentrySpan } from '../utils/span';
12-
import { ignoreEmptyBackNavigation } from './onSpanEndUtils';
12+
import { ignoreEmptyBackNavigation, ignoreEmptyRouteChangeTransactions } from './onSpanEndUtils';
1313
import { SPAN_ORIGIN_AUTO_NAVIGATION_REACT_NATIVE_NAVIGATION } from './origin';
1414
import type { ReactNativeTracingIntegration } from './reactnativetracing';
1515
import { getReactNativeTracingIntegration } from './reactnativetracing';
@@ -140,6 +140,14 @@ export const reactNativeNavigationIntegration = ({
140140
if (ignoreEmptyBackNavigationTransactions) {
141141
ignoreEmptyBackNavigation(getClient(), latestNavigationSpan);
142142
}
143+
// Always discard transactions that never receive route information
144+
const spanToCheck = latestNavigationSpan;
145+
ignoreEmptyRouteChangeTransactions(
146+
getClient(),
147+
spanToCheck,
148+
DEFAULT_NAVIGATION_SPAN_NAME,
149+
() => latestNavigationSpan === spanToCheck,
150+
);
143151

144152
stateChangeTimeout = setTimeout(discardLatestNavigationSpan.bind(this), routeChangeTimeoutMs);
145153
};

‎packages/core/src/js/tracing/reactnavigation.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import { isSentrySpan } from '../utils/span';
1717
import { RN_GLOBAL_OBJ } from '../utils/worldwide';
1818
import type { UnsafeAction } from '../vendor/react-navigation/types';
1919
import { NATIVE } from '../wrapper';
20-
import { ignoreEmptyBackNavigation } from './onSpanEndUtils';
20+
import { ignoreEmptyBackNavigation, ignoreEmptyRouteChangeTransactions } from './onSpanEndUtils';
2121
import { SPAN_ORIGIN_AUTO_NAVIGATION_REACT_NAVIGATION } from './origin';
2222
import type { ReactNativeTracingIntegration } from './reactnativetracing';
2323
import { getReactNativeTracingIntegration } from './reactnativetracing';
@@ -255,6 +255,14 @@ export const reactNavigationIntegration = ({
255255
if (ignoreEmptyBackNavigationTransactions) {
256256
ignoreEmptyBackNavigation(getClient(), latestNavigationSpan);
257257
}
258+
// Always discard transactions that never receive route information
259+
const spanToCheck = latestNavigationSpan;
260+
ignoreEmptyRouteChangeTransactions(
261+
getClient(),
262+
spanToCheck,
263+
DEFAULT_NAVIGATION_SPAN_NAME,
264+
() => latestNavigationSpan === spanToCheck,
265+
);
258266

259267
if (enableTimeToInitialDisplay && latestNavigationSpan) {
260268
NATIVE.setActiveSpanId(latestNavigationSpan.spanContext().spanId);

‎packages/core/test/tracing/reactnavigation.test.ts‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,62 @@ describe('ReactNavigationInstrumentation', () => {
329329
);
330330
});
331331

332+
test('empty Route Change transaction is not sent when route is undefined', async () => {
333+
setupTestClient();
334+
jest.runOnlyPendingTimers(); // Flush the init transaction
335+
336+
mockNavigation.emitNavigationWithUndefinedRoute();
337+
jest.runOnlyPendingTimers(); // Flush the empty route change transaction
338+
339+
await client.flush();
340+
341+
// Only the initial transaction should be sent
342+
expect(client.eventQueue.length).toBe(1);
343+
expect(client.event).toEqual(
344+
expect.objectContaining({
345+
type: 'transaction',
346+
transaction: 'Initial Screen',
347+
}),
348+
);
349+
});
350+
351+
test('empty Route Change transaction is recorded as dropped', async () => {
352+
const mockRecordDroppedEvent = jest.fn();
353+
setupTestClient();
354+
client.recordDroppedEvent = mockRecordDroppedEvent;
355+
jest.runOnlyPendingTimers(); // Flush the init transaction
356+
357+
mockNavigation.emitNavigationWithUndefinedRoute();
358+
jest.runOnlyPendingTimers(); // Flush the empty route change transaction
359+
360+
await client.flush();
361+
362+
// Should have recorded a dropped transaction
363+
expect(mockRecordDroppedEvent).toHaveBeenCalledWith('sample_rate', 'transaction');
364+
});
365+
366+
test('empty Route Change transaction is not sent after multiple undefined routes', async () => {
367+
setupTestClient();
368+
jest.runOnlyPendingTimers(); // Flush the init transaction
369+
370+
mockNavigation.emitNavigationWithUndefinedRoute();
371+
jest.runOnlyPendingTimers(); // Flush the first empty route change transaction
372+
373+
mockNavigation.emitNavigationWithUndefinedRoute();
374+
jest.runOnlyPendingTimers(); // Flush the second empty route change transaction
375+
376+
await client.flush();
377+
378+
// Only the initial transaction should be sent
379+
expect(client.eventQueue.length).toBe(1);
380+
expect(client.event).toEqual(
381+
expect.objectContaining({
382+
type: 'transaction',
383+
transaction: 'Initial Screen',
384+
}),
385+
);
386+
});
387+
332388
describe('navigation container registration', () => {
333389
test('registers navigation container object ref', () => {
334390
const instrumentation = reactNavigationIntegration();

‎packages/core/test/tracing/reactnavigationutils.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,13 @@ export function createMockNavigationAndAttachTo(sut: ReturnType<typeof reactNavi
6868
// this object is not used by the instrumentation
6969
});
7070
},
71+
emitNavigationWithUndefinedRoute: () => {
72+
mockedNavigationContained.listeners['__unsafe_action__'](navigationAction);
73+
mockedNavigationContained.currentRoute = undefined as any;
74+
mockedNavigationContained.listeners['state']({
75+
// this object is not used by the instrumentation
76+
});
77+
},
7178
};
7279
sut.registerNavigationContainer(mockRef(mockedNavigationContained));
7380

0 commit comments

Comments
 (0)