From 8102ffcfe2693a97d2fdca16631a645102c7907c Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 13 May 2025 13:10:31 +0200 Subject: [PATCH 1/7] fix(browser): Ensure pageload & navigation spans have correct data --- .../navigation-aborting-pageload/init.js | 2 +- .../navigation-aborting-pageload/test.ts | 15 +++++++++++++ .../navigation/test.ts | 16 ++++++++++++++ packages/browser/src/helpers.ts | 22 +++++++++++++++++++ .../browser/src/integrations/httpcontext.ts | 20 ++++++----------- .../src/tracing/browserTracingIntegration.ts | 16 ++++++++++---- packages/core/src/tracing/sentrySpan.ts | 3 +++ 7 files changed, 76 insertions(+), 18 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-aborting-pageload/init.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-aborting-pageload/init.js index c0424a9b743f..8fb188a75278 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-aborting-pageload/init.js +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-aborting-pageload/init.js @@ -9,4 +9,4 @@ Sentry.init({ }); // Immediately navigate to a new page to abort the pageload -window.location.href = '#foo'; +window.history.pushState({}, '', '/sub-page'); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-aborting-pageload/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-aborting-pageload/test.ts index ad224aa6d1d9..b68d1903a0db 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-aborting-pageload/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-aborting-pageload/test.ts @@ -40,6 +40,9 @@ sentryTest( expect(navigationTraceId).toBeDefined(); expect(pageloadTraceId).not.toEqual(navigationTraceId); + expect(pageloadRequest.transaction).toEqual('/index.html'); + expect(navigationRequest.transaction).toEqual('/sub-page'); + expect(pageloadRequest.contexts?.trace?.data).toMatchObject({ [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser', [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1, @@ -54,5 +57,17 @@ sentryTest( [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', ['sentry.idle_span_finish_reason']: 'idleTimeout', }); + expect(pageloadRequest.request).toEqual({ + headers: { + 'User-Agent': expect.any(String), + }, + url: 'http://sentry-test.io/index.html', + }); + expect(navigationRequest.request).toEqual({ + headers: { + 'User-Agent': expect.any(String), + }, + url: 'http://sentry-test.io/sub-page', + }); }, ); 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 503aa73ba4ff..f9ad14a4c9dc 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 @@ -31,6 +31,10 @@ sentryTest('should create a navigation transaction on page navigation', async ({ expect(navigationTraceId).toBeDefined(); expect(pageloadTraceId).not.toEqual(navigationTraceId); + expect(pageloadRequest.transaction).toEqual('/index.html'); + // Fragment is not in transaction name + expect(navigationRequest.transaction).toEqual('/index.html'); + expect(pageloadRequest.contexts?.trace?.data).toMatchObject({ [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser', [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1, @@ -45,6 +49,18 @@ sentryTest('should create a navigation transaction on page navigation', async ({ [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', ['sentry.idle_span_finish_reason']: 'idleTimeout', }); + expect(pageloadRequest.request).toEqual({ + headers: { + 'User-Agent': expect.any(String), + }, + url: 'http://sentry-test.io/index.html', + }); + expect(navigationRequest.request).toEqual({ + headers: { + 'User-Agent': expect.any(String), + }, + url: 'http://sentry-test.io/index.html#foo', + }); const pageloadSpans = pageloadRequest.spans; const navigationSpans = navigationRequest.spans; diff --git a/packages/browser/src/helpers.ts b/packages/browser/src/helpers.ts index 76578fe356dc..8fe8d650f322 100644 --- a/packages/browser/src/helpers.ts +++ b/packages/browser/src/helpers.ts @@ -4,6 +4,7 @@ import { addExceptionTypeValue, addNonEnumerableProperty, captureException, + getLocationHref, getOriginalFunction, GLOBAL_OBJ, markFunctionWrapped, @@ -175,3 +176,24 @@ export function wrap( return sentryWrapped; } + +/** + * Get HTTP request data from the current page. + */ +export function getHttpRequestData(): { url: string; headers: Record } { + // grab as much info as exists and add it to the event + const url = getLocationHref(); + const { referrer } = WINDOW.document || {}; + const { userAgent } = WINDOW.navigator || {}; + + const headers = { + ...(referrer && { Referer: referrer }), + ...(userAgent && { 'User-Agent': userAgent }), + }; + const request = { + url, + headers, + }; + + return request; +} diff --git a/packages/browser/src/integrations/httpcontext.ts b/packages/browser/src/integrations/httpcontext.ts index 78e27713c78f..9517b2364e83 100644 --- a/packages/browser/src/integrations/httpcontext.ts +++ b/packages/browser/src/integrations/httpcontext.ts @@ -1,5 +1,5 @@ -import { defineIntegration, getLocationHref } from '@sentry/core'; -import { WINDOW } from '../helpers'; +import { defineIntegration } from '@sentry/core'; +import { getHttpRequestData, WINDOW } from '../helpers'; /** * Collects information about HTTP request headers and @@ -14,23 +14,17 @@ export const httpContextIntegration = defineIntegration(() => { return; } - // grab as much info as exists and add it to the event - const url = event.request?.url || getLocationHref(); - const { referrer } = WINDOW.document || {}; - const { userAgent } = WINDOW.navigator || {}; - + const reqData = getHttpRequestData(); const headers = { + ...reqData.headers, ...event.request?.headers, - ...(referrer && { Referer: referrer }), - ...(userAgent && { 'User-Agent': userAgent }), }; - const request = { + + event.request = { + ...reqData, ...event.request, - ...(url && { url }), headers, }; - - event.request = request; }, }; }); diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index 643b561af583..edfe7a8e617a 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -33,7 +33,7 @@ import { startTrackingWebVitals, } from '@sentry-internal/browser-utils'; import { DEBUG_BUILD } from '../debug-build'; -import { WINDOW } from '../helpers'; +import { getHttpRequestData, WINDOW } from '../helpers'; import { registerBackgroundTabDetection } from './backgroundtab'; import { linkTraces } from './linkedTraces'; import { defaultRequestInstrumentationOptions, instrumentOutgoingRequests } from './request'; @@ -360,6 +360,12 @@ export const browserTracingIntegration = ((_options: Partial { startBrowserTracingNavigationSpan(client, { name: WINDOW.location.pathname, attributes: { @@ -468,7 +476,7 @@ export const browserTracingIntegration = ((_options: Partial Date: Wed, 14 May 2025 09:51:48 +0200 Subject: [PATCH 2/7] fix tests --- .../test/tracing/browserTracingIntegration.test.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/browser/test/tracing/browserTracingIntegration.test.ts b/packages/browser/test/tracing/browserTracingIntegration.test.ts index 728bee5fd1dd..35e3acd68c2c 100644 --- a/packages/browser/test/tracing/browserTracingIntegration.test.ts +++ b/packages/browser/test/tracing/browserTracingIntegration.test.ts @@ -152,7 +152,7 @@ describe('browserTracingIntegration', () => { expect(spanIsSampled(span!)).toBe(false); }); - it('starts navigation when URL changes', () => { + it('starts navigation when URL changes', async () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ tracesSampleRate: 1, @@ -187,6 +187,9 @@ describe('browserTracingIntegration', () => { WINDOW.history.pushState({}, '', '/test'); + // wait a tick to ensure everything settled + await new Promise(resolve => setTimeout(resolve, 1)); + expect(span!.isRecording()).toBe(false); const span2 = getActiveSpan(); @@ -225,6 +228,9 @@ describe('browserTracingIntegration', () => { WINDOW.history.pushState({}, '', '/test2'); + // wait a tick to ensure everything settled + await new Promise(resolve => setTimeout(resolve, 1)); + expect(span2!.isRecording()).toBe(false); const span3 = getActiveSpan(); @@ -861,7 +867,7 @@ describe('browserTracingIntegration', () => { expect(propagationContext.parentSpanId).toEqual('1121201211212012'); }); - it('ignores the meta tag data for navigation spans', () => { + it('ignores the meta tag data for navigation spans', async () => { document.head.innerHTML = '' + ''; @@ -883,6 +889,9 @@ describe('browserTracingIntegration', () => { WINDOW.history.pushState({}, '', '/navigation-test'); + // wait a tick to ensure everything settled + await new Promise(resolve => setTimeout(resolve, 1)); + const idleSpan = getActiveSpan()!; expect(idleSpan).toBeDefined(); From 694883d5a11ce766d484c6e78d96611c8f2b07f6 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 14 May 2025 09:57:52 +0200 Subject: [PATCH 3/7] reset it afterwards --- packages/browser/src/tracing/browserTracingIntegration.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index edfe7a8e617a..40ccd1357c5b 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -355,6 +355,10 @@ export const browserTracingIntegration = ((_options: Partial Date: Wed, 14 May 2025 12:30:23 +0200 Subject: [PATCH 4/7] fix by using `to` from history instrumentation --- .../browser-utils/src/instrument/history.ts | 20 ++++++- .../src/tracing/browserTracingIntegration.ts | 55 ++++++++++++------- packages/core/src/types-hoist/instrument.ts | 2 + 3 files changed, 54 insertions(+), 23 deletions(-) diff --git a/packages/browser-utils/src/instrument/history.ts b/packages/browser-utils/src/instrument/history.ts index 60ee888aae24..76bf43f7b398 100644 --- a/packages/browser-utils/src/instrument/history.ts +++ b/packages/browser-utils/src/instrument/history.ts @@ -47,9 +47,15 @@ export function instrumentHistory(): void { return function (this: History, ...args: unknown[]): void { const url = args.length > 2 ? args[2] : undefined; if (url) { - // coerce to string (this is what pushState does) const from = lastHref; - const to = String(url); + + // Ensure the URL is absolute + // this can be either a path, then it is relative to the current origin + // or a full URL of the current origin - other origins are not allowed + // See: https://developer.mozilla.org/en-US/docs/Web/API/History/pushState#url + // coerce to string (this is what pushState does) + const to = getAbsoluteUrl(String(url)); + // keep track of the current URL state, as we always receive only the updated state lastHref = to; @@ -67,3 +73,13 @@ export function instrumentHistory(): void { fill(WINDOW.history, 'pushState', historyReplacementFunction); fill(WINDOW.history, 'replaceState', historyReplacementFunction); } + +function getAbsoluteUrl(urlOrPath: string): string { + try { + const url = new URL(urlOrPath, WINDOW.location.origin); + return url.toString(); + } catch { + // fallback, just do nothing + return urlOrPath; + } +} diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index 40ccd1357c5b..0a4579f40774 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -12,6 +12,7 @@ import { getLocationHref, GLOBAL_OBJ, logger, + parseStringToURLObject, propagationContextFromHeaders, registerSpanErrorInstrumentation, SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON, @@ -355,21 +356,11 @@ export const browserTracingIntegration = ((_options: Partial { - startBrowserTracingNavigationSpan(client, { - name: WINDOW.location.pathname, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser', - }, - }); + // We store the normalized request data on the scope, so we get the request data at time of span creation + // otherwise, the URL etc. may already be of the following navigation, and we'd report the wrong URL + getCurrentScope().setSDKProcessingMetadata({ + normalizedRequest: { + ...getHttpRequestData(), + // Ensure to set this, so this matches the target route even if the URL has not yet been updated + url: to, + }, }); }); } diff --git a/packages/core/src/types-hoist/instrument.ts b/packages/core/src/types-hoist/instrument.ts index 5eba6066432a..b067f6618558 100644 --- a/packages/core/src/types-hoist/instrument.ts +++ b/packages/core/src/types-hoist/instrument.ts @@ -78,7 +78,9 @@ export interface HandlerDataConsole { } export interface HandlerDataHistory { + /** The full URL of the previous page */ from: string | undefined; + /** The full URL of the new page */ to: string; } From dcd931138530a1d238e4a00989e7b7f94b444db0 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 14 May 2025 12:51:25 +0200 Subject: [PATCH 5/7] fix tests & add test for absolute url handling --- .../Breadcrumbs/history/navigation/test.ts | 4 +- .../navigation/test.ts | 69 ++++++++++++++++++- 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/integrations/Breadcrumbs/history/navigation/test.ts b/dev-packages/browser-integration-tests/suites/integrations/Breadcrumbs/history/navigation/test.ts index c03dedd417bd..1eb7f55b60cd 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/Breadcrumbs/history/navigation/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/Breadcrumbs/history/navigation/test.ts @@ -29,14 +29,14 @@ sentryTest('should record history changes as navigation breadcrumbs', async ({ g category: 'navigation', data: { from: '/bar?a=1#fragment', - to: '[object Object]', + to: '/[object%20Object]', }, timestamp: expect.any(Number), }, { category: 'navigation', data: { - from: '[object Object]', + from: '/[object%20Object]', to: '/bar?a=1#fragment', }, timestamp: expect.any(Number), 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 f9ad14a4c9dc..cd80a2e3fa8e 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 @@ -7,7 +7,12 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, } from '@sentry/core'; import { sentryTest } from '../../../../utils/fixtures'; -import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers'; +import { + envelopeRequestParser, + getFirstSentryEnvelopeRequest, + shouldSkipTracingTest, + waitForTransactionRequest, +} from '../../../../utils/helpers'; sentryTest('should create a navigation transaction on page navigation', async ({ getLocalTestUrl, page }) => { if (shouldSkipTracingTest()) { @@ -85,3 +90,65 @@ sentryTest('should create a navigation transaction on page navigation', async ({ expect(pageloadSpanId).not.toEqual(navigationSpanId); }); + +// +sentryTest('should handle pushState with full URL', 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' && event.transaction === '/sub-page', + ); + const navigationRequestPromise2 = waitForTransactionRequest( + page, + event => event.contexts?.trace?.op === 'navigation' && event.transaction === '/sub-page-2', + ); + + await page.goto(url); + await pageloadRequestPromise; + + await page.evaluate("window.history.pushState({}, '', `${window.location.origin}/sub-page`);"); + + const navigationRequest = envelopeRequestParser(await navigationRequestPromise); + + expect(navigationRequest.transaction).toEqual('/sub-page'); + + expect(navigationRequest.contexts?.trace?.data).toMatchObject({ + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser', + [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1, + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + ['sentry.idle_span_finish_reason']: 'idleTimeout', + }); + expect(navigationRequest.request).toEqual({ + headers: { + 'User-Agent': expect.any(String), + }, + url: 'http://sentry-test.io/sub-page', + }); + + await page.evaluate("window.history.pushState({}, '', `${window.location.origin}/sub-page-2`);"); + + const navigationRequest2 = envelopeRequestParser(await navigationRequestPromise2); + + expect(navigationRequest2.transaction).toEqual('/sub-page-2'); + + expect(navigationRequest2.contexts?.trace?.data).toMatchObject({ + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser', + [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1, + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + ['sentry.idle_span_finish_reason']: 'idleTimeout', + }); + expect(navigationRequest2.request).toEqual({ + headers: { + 'User-Agent': expect.any(String), + }, + url: 'http://sentry-test.io/sub-page-2', + }); +}); From d12215d48b37adf1f85bba37b6cdf4f35f6f502c Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 14 May 2025 12:53:27 +0200 Subject: [PATCH 6/7] Revert "fix tests" This reverts commit 5cd662aef78238210ed31afde6cd90575142215e. --- .../test/tracing/browserTracingIntegration.test.ts | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/packages/browser/test/tracing/browserTracingIntegration.test.ts b/packages/browser/test/tracing/browserTracingIntegration.test.ts index 35e3acd68c2c..728bee5fd1dd 100644 --- a/packages/browser/test/tracing/browserTracingIntegration.test.ts +++ b/packages/browser/test/tracing/browserTracingIntegration.test.ts @@ -152,7 +152,7 @@ describe('browserTracingIntegration', () => { expect(spanIsSampled(span!)).toBe(false); }); - it('starts navigation when URL changes', async () => { + it('starts navigation when URL changes', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ tracesSampleRate: 1, @@ -187,9 +187,6 @@ describe('browserTracingIntegration', () => { WINDOW.history.pushState({}, '', '/test'); - // wait a tick to ensure everything settled - await new Promise(resolve => setTimeout(resolve, 1)); - expect(span!.isRecording()).toBe(false); const span2 = getActiveSpan(); @@ -228,9 +225,6 @@ describe('browserTracingIntegration', () => { WINDOW.history.pushState({}, '', '/test2'); - // wait a tick to ensure everything settled - await new Promise(resolve => setTimeout(resolve, 1)); - expect(span2!.isRecording()).toBe(false); const span3 = getActiveSpan(); @@ -867,7 +861,7 @@ describe('browserTracingIntegration', () => { expect(propagationContext.parentSpanId).toEqual('1121201211212012'); }); - it('ignores the meta tag data for navigation spans', async () => { + it('ignores the meta tag data for navigation spans', () => { document.head.innerHTML = '' + ''; @@ -889,9 +883,6 @@ describe('browserTracingIntegration', () => { WINDOW.history.pushState({}, '', '/navigation-test'); - // wait a tick to ensure everything settled - await new Promise(resolve => setTimeout(resolve, 1)); - const idleSpan = getActiveSpan()!; expect(idleSpan).toBeDefined(); From 6139d0801cc00c5b19502e4b5d5100adbf40f8bc Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 14 May 2025 13:11:41 +0200 Subject: [PATCH 7/7] fix replay test --- .../suites/replay/multiple-pages/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/multiple-pages/test.ts b/dev-packages/browser-integration-tests/suites/replay/multiple-pages/test.ts index ac046c74d337..2c059bb226f4 100644 --- a/dev-packages/browser-integration-tests/suites/replay/multiple-pages/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/multiple-pages/test.ts @@ -210,7 +210,7 @@ sentryTest( expect(replayEvent6).toEqual( getExpectedReplayEvent({ segment_id: 6, - urls: ['/spa'], + urls: [`${TEST_HOST}/spa`], request: { url: `${TEST_HOST}/spa`, headers: {