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

Remove the now incorrect propagationContext resetting logic for
pageload spans and add tests to cover this scenario.

Author: Lms24

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

@@ -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);
-});

dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js

@@ -0,0 +1,9 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [Sentry.browserTracingIntegration()],
+  tracesSampleRate: 1,
+});

dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts

@@ -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);
+});

dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts

@@ -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);
+});

dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/subject.js

@@ -0,0 +1,4 @@
+const errorBtn = document.getElementById('errorBtn');
+errorBtn.addEventListener('click', () => {
+  throw new Error('Sentry Test Error');
+});

dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/template.html

@@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+  </head>
+  <body>
+    <button id="errorBtn">Throw Error</button>
+  </body>
+</html>

packages/browser/src/tracing/browserTracingIntegration.ts

@@ -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
@@ -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) {

packages/browser/test/unit/tracing/browserTracingIntegration.test.ts

@@ -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', () => {
@@ -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', () => {
@@ -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');
     });
   });