Skip to content

Commit 3e5eac5

Browse files
mydeas1gr1d
mydea
and
s1gr1d
authored
feat(browser): Detect redirects when emitting navigation spans (#16324)
Closes #15286 This PR adds a new option to `browserTracingIntegration`, `detectRedirects`, which is enabled by default. If this is enabled, the integration will try to detect if a navigation is actually a redirect based on a simple heuristic, and in this case, will not end the ongoing pageload/navigation, but instead let it run and create a `navigation.redirect` zero-duration span instead. An example trace for this would be: https://sentry-sdks.sentry.io/explore/discover/trace/95280de69dc844448d39de7458eab527/?dataset=transactions&eventId=8a1150fd1dc846e4ac8420ccf03ad0ee&field=title&field=project&field=user.display&field=timestamp&name=All%20Errors&project=4504956726345728&query=&queryDataset=transaction-like&sort=-timestamp&source=discover&statsPeriod=5m&timestamp=1747646096&yAxis=count%28%29 ![image](https://github.com/user-attachments/assets/5f30210b-5ad0-44d9-ad49-bb00354b5fe5) Where the respective index route that triggered this has this code: ```js setTimeout(() => { window.history.pushState({}, "", "/test-sub-page"); fetch('https://example.com') }, 100); ``` The used heuristic is: * If the ongoing pageload/navigation was started less than 300ms ago... * ... and no click has happened in this time... * ... then we consider the navigation a redirect this limit was chosen somewhat arbitrarily, open for other suggestions too. While this logic will not be 100% bullet proof, it should be reliable enough and likely better than what we have today. Users can opt-out of this logic via `browserTracingIntegration({ detectRedirects: false })`, if needed. --------- Co-authored-by: Sigrid Huemer <[email protected]>
1 parent 2a7a74b commit 3e5eac5

File tree

26 files changed

+571
-40
lines changed

26 files changed

+571
-40
lines changed

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/linked-traces/negatively-sampled/init.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@ window.Sentry = Sentry;
44

55
Sentry.init({
66
dsn: 'https://[email protected]/1337',
7-
integrations: [Sentry.browserTracingIntegration()],
7+
// We want to ignore redirects for this test
8+
integrations: [Sentry.browserTracingIntegration({ detectRedirects: false })],
89
tracesSampler: ctx => {
910
if (ctx.attributes['sentry.origin'] === 'auto.pageload.browser') {
1011
return 0;

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-aborting-pageload/init.js‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,12 @@ window.Sentry = Sentry;
44

55
Sentry.init({
66
dsn: 'https://[email protected]/1337',
7-
integrations: [Sentry.browserTracingIntegration()],
7+
integrations: [Sentry.browserTracingIntegration({ idleTimeout: 2000 })],
88
tracesSampleRate: 1,
99
});
1010

11-
// Immediately navigate to a new page to abort the pageload
12-
window.history.pushState({}, '', '/sub-page');
11+
// Navigate to a new page to abort the pageload
12+
// We have to wait >300ms to avoid the redirect handling
13+
setTimeout(() => {
14+
window.history.pushState({}, '', '/sub-page');
15+
}, 500);

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/click-early/init.js‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
5+
Sentry.init({
6+
dsn: 'https://[email protected]/1337',
7+
integrations: [Sentry.browserTracingIntegration()],
8+
tracesSampleRate: 1,
9+
debug: true,
10+
});
11+
12+
document.getElementById('btn1').addEventListener('click', () => {
13+
// Trigger navigation later than click, so the last click is more than 300ms ago
14+
setTimeout(() => {
15+
window.history.pushState({}, '', '/sub-page');
16+
17+
// then trigger redirect inside of this navigation, which should be detected as a redirect
18+
// because the last click was more than 300ms ago
19+
setTimeout(() => {
20+
window.history.pushState({}, '', '/sub-page-redirect');
21+
}, 100);
22+
}, 400);
23+
});

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/click-early/template.html‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
<!doctype html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<button id="btn1">Trigger navigation</button>
7+
</html>

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/click-early/test.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import { expect } from '@playwright/test';
2+
import { sentryTest } from '../../../../../utils/fixtures';
3+
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
4+
5+
sentryTest(
6+
'should create a navigation.redirect span if a click happened more than 300ms before navigation',
7+
async ({ getLocalTestUrl, page }) => {
8+
if (shouldSkipTracingTest()) {
9+
sentryTest.skip();
10+
}
11+
12+
const url = await getLocalTestUrl({ testDir: __dirname });
13+
14+
const pageloadRequestPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');
15+
const navigationRequestPromise = waitForTransactionRequest(
16+
page,
17+
event => event.contexts?.trace?.op === 'navigation',
18+
);
19+
20+
await page.goto(url);
21+
22+
await pageloadRequestPromise;
23+
24+
// Now trigger navigation, and then a redirect in the navigation, with
25+
await page.click('#btn1');
26+
27+
const navigationRequest = envelopeRequestParser(await navigationRequestPromise);
28+
29+
expect(navigationRequest.contexts?.trace?.op).toBe('navigation');
30+
expect(navigationRequest.transaction).toEqual('/sub-page');
31+
32+
const spans = navigationRequest.spans || [];
33+
34+
expect(spans).toContainEqual(
35+
expect.objectContaining({
36+
op: 'navigation.redirect',
37+
description: '/sub-page-redirect',
38+
}),
39+
);
40+
},
41+
);

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/click/init.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
5+
Sentry.init({
6+
dsn: 'https://[email protected]/1337',
7+
integrations: [Sentry.browserTracingIntegration()],
8+
tracesSampleRate: 1,
9+
debug: true,
10+
});

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/click/subject.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
document.getElementById('btn1').addEventListener('click', () => {
2+
// trigger redirect immediately
3+
window.history.pushState({}, '', '/sub-page');
4+
});
5+
6+
// Now trigger click, which should trigger navigation
7+
document.getElementById('btn1').click();

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/click/template.html‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
<!doctype html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<button id="btn1" type="button">Trigger navigation</button>
7+
</html>

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/click/test.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
import { expect } from '@playwright/test';
2+
import { sentryTest } from '../../../../../utils/fixtures';
3+
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
4+
5+
sentryTest(
6+
'should not create a navigation.redirect span if a click happened before navigation',
7+
async ({ getLocalTestUrl, page }) => {
8+
if (shouldSkipTracingTest()) {
9+
sentryTest.skip();
10+
}
11+
12+
const url = await getLocalTestUrl({ testDir: __dirname });
13+
14+
const pageloadRequestPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');
15+
const navigationRequestPromise = waitForTransactionRequest(
16+
page,
17+
event => event.contexts?.trace?.op === 'navigation',
18+
);
19+
20+
await page.goto(url);
21+
22+
const pageloadRequest = envelopeRequestParser(await pageloadRequestPromise);
23+
// Ensure a navigation span is sent, too
24+
await navigationRequestPromise;
25+
26+
const spans = pageloadRequest.spans || [];
27+
28+
expect(spans).not.toContainEqual(
29+
expect.objectContaining({
30+
op: 'navigation.redirect',
31+
}),
32+
);
33+
},
34+
);

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/immediately/init.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
5+
Sentry.init({
6+
dsn: 'https://[email protected]/1337',
7+
integrations: [Sentry.browserTracingIntegration()],
8+
tracesSampleRate: 1,
9+
});
10+
11+
// trigger redirect immediately
12+
window.history.pushState({}, '', '/sub-page');

0 commit comments

Comments
 (0)