Skip to content

fix(browser): Ensure pageload trace remains active after pageload span finished #11600

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

Merged
merged 3 commits into from
Apr 15, 2024
Merged
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved this test to the trace-lifetime directory where I plan on putting all tests from #11599

Original file line number Diff line number Diff line change
Expand Up @@ -49,25 +49,3 @@ sentryTest('should create a navigation transaction on page navigation', async ({

expect(pageloadSpanId).not.toEqual(navigationSpanId);
});

sentryTest('should create a new trace for for multiple navigations', async ({ getLocalTestPath, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

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

await getFirstSentryEnvelopeRequest<Event>(page, url);
const navigationEvent1 = await getFirstSentryEnvelopeRequest<Event>(page, `${url}#foo`);
const navigationEvent2 = await getFirstSentryEnvelopeRequest<Event>(page, `${url}#bar`);

expect(navigationEvent1.contexts?.trace?.op).toBe('navigation');
expect(navigationEvent2.contexts?.trace?.op).toBe('navigation');

const navigation1TraceId = navigationEvent1.contexts?.trace?.trace_id;
const navigation2TraceId = navigationEvent2.contexts?.trace?.trace_id;

expect(navigation1TraceId).toBeDefined();
expect(navigation2TraceId).toBeDefined();
expect(navigation1TraceId).not.toEqual(navigation2TraceId);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [Sentry.browserTracingIntegration()],
tracesSampleRate: 1,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';
import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';

sentryTest('should create a new trace on each navigation', async ({ getLocalTestPath, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

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

await getFirstSentryEnvelopeRequest<Event>(page, url);
const navigationEvent1 = await getFirstSentryEnvelopeRequest<Event>(page, `${url}#foo`);
const navigationEvent2 = await getFirstSentryEnvelopeRequest<Event>(page, `${url}#bar`);

expect(navigationEvent1.contexts?.trace?.op).toBe('navigation');
expect(navigationEvent2.contexts?.trace?.op).toBe('navigation');

const navigation1TraceId = navigationEvent1.contexts?.trace?.trace_id;
const navigation2TraceId = navigationEvent2.contexts?.trace?.trace_id;

expect(navigation1TraceId).toMatch(/^[0-9a-f]{32}$/);
expect(navigation2TraceId).toMatch(/^[0-9a-f]{32}$/);
expect(navigation1TraceId).not.toEqual(navigation2TraceId);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';
import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';

sentryTest(
'should create a new trace for a navigation after the initial pageload',
async ({ getLocalTestPath, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

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

const pageloadEvent = await getFirstSentryEnvelopeRequest<Event>(page, url);
const navigationEvent1 = await getFirstSentryEnvelopeRequest<Event>(page, `${url}#foo`);

expect(pageloadEvent.contexts?.trace?.op).toBe('pageload');
expect(navigationEvent1.contexts?.trace?.op).toBe('navigation');

const pageloadTraceId = pageloadEvent.contexts?.trace?.trace_id;
const navigation1TraceId = navigationEvent1.contexts?.trace?.trace_id;

expect(pageloadTraceId).toMatch(/^[0-9a-f]{32}$/);
expect(navigation1TraceId).toMatch(/^[0-9a-f]{32}$/);
expect(pageloadTraceId).not.toEqual(navigation1TraceId);
},
);

sentryTest('error after pageload has pageload traceId', async ({ getLocalTestPath, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

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

const pageloadEvent = await getFirstSentryEnvelopeRequest<Event>(page, url);
expect(pageloadEvent.contexts?.trace?.op).toBe('pageload');

const pageloadTraceId = pageloadEvent.contexts?.trace?.trace_id;
expect(pageloadTraceId).toMatch(/^[0-9a-f]{32}$/);

const [, errorEvent] = await Promise.all([
page.locator('#errorBtn').click(),
getFirstSentryEnvelopeRequest<Event>(page),
]);

const errorTraceId = errorEvent.contexts?.trace?.trace_id;
expect(errorTraceId).toBe(pageloadTraceId);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
const errorBtn = document.getElementById('errorBtn');
errorBtn.addEventListener('click', () => {
throw new Error('Sentry Test Error');
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
</head>
<body>
<button id="errorBtn">Throw Error</button>
</body>
</html>
8 changes: 3 additions & 5 deletions packages/browser/src/tracing/browserTracingIntegration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -265,10 +265,11 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
// This is a bit hacky, because we want to get the span to use both the correct scope _and_ the correct propagation context
// but afterwards, we want to reset it to avoid this also applying to other spans
const scope = getCurrentScope();
const propagationContextBefore = scope.getPropagationContext();

activeSpan = continueTrace({ sentryTrace, baggage }, () => {
// We update the outer current scope to have the correct propagation context...
// We update the outer current scope to have the correct propagation context
// this means, the scope active when the pageload span is created will continue to hold the
// propagationContext from the incoming trace, even after the pageload span ended.
scope.setPropagationContext(getCurrentScope().getPropagationContext());

// Ensure we are on the original current scope again, so the span is set as active on it
Expand All @@ -279,9 +280,6 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
});
});
});

// Reset this back to what it was before
scope.setPropagationContext(propagationContextBefore);
});

if (options.instrumentPageLoad && WINDOW.location) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -710,9 +710,9 @@ describe('browserTracingIntegration', () => {
expect(dynamicSamplingContext).toBeDefined();
expect(dynamicSamplingContext).toStrictEqual({ release: '2.1.14' });

// Propagation context is reset and does not contain the meta tag data
expect(propagationContext.traceId).not.toEqual('12312012123120121231201212312012');
expect(propagationContext.parentSpanId).not.toEqual('1121201211212012');
// Propagation context keeps the meta tag trace data for later events on the same route to add them to the trace
expect(propagationContext.traceId).toEqual('12312012123120121231201212312012');
expect(propagationContext.parentSpanId).toEqual('1121201211212012');
});

it('puts frozen Dynamic Sampling Context on pageload span if sentry-trace data and only 3rd party baggage is present', () => {
Expand Down Expand Up @@ -747,9 +747,9 @@ describe('browserTracingIntegration', () => {
expect(dynamicSamplingContext).toBeDefined();
expect(dynamicSamplingContext).toStrictEqual({});

// Propagation context is reset and does not contain the meta tag data
expect(propagationContext.traceId).not.toEqual('12312012123120121231201212312012');
expect(propagationContext.parentSpanId).not.toEqual('1121201211212012');
// Propagation context keeps the meta tag trace data for later events on the same route to add them to the trace
expect(propagationContext.traceId).toEqual('12312012123120121231201212312012');
expect(propagationContext.parentSpanId).toEqual('1121201211212012');
});

it('ignores the meta tag data for navigation spans', () => {
Expand Down Expand Up @@ -843,9 +843,9 @@ describe('browserTracingIntegration', () => {
expect(dynamicSamplingContext).toBeDefined();
expect(dynamicSamplingContext).toStrictEqual({ release: '2.2.14' });

// Propagation context is reset and does not contain the meta tag data
expect(propagationContext.traceId).not.toEqual('12312012123120121231201212312012');
expect(propagationContext.parentSpanId).not.toEqual('1121201211212012');
// Propagation context keeps the custom trace data for later events on the same route to add them to the trace
expect(propagationContext.traceId).toEqual('12312012123120121231201212312011');
expect(propagationContext.parentSpanId).toEqual('1121201211212011');
});
});

Expand Down