Skip to content

feat(browser): Detect redirects when emitting navigation spans #16324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [Sentry.browserTracingIntegration()],
// We want to ignore redirects for this test
integrations: [Sentry.browserTracingIntegration({ detectRedirects: false })],
tracesSampler: ctx => {
if (ctx.attributes['sentry.origin'] === 'auto.pageload.browser') {
return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@ window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [Sentry.browserTracingIntegration()],
integrations: [Sentry.browserTracingIntegration({ idleTimeout: 2000 })],
tracesSampleRate: 1,
});

// Immediately navigate to a new page to abort the pageload
window.history.pushState({}, '', '/sub-page');
// Navigate to a new page to abort the pageload
// We have to wait >300ms to avoid the redirect handling
setTimeout(() => {
window.history.pushState({}, '', '/sub-page');
}, 500);
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [Sentry.browserTracingIntegration()],
tracesSampleRate: 1,
debug: true,
});

document.getElementById('btn1').addEventListener('click', () => {
// Trigger navigation later than click, so the last click is more than 300ms ago
setTimeout(() => {
window.history.pushState({}, '', '/sub-page');

// then trigger redirect inside of this navigation, which should be detected as a redirect
// because the last click was more than 300ms ago
setTimeout(() => {
window.history.pushState({}, '', '/sub-page-redirect');
}, 100);
}, 400);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<!doctype html>
<html>
<head>
<meta charset="utf-8" />
</head>
<button id="btn1">Trigger navigation</button>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { expect } from '@playwright/test';
import { sentryTest } from '../../../../../utils/fixtures';
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';

sentryTest(
'should create a navigation.redirect span if a click happened more than 300ms before navigation',
async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });

const pageloadRequestPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');
const navigationRequestPromise = waitForTransactionRequest(
page,
event => event.contexts?.trace?.op === 'navigation',
);

await page.goto(url);

await pageloadRequestPromise;

// Now trigger navigation, and then a redirect in the navigation, with
await page.click('#btn1');

const navigationRequest = envelopeRequestParser(await navigationRequestPromise);

expect(navigationRequest.contexts?.trace?.op).toBe('navigation');
expect(navigationRequest.transaction).toEqual('/sub-page');

const spans = navigationRequest.spans || [];

expect(spans).toContainEqual(
expect.objectContaining({
op: 'navigation.redirect',
description: '/sub-page-redirect',
}),
);
},
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [Sentry.browserTracingIntegration()],
tracesSampleRate: 1,
debug: true,
});

document.getElementById('btn1').addEventListener('click', () => {
// trigger redirect immediately
window.history.pushState({}, '', '/sub-page');
});

// Now trigger click, whic should trigger navigation
document.getElementById('btn1').click();
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<!doctype html>
<html>
<head>
<meta charset="utf-8" />
</head>
<button id="btn1">Trigger navigation</button>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { expect } from '@playwright/test';
import { sentryTest } from '../../../../../utils/fixtures';
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';

sentryTest(
'should not create a navigation.redirect span if a click happened before navigation',
async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });

const pageloadRequestPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');
const navigationRequestPromise = waitForTransactionRequest(
page,
event => event.contexts?.trace?.op === 'navigation',
);

await page.goto(url);

const pageloadRequest = envelopeRequestParser(await pageloadRequestPromise);
// Ensure a navigation span is sent, too
await navigationRequestPromise;

const spans = pageloadRequest.spans || [];

expect(spans).not.toContainEqual(
expect.objectContaining({
op: 'navigation.redirect',
}),
);
},
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [Sentry.browserTracingIntegration()],
tracesSampleRate: 1,
});

// trigger redirect immediately
window.history.pushState({}, '', '/sub-page');
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { expect } from '@playwright/test';
import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
} from '@sentry/core';
import { sentryTest } from '../../../../../utils/fixtures';
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';

sentryTest('should create a pageload transaction with navigation.redirect span', async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });

const pageloadRequestPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');

await page.goto(url);

const pageloadRequest = envelopeRequestParser(await pageloadRequestPromise);

expect(pageloadRequest.contexts?.trace?.op).toBe('pageload');

expect(pageloadRequest.contexts?.trace?.data).toMatchObject({
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
['sentry.idle_span_finish_reason']: 'idleTimeout',
});

expect(pageloadRequest.request).toEqual({
headers: {
'User-Agent': expect.any(String),
},
url: 'http://sentry-test.io/index.html',
});

const spans = pageloadRequest.spans || [];

expect(spans).toContainEqual(
expect.objectContaining({
op: 'navigation.redirect',
}),
);

const navigationSpan = spans.find(span => span.op === 'navigation.redirect');
expect(navigationSpan?.timestamp).toEqual(navigationSpan?.start_timestamp);
expect(navigationSpan).toEqual({
data: {
'sentry.op': 'navigation.redirect',
'sentry.origin': 'auto.navigation.browser',
'sentry.source': 'url',
},
description: '/sub-page',
op: 'navigation.redirect',
origin: 'auto.navigation.browser',
parent_span_id: pageloadRequest.contexts!.trace!.span_id,
span_id: expect.any(String),
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: expect.any(String),
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [Sentry.browserTracingIntegration()],
tracesSampleRate: 1,
});

// trigger redirect later
setTimeout(() => {
window.history.pushState({}, '', '/sub-page');
}, 400);
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { expect } from '@playwright/test';
import { sentryTest } from '../../../../../utils/fixtures';
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';

sentryTest(
'should not create a navigation.redirect span if redirect happened more than 300ms after pageload',
async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });

const pageloadRequestPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');
const navigationRequestPromise = waitForTransactionRequest(
page,
event => event.contexts?.trace?.op === 'navigation',
);

await page.goto(url);

const pageloadRequest = envelopeRequestParser(await pageloadRequestPromise);
// Ensure a navigation span is sent, too
await navigationRequestPromise;

const spans = pageloadRequest.spans || [];

expect(spans).not.toContainEqual(
expect.objectContaining({
op: 'navigation.redirect',
}),
);
},
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [
Sentry.browserTracingIntegration({
detectRedirects: false,
}),
],
tracesSampleRate: 1,
});

// trigger redirect immediately
window.history.pushState({}, '', '/sub-page');
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { expect } from '@playwright/test';
import { sentryTest } from '../../../../../utils/fixtures';
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';

sentryTest(
'should not create a navigation.redirect span if `detectRedirects` is set to false',
async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });

const pageloadRequestPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');
const navigationRequestPromise = waitForTransactionRequest(
page,
event => event.contexts?.trace?.op === 'navigation',
);

await page.goto(url);

const pageloadRequest = envelopeRequestParser(await pageloadRequestPromise);
// Ensure a navigation span is sent, too
await navigationRequestPromise;

const spans = pageloadRequest.spans || [];

expect(spans).not.toContainEqual(
expect.objectContaining({
op: 'navigation.redirect',
}),
);
},
);
Loading
Loading