Skip to content

Commit 0cb306e

Browse files
authored
fix(browser): Add ok status to succesful idleSpans (#18139)
This came up while working on improvements for React Router wildcard routes. Looks like the successful browser `idleSpans` are reported with `unknown` status at the moment.
1 parent 5c184f4 commit 0cb306e

File tree

8 files changed

+108
-1
lines changed

8 files changed

+108
-1
lines changed

dev-packages/e2e-tests/test-applications/create-next-app/tests/client-transactions.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ test('Sends a pageload transaction to Sentry', async ({ page }) => {
2424
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
2525
op: 'pageload',
2626
origin: 'auto.pageload.nextjs.pages_router_instrumentation',
27+
status: 'ok',
2728
data: expect.objectContaining({
2829
'sentry.idle_span_finish_reason': 'idleTimeout',
2930
'sentry.op': 'pageload',
@@ -69,6 +70,7 @@ test('captures a navigation transaction to Sentry', async ({ page }) => {
6970
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
7071
op: 'navigation',
7172
origin: 'auto.navigation.nextjs.pages_router_instrumentation',
73+
status: 'ok',
7274
data: expect.objectContaining({
7375
'sentry.idle_span_finish_reason': 'idleTimeout',
7476
'sentry.op': 'navigation',

dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/transactions.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ test('Sends a pageload transaction', async ({ page }) => {
3333
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
3434
op: 'pageload',
3535
origin: 'auto.pageload.nextjs.app_router_instrumentation',
36+
status: 'ok',
3637
data: expect.objectContaining({
3738
'sentry.op': 'pageload',
3839
'sentry.origin': 'auto.pageload.nextjs.app_router_instrumentation',

dev-packages/e2e-tests/test-applications/nextjs-pages-dir/tests/transactions.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ test('Sends a pageload transaction', async ({ page }) => {
3333
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
3434
op: 'pageload',
3535
origin: 'auto.pageload.nextjs.pages_router_instrumentation',
36+
status: 'ok',
3637
data: expect.objectContaining({
3738
'sentry.op': 'pageload',
3839
'sentry.origin': 'auto.pageload.nextjs.pages_router_instrumentation',

dev-packages/e2e-tests/test-applications/react-create-browser-router/tests/transactions.test.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ test('Captures a pageload transaction', async ({ page }) => {
3636
span_id: expect.stringMatching(/[a-f0-9]{16}/),
3737
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
3838
origin: 'auto.pageload.react.reactrouter_v6',
39+
status: 'ok',
3940
}),
4041
);
4142
});
@@ -72,6 +73,7 @@ test('Captures a navigation transaction', async ({ page }) => {
7273
span_id: expect.stringMatching(/[a-f0-9]{16}/),
7374
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
7475
origin: 'auto.navigation.react.reactrouter_v6',
76+
status: 'ok',
7577
});
7678

7779
expect(transactionEvent).toEqual(
@@ -107,6 +109,7 @@ test('Captures a lazy pageload transaction', async ({ page }) => {
107109
span_id: expect.stringMatching(/[a-f0-9]{16}/),
108110
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
109111
origin: 'auto.pageload.react.reactrouter_v6',
112+
status: 'ok',
110113
});
111114

112115
expect(transactionEvent).toEqual(
@@ -169,6 +172,7 @@ test('Captures a lazy navigation transaction', async ({ page }) => {
169172
span_id: expect.stringMatching(/[a-f0-9]{16}/),
170173
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
171174
origin: 'auto.navigation.react.reactrouter_v6',
175+
status: 'ok',
172176
});
173177

174178
expect(transactionEvent).toEqual(

dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/transactions.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ test('Captures a pageload transaction', async ({ page }) => {
3131
span_id: expect.stringMatching(/[a-f0-9]{16}/),
3232
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
3333
origin: 'auto.pageload.react.reactrouter_v6',
34+
status: 'ok',
3435
});
3536

3637
expect(transactionEvent).toEqual(
@@ -136,6 +137,7 @@ test('Captures a navigation transaction', async ({ page }) => {
136137
span_id: expect.stringMatching(/[a-f0-9]{16}/),
137138
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
138139
origin: 'auto.navigation.react.reactrouter_v6',
140+
status: 'ok',
139141
});
140142

141143
expect(transactionEvent).toEqual(

dev-packages/e2e-tests/test-applications/react-create-memory-router/tests/transactions.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ test('Captures a pageload transaction', async ({ page }) => {
3333
span_id: expect.stringMatching(/[a-f0-9]{16}/),
3434
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
3535
origin: 'auto.pageload.react.reactrouter_v6',
36+
status: 'ok',
3637
}),
3738
);
3839
});
@@ -69,6 +70,7 @@ test('Captures a navigation transaction', async ({ page }) => {
6970
span_id: expect.stringMatching(/[a-f0-9]{16}/),
7071
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
7172
origin: 'auto.navigation.react.reactrouter_v6',
73+
status: 'ok',
7274
});
7375

7476
expect(transactionEvent).toEqual(

dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ test('Creates a pageload transaction with parameterized route', async ({ page })
2121
expect(event.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
2222
expect(event.type).toBe('transaction');
2323
expect(event.contexts?.trace?.op).toBe('pageload');
24+
expect(event.contexts?.trace?.status).toBe('ok');
2425
});
2526

2627
test('Does not create a navigation transaction on initial load to deep lazy route', async ({ page }) => {
@@ -82,6 +83,7 @@ test('Creates a navigation transaction inside a lazy route', async ({ page }) =>
8283
expect(event.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
8384
expect(event.type).toBe('transaction');
8485
expect(event.contexts?.trace?.op).toBe('navigation');
86+
expect(event.contexts?.trace?.status).toBe('ok');
8587
});
8688

8789
test('Creates navigation transactions between two different lazy routes', async ({ page }) => {
@@ -498,3 +500,90 @@ test('Updates navigation transaction name correctly when span is cancelled early
498500
expect(['externalFinish', 'cancelled']).toContain(idleSpanFinishReason);
499501
}
500502
});
503+
504+
test('Creates separate transactions for rapid consecutive navigations', async ({ page }) => {
505+
await page.goto('/');
506+
507+
// First navigation: / -> /lazy/inner/:id/:anotherId/:someAnotherId
508+
const firstTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
509+
return (
510+
!!transactionEvent?.transaction &&
511+
transactionEvent.contexts?.trace?.op === 'navigation' &&
512+
transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId'
513+
);
514+
});
515+
516+
const navigationToInner = page.locator('id=navigation');
517+
await expect(navigationToInner).toBeVisible();
518+
await navigationToInner.click();
519+
520+
const firstEvent = await firstTransactionPromise;
521+
522+
// Verify first transaction
523+
expect(firstEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
524+
expect(firstEvent.contexts?.trace?.op).toBe('navigation');
525+
expect(firstEvent.contexts?.trace?.status).toBe('ok');
526+
const firstTraceId = firstEvent.contexts?.trace?.trace_id;
527+
const firstSpanId = firstEvent.contexts?.trace?.span_id;
528+
529+
// Second navigation: /lazy/inner -> /another-lazy/sub/:id/:subId
530+
const secondTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
531+
return (
532+
!!transactionEvent?.transaction &&
533+
transactionEvent.contexts?.trace?.op === 'navigation' &&
534+
transactionEvent.transaction === '/another-lazy/sub/:id/:subId'
535+
);
536+
});
537+
538+
const navigationToAnother = page.locator('id=navigate-to-another-from-inner');
539+
await expect(navigationToAnother).toBeVisible();
540+
await navigationToAnother.click();
541+
542+
const secondEvent = await secondTransactionPromise;
543+
544+
// Verify second transaction
545+
expect(secondEvent.transaction).toBe('/another-lazy/sub/:id/:subId');
546+
expect(secondEvent.contexts?.trace?.op).toBe('navigation');
547+
expect(secondEvent.contexts?.trace?.status).toBe('ok');
548+
const secondTraceId = secondEvent.contexts?.trace?.trace_id;
549+
const secondSpanId = secondEvent.contexts?.trace?.span_id;
550+
551+
// Third navigation: /another-lazy -> /lazy/inner/:id/:anotherId/:someAnotherId (back to same route as first)
552+
const thirdTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
553+
return (
554+
!!transactionEvent?.transaction &&
555+
transactionEvent.contexts?.trace?.op === 'navigation' &&
556+
transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId' &&
557+
// Ensure we're not matching the first transaction again
558+
transactionEvent.contexts?.trace?.trace_id !== firstTraceId
559+
);
560+
});
561+
562+
const navigationBackToInner = page.locator('id=navigate-to-inner-from-deep');
563+
await expect(navigationBackToInner).toBeVisible();
564+
await navigationBackToInner.click();
565+
566+
const thirdEvent = await thirdTransactionPromise;
567+
568+
// Verify third transaction
569+
expect(thirdEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
570+
expect(thirdEvent.contexts?.trace?.op).toBe('navigation');
571+
expect(thirdEvent.contexts?.trace?.status).toBe('ok');
572+
const thirdTraceId = thirdEvent.contexts?.trace?.trace_id;
573+
const thirdSpanId = thirdEvent.contexts?.trace?.span_id;
574+
575+
// Verify each navigation created a separate transaction with unique trace and span IDs
576+
expect(firstTraceId).toBeDefined();
577+
expect(secondTraceId).toBeDefined();
578+
expect(thirdTraceId).toBeDefined();
579+
580+
// All trace IDs should be unique
581+
expect(firstTraceId).not.toBe(secondTraceId);
582+
expect(secondTraceId).not.toBe(thirdTraceId);
583+
expect(firstTraceId).not.toBe(thirdTraceId);
584+
585+
// All span IDs should be unique
586+
expect(firstSpanId).not.toBe(secondSpanId);
587+
expect(secondSpanId).not.toBe(thirdSpanId);
588+
expect(firstSpanId).not.toBe(thirdSpanId);
589+
});

packages/core/src/tracing/idleSpan.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import { timestampInSeconds } from '../utils/time';
1919
import { freezeDscOnSpan, getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
2020
import { SentryNonRecordingSpan } from './sentryNonRecordingSpan';
2121
import { SentrySpan } from './sentrySpan';
22-
import { SPAN_STATUS_ERROR } from './spanstatus';
22+
import { SPAN_STATUS_ERROR, SPAN_STATUS_OK } from './spanstatus';
2323
import { startInactiveSpan } from './trace';
2424

2525
export const TRACING_DEFAULTS = {
@@ -302,6 +302,12 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
302302
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON, _finishReason);
303303
}
304304

305+
// Set span status to 'ok' if it hasn't been explicitly set to an error status
306+
const currentStatus = spanJSON.status;
307+
if (!currentStatus || currentStatus === 'unknown') {
308+
span.setStatus({ code: SPAN_STATUS_OK });
309+
}
310+
305311
debug.log(`[Tracing] Idle span "${spanJSON.op}" finished`);
306312

307313
const childSpans = getSpanDescendants(span).filter(child => child !== span);

0 commit comments

Comments
 (0)