Skip to content

Commit dded1f2

Browse files
mydeamydea
committed
fixes & tests
1 parent 9dc3d78 commit dded1f2

File tree

12 files changed

+298
-26
lines changed

12 files changed

+298
-26
lines changed

‎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: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
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 redirect immediately
14+
window.history.pushState({}, '', '/sub-page');
15+
});
16+
17+
// Now trigger click, whic should trigger navigation
18+
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">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');

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

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
import { expect } from '@playwright/test';
2+
import {
3+
SEMANTIC_ATTRIBUTE_SENTRY_OP,
4+
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
5+
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
6+
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
7+
} from '@sentry/core';
8+
import { sentryTest } from '../../../../../utils/fixtures';
9+
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
10+
11+
sentryTest('should create a pageload transaction with navigation.redirect span', async ({ getLocalTestUrl, page }) => {
12+
if (shouldSkipTracingTest()) {
13+
sentryTest.skip();
14+
}
15+
16+
const url = await getLocalTestUrl({ testDir: __dirname });
17+
18+
const pageloadRequestPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');
19+
20+
await page.goto(url);
21+
22+
const pageloadRequest = envelopeRequestParser(await pageloadRequestPromise);
23+
24+
expect(pageloadRequest.contexts?.trace?.op).toBe('pageload');
25+
26+
expect(pageloadRequest.contexts?.trace?.data).toMatchObject({
27+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
28+
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
29+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
30+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
31+
['sentry.idle_span_finish_reason']: 'idleTimeout',
32+
});
33+
34+
expect(pageloadRequest.request).toEqual({
35+
headers: {
36+
'User-Agent': expect.any(String),
37+
},
38+
url: 'http://sentry-test.io/index.html',
39+
});
40+
41+
const spans = pageloadRequest.spans || [];
42+
43+
expect(spans).toContainEqual(
44+
expect.objectContaining({
45+
op: 'navigation.redirect',
46+
}),
47+
);
48+
49+
const navigationSpan = spans.find(span => span.op === 'navigation.redirect');
50+
expect(navigationSpan?.timestamp).toEqual(navigationSpan?.start_timestamp);
51+
expect(navigationSpan).toEqual({
52+
data: {
53+
'sentry.op': 'navigation.redirect',
54+
'sentry.origin': 'auto.navigation.browser',
55+
'sentry.source': 'url',
56+
},
57+
description: '/sub-page',
58+
op: 'navigation.redirect',
59+
origin: 'auto.navigation.browser',
60+
parent_span_id: pageloadRequest.contexts!.trace!.span_id,
61+
span_id: expect.any(String),
62+
start_timestamp: expect.any(Number),
63+
timestamp: expect.any(Number),
64+
trace_id: expect.any(String),
65+
});
66+
});

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
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 later
12+
setTimeout(() => {
13+
window.history.pushState({}, '', '/sub-page');
14+
}, 400);

‎dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/late/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 redirect happened more than 300ms after pageload',
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+
);

‎packages/browser/src/tracing/browserTracingIntegration.ts

Lines changed: 36 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -521,23 +521,19 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
521521

522522
startingUrl = undefined;
523523
const parsed = parseStringToURLObject(to);
524-
startBrowserTracingNavigationSpan(client, {
525-
name: parsed?.pathname || WINDOW.location.pathname,
526-
attributes: {
527-
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
528-
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser',
524+
const activeSpan = getActiveIdleSpan(client);
525+
const navigationIsRedirect = activeSpan && isRedirect(activeSpan, lastClickTimestamp);
526+
startBrowserTracingNavigationSpan(
527+
client,
528+
{
529+
name: parsed?.pathname || WINDOW.location.pathname,
530+
attributes: {
531+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
532+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser',
533+
},
529534
},
530-
});
531-
532-
// We store the normalized request data on the scope, so we get the request data at time of span creation
533-
// otherwise, the URL etc. may already be of the following navigation, and we'd report the wrong URL
534-
getCurrentScope().setSDKProcessingMetadata({
535-
normalizedRequest: {
536-
...getHttpRequestData(),
537-
// Ensure to set this, so this matches the target route even if the URL has not yet been updated
538-
url: to,
539-
},
540-
});
535+
!navigationIsRedirect ? { url: to } : undefined,
536+
);
541537
});
542538
}
543539
}
@@ -589,10 +585,27 @@ export function startBrowserTracingPageLoadSpan(
589585
* Manually start a navigation span.
590586
* This will only do something if a browser tracing integration has been setup.
591587
*/
592-
export function startBrowserTracingNavigationSpan(client: Client, spanOptions: StartSpanOptions): Span | undefined {
588+
export function startBrowserTracingNavigationSpan(
589+
client: Client,
590+
spanOptions: StartSpanOptions,
591+
options?: { url?: string },
592+
): Span | undefined {
593593
client.emit('startNavigationSpan', spanOptions);
594594

595-
getCurrentScope().setTransactionName(spanOptions.name);
595+
const scope = getCurrentScope();
596+
scope.setTransactionName(spanOptions.name);
597+
598+
// We store the normalized request data on the scope, so we get the request data at time of span creation
599+
// otherwise, the URL etc. may already be of the following navigation, and we'd report the wrong URL
600+
const url = options?.url;
601+
if (url) {
602+
scope.setSDKProcessingMetadata({
603+
normalizedRequest: {
604+
...getHttpRequestData(),
605+
url,
606+
},
607+
});
608+
}
596609

597610
return getActiveIdleSpan(client);
598611
}
@@ -679,24 +692,24 @@ function setActiveIdleSpan(client: Client, span: Span | undefined): void {
679692
addNonEnumerableProperty(client, ACTIVE_IDLE_SPAN_PROPERTY, span);
680693
}
681694

682-
// The max. time in ms between two pageload/navigation spans that makes us consider the second one a redirect
683-
const REDIRECT_THRESHOLD = 300;
695+
// The max. time in seconds between two pageload/navigation spans that makes us consider the second one a redirect
696+
const REDIRECT_THRESHOLD = 0.3;
684697

685698
function isRedirect(activeSpan: Span, lastClickTimestamp: number | undefined): boolean {
686699
const spanData = spanToJSON(activeSpan);
687700

688701
const now = dateTimestampInSeconds();
689702

690-
// More than 500ms since last navigation/pageload span?
703+
// More than 300ms since last navigation/pageload span?
691704
// --> never consider this a redirect
692705
const startTimestamp = spanData.start_timestamp;
693706
if (now - startTimestamp > REDIRECT_THRESHOLD) {
694707
return false;
695708
}
696709

697-
// More than 500ms since last click?
710+
// A click happened in the last 300ms?
698711
// --> never consider this a redirect
699-
if (lastClickTimestamp && now - lastClickTimestamp > REDIRECT_THRESHOLD) {
712+
if (lastClickTimestamp && now - lastClickTimestamp <= REDIRECT_THRESHOLD) {
700713
return false;
701714
}
702715

0 commit comments

Comments
 (0)