fix(browser): Ensure pageload & navigation spans have correct data (#16279)

This PR fixes two things about our idle spans emitted from
`browserTracingIntegration`:

1. The navigation name was sometimes incorrect. This is because we look
at `window.location.pathname` at the time when the `popstate` event is
emitted - but at this point, this may not be updated yet. So a
`navigation` transaction would possibly have the pathname of the
previous page as transaction name.
2. The request data is also possibly incorrect - this is set by
`HttpContext` integration at event processing time. However, at this
time the `window.location` data is also usually already of the following
navigation, so the pageload would often have wrong request data
associated to it. Now, we store this on the current scope at span
creation time to ensure it is actually correct.

Author: mydea

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),

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: {

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');

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

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()) {
@@ -31,6 +36,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 +54,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;
@@ -69,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',
+  });
+});

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

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<T extends WrappableFunction, NonFunction>(
 
   return sentryWrapped;
 }
+
+/**
+ * Get HTTP request data from the current page.
+ */
+export function getHttpRequestData(): { url: string; headers: Record<string, string> } {
+  // 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;
+}

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

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,
@@ -33,7 +34,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';
@@ -399,7 +400,14 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
         maybeEndActiveSpan();
 
         getIsolationScope().setPropagationContext({ traceId: generateTraceId(), sampleRand: Math.random() });
-        getCurrentScope().setPropagationContext({ traceId: generateTraceId(), sampleRand: Math.random() });
+
+        const scope = getCurrentScope();
+        scope.setPropagationContext({ traceId: generateTraceId(), sampleRand: Math.random() });
+        // We reset this to ensure we do not have lingering incorrect data here
+        // places that call this hook may set this where appropriate - else, the URL at span sending time is used
+        scope.setSDKProcessingMetadata({
+          normalizedRequest: undefined,
+        });
 
         _createRouteSpan(client, {
           op: 'navigation',
@@ -417,7 +425,15 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
         const baggage = traceOptions.baggage || getMetaContent('baggage');
 
         const propagationContext = propagationContextFromHeaders(sentryTrace, baggage);
-        getCurrentScope().setPropagationContext(propagationContext);
+
+        const scope = getCurrentScope();
+        scope.setPropagationContext(propagationContext);
+
+        // 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
+        scope.setSDKProcessingMetadata({
+          normalizedRequest: getHttpRequestData(),
+        });
 
         _createRouteSpan(client, {
           op: 'pageload',
@@ -459,16 +475,25 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
               return;
             }
 
-            if (from !== to) {
-              startingUrl = undefined;
-              startBrowserTracingNavigationSpan(client, {
-                name: WINDOW.location.pathname,
-                attributes: {
-                  [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
-                  [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser',
-                },
-              });
-            }
+            startingUrl = undefined;
+            const parsed = parseStringToURLObject(to);
+            startBrowserTracingNavigationSpan(client, {
+              name: parsed?.pathname || 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,
+              },
+            });
           });
         }
       }

packages/core/src/tracing/sentrySpan.ts

@@ -336,6 +336,8 @@ export class SentrySpan implements Span {
 
     const { scope: capturedSpanScope, isolationScope: capturedSpanIsolationScope } = getCapturedScopesOnSpan(this);
 
+    const normalizedRequest = capturedSpanScope?.getScopeData().sdkProcessingMetadata?.normalizedRequest;
+
     if (this._sampled !== true) {
       return undefined;
     }
@@ -374,6 +376,7 @@ export class SentrySpan implements Span {
         capturedSpanIsolationScope,
         dynamicSamplingContext: getDynamicSamplingContextFromSpan(this),
       },
+      request: normalizedRequest,
       ...(source && {
         transaction_info: {
           source,

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