From 4583556bf5c4845920a221ae40bd5436692f1106 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 15 Apr 2024 12:26:42 +0200 Subject: [PATCH 1/3] fix(browser): Ensure pageload trace remains active after pageload span finished --- .../navigation/test.ts | 22 -------- .../suites/tracing/trace-lifetime/init.js | 9 ++++ .../tracing/trace-lifetime/navigation/test.ts | 26 ++++++++++ .../tracing/trace-lifetime/pageload/test.ts | 50 +++++++++++++++++++ .../suites/tracing/trace-lifetime/subject.js | 4 ++ .../tracing/trace-lifetime/template.html | 9 ++++ .../src/tracing/browserTracingIntegration.ts | 8 ++- 7 files changed, 101 insertions(+), 27 deletions(-) create mode 100644 dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts create mode 100644 dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts create mode 100644 dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/template.html diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation/test.ts index f7dc01ca7f54..5a46a65a4392 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation/test.ts +++ b/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(page, url); - const navigationEvent1 = await getFirstSentryEnvelopeRequest(page, `${url}#foo`); - const navigationEvent2 = await getFirstSentryEnvelopeRequest(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); -}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js new file mode 100644 index 000000000000..83076460599f --- /dev/null +++ b/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, +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts new file mode 100644 index 000000000000..cb8dc17812c7 --- /dev/null +++ b/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(page, url); + const navigationEvent1 = await getFirstSentryEnvelopeRequest(page, `${url}#foo`); + const navigationEvent2 = await getFirstSentryEnvelopeRequest(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); +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts new file mode 100644 index 000000000000..c0130dbc887c --- /dev/null +++ b/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(page, url); + const navigationEvent1 = await getFirstSentryEnvelopeRequest(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 should have pageload traceId', async ({ getLocalTestPath, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestPath({ testDir: __dirname }); + + const pageloadEvent = await getFirstSentryEnvelopeRequest(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(page), + ]); + + const errorTraceId = errorEvent.contexts?.trace?.trace_id; + expect(errorTraceId).toBe(pageloadTraceId); +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/subject.js b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/subject.js new file mode 100644 index 000000000000..5131ea7631e9 --- /dev/null +++ b/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'); +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/template.html b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/template.html new file mode 100644 index 000000000000..a29ad2056a45 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/template.html @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index d1b4a90d6f97..16e715b15ad2 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -265,10 +265,11 @@ export const browserTracingIntegration = ((_options: Partial { - // 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 Date: Mon, 15 Apr 2024 12:49:24 +0200 Subject: [PATCH 2/3] slight rewording --- .../suites/tracing/trace-lifetime/pageload/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts index c0130dbc887c..16659f013dd0 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts @@ -27,7 +27,7 @@ sentryTest( }, ); -sentryTest('error after pageload should have pageload traceId', async ({ getLocalTestPath, page }) => { +sentryTest('error after pageload has pageload traceId', async ({ getLocalTestPath, page }) => { if (shouldSkipTracingTest()) { sentryTest.skip(); } From 0da0434b11ea088f039124d1790f1f7e5600b350 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 15 Apr 2024 14:27:29 +0200 Subject: [PATCH 3/3] adapt unit tests --- .../tracing/browserTracingIntegration.test.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/browser/test/unit/tracing/browserTracingIntegration.test.ts b/packages/browser/test/unit/tracing/browserTracingIntegration.test.ts index 30bf265cf20e..7fde92ab764e 100644 --- a/packages/browser/test/unit/tracing/browserTracingIntegration.test.ts +++ b/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'); }); });