add tests, adjust end time trimming

Lms24 · Lms24 · commit 9ac071e90082 · 2025-09-19T09:22:52.000+02:00
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/default/init.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/default/init.js
@@ -0,0 +1,15 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+window._testBaseTimestamp = performance.timeOrigin / 1000;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [Sentry.browserTracingIntegration({ explicitPageloadEnd: true })],
+  tracesSampleRate: 1,
+  debug: true,
+});
+
+setTimeout(() => {
+  Sentry.reportPageLoaded();
+}, 2500);
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/default/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/default/test.ts
@@ -0,0 +1,42 @@
+import { expect } from '@playwright/test';
+import {
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
+  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
+  SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
+  SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
+} from '@sentry/browser';
+import { sentryTest } from '../../../../../utils/fixtures';
+import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
+
+sentryTest(
+  'waits for Sentry.reportPageLoaded() to be called when `explicitPageloadEnd` is true',
+  async ({ getLocalTestUrl, page }) => {
+    if (shouldSkipTracingTest()) {
+      sentryTest.skip();
+    }
+
+    const pageloadEventPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');
+
+    const url = await getLocalTestUrl({ testDir: __dirname });
+
+    await page.goto(url);
+
+    const eventData = envelopeRequestParser(await pageloadEventPromise);
+
+    const traceContextData = eventData.contexts?.trace?.data;
+    const spanDurationSeconds = eventData.timestamp! - eventData.start_timestamp!;
+
+    expect(traceContextData).toMatchObject({
+      [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
+      [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
+      [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
+      [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
+      ['sentry.idle_span_finish_reason']: 'reportPageLoaded',
+    });
+
+    // We wait for 2.5 seconds before calling Sentry.reportPageLoaded()
+    // the margins are to account for timing weirdness in CI to avoid flakes
+    expect(spanDurationSeconds).toBeGreaterThan(2);
+    expect(spanDurationSeconds).toBeLessThan(3);
+  },
+);
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/finalTimeout/init.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/finalTimeout/init.js
@@ -0,0 +1,13 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+window._testBaseTimestamp = performance.timeOrigin / 1000;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [Sentry.browserTracingIntegration({ explicitPageloadEnd: true, finalTimeout: 3000 })],
+  tracesSampleRate: 1,
+  debug: true,
+});
+
+// not calling Sentry.reportPageLoaded() on purpose!
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/finalTimeout/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/finalTimeout/test.ts
@@ -0,0 +1,42 @@
+import { expect } from '@playwright/test';
+import {
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
+  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
+  SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
+  SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
+} from '@sentry/browser';
+import { sentryTest } from '../../../../../utils/fixtures';
+import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
+
+sentryTest(
+  'final timeout cancels the pageload span even if `explicitPageloadEnd` is true',
+  async ({ getLocalTestUrl, page }) => {
+    if (shouldSkipTracingTest()) {
+      sentryTest.skip();
+    }
+
+    const pageloadEventPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');
+
+    const url = await getLocalTestUrl({ testDir: __dirname });
+
+    await page.goto(url);
+
+    const eventData = envelopeRequestParser(await pageloadEventPromise);
+
+    const traceContextData = eventData.contexts?.trace?.data;
+    const spanDurationSeconds = eventData.timestamp! - eventData.start_timestamp!;
+
+    expect(traceContextData).toMatchObject({
+      [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
+      [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
+      [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
+      [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
+      ['sentry.idle_span_finish_reason']: 'finalTimeout',
+    });
+
+    // We wait for 3 seconds before calling Sentry.reportPageLoaded()
+    // the margins are to account for timing weirdness in CI to avoid flakes
+    expect(spanDurationSeconds).toBeGreaterThan(2.5);
+    expect(spanDurationSeconds).toBeLessThan(3.5);
+  },
+);
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/navigation/init.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/navigation/init.js
@@ -0,0 +1,19 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+window._testBaseTimestamp = performance.timeOrigin / 1000;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [Sentry.browserTracingIntegration({ explicitPageloadEnd: true, instrumentNavigation: false })],
+  tracesSampleRate: 1,
+  debug: true,
+});
+
+setTimeout(() => {
+  Sentry.startBrowserTracingNavigationSpan(Sentry.getClient(), { name: 'custom_navigation' });
+}, 1000);
+
+setTimeout(() => {
+  Sentry.reportPageLoaded();
+}, 2500);
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/reportPageLoaded/navigation/test.ts
@@ -0,0 +1,40 @@
+import { expect } from '@playwright/test';
+import {
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
+  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
+  SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
+  SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
+} from '@sentry/browser';
+import { sentryTest } from '../../../../../utils/fixtures';
+import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
+
+sentryTest(
+  'starting a navigation span cancels the pageload span even if `explicitPageloadEnd` is true',
+  async ({ getLocalTestUrl, page }) => {
+    if (shouldSkipTracingTest()) {
+      sentryTest.skip();
+    }
+
+    const pageloadEventPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');
+
+    const url = await getLocalTestUrl({ testDir: __dirname });
+
+    await page.goto(url);
+
+    const eventData = envelopeRequestParser(await pageloadEventPromise);
+
+    const traceContextData = eventData.contexts?.trace?.data;
+    const spanDurationSeconds = eventData.timestamp! - eventData.start_timestamp!;
+
+    expect(traceContextData).toMatchObject({
+      [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
+      [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
+      [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
+      [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
+      ['sentry.idle_span_finish_reason']: 'cancelled',
+    });
+
+    // ending span after 1s but adding a margin of 0.5s to account for timing weirdness in CI to avoid flakes
+    expect(spanDurationSeconds).toBeLessThan(1.5);
+  },
+);
diff --git a/packages/browser/src/index.bundle.tracing.replay.feedback.ts b/packages/browser/src/index.bundle.tracing.replay.feedback.ts
@@ -21,6 +21,7 @@ export {
   browserTracingIntegration,
   startBrowserTracingNavigationSpan,
   startBrowserTracingPageLoadSpan,
+  reportPageLoaded,
 } from './tracing/browserTracingIntegration';
 
 export { getFeedback, sendFeedback } from '@sentry-internal/feedback';
diff --git a/packages/browser/src/index.bundle.tracing.replay.ts b/packages/browser/src/index.bundle.tracing.replay.ts
@@ -21,6 +21,7 @@ export {
   browserTracingIntegration,
   startBrowserTracingNavigationSpan,
   startBrowserTracingPageLoadSpan,
+  reportPageLoaded,
 } from './tracing/browserTracingIntegration';
 export { feedbackIntegrationShim as feedbackAsyncIntegration, feedbackIntegrationShim as feedbackIntegration };
 
diff --git a/packages/browser/src/index.bundle.tracing.ts b/packages/browser/src/index.bundle.tracing.ts
@@ -21,6 +21,7 @@ export {
   browserTracingIntegration,
   startBrowserTracingNavigationSpan,
   startBrowserTracingPageLoadSpan,
+  reportPageLoaded,
 } from './tracing/browserTracingIntegration';
 
 export {
diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts
@@ -41,6 +41,7 @@ export {
   browserTracingIntegration,
   startBrowserTracingNavigationSpan,
   startBrowserTracingPageLoadSpan,
+  reportPageLoaded,
 } from './tracing/browserTracingIntegration';
 export type { RequestInstrumentationOptions } from './tracing/request';
 export {
diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts
@@ -5,6 +5,7 @@ import {
   browserPerformanceTimeOrigin,
   dateTimestampInSeconds,
   debug,
+  defineIntegration,
   generateSpanId,
   generateTraceId,
   getClient,
@@ -328,7 +329,7 @@ const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = {
  *
  * We explicitly export the proper type here, as this has to be extended in some cases.
  */
-export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptions> = {}) => {
+export const browserTracingIntegration = ((options: Partial<BrowserTracingOptions> = {}) => {
   const latestRoute: RouteInfo = {
     name: undefined,
     source: undefined,
@@ -367,7 +368,7 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
     onRequestSpanStart,
   } = {
     ...DEFAULT_BROWSER_TRACING_OPTIONS,
-    ..._options,
+    ...options,
   };
 
   let _collectWebVitals: undefined | (() => void);
@@ -437,6 +438,7 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
           dsc: getDynamicSamplingContextFromSpan(span),
         });
       },
+      trimIdleSpanEndTimestamp: !explicitPageloadEnd,
     });
 
     if (isPageloadSpan && explicitPageloadEnd) {
@@ -599,6 +601,14 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
           ...startSpanOptions,
         });
       });
+
+      client.on('endPageloadSpan', () => {
+        if (explicitPageloadEnd && _pageloadSpan) {
+          _pageloadSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON, 'reportPageLoaded');
+          _pageloadSpan.end();
+          _pageloadSpan = undefined;
+        }
+      });
     },
 
     afterAllSetup(client) {
@@ -681,13 +691,6 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
         onRequestSpanStart,
       });
     },
-
-    endPageloadSpan() {
-      if (_pageloadSpan && explicitPageloadEnd) {
-        _pageloadSpan.end();
-        _pageloadSpan = undefined;
-      }
-    },
   };
 }) satisfies IntegrationFn;
 
@@ -768,11 +771,7 @@ export function getMetaContent(metaName: string): string | undefined {
 export function reportPageLoaded(client?: Client): void {
   const clientToUse = client ?? getClient();
   if (clientToUse) {
-    const browserTracing =
-      clientToUse.getIntegrationByName<ReturnType<typeof browserTracingIntegration>>('BrowserTracing');
-    if (browserTracing) {
-      browserTracing.endPageloadSpan();
-    }
+    clientToUse.emit('endPageloadSpan');
   }
 }
 
diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts
@@ -606,6 +606,8 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
     ) => void,
   ): () => void;
 
+  public on(hook: 'endPageloadSpan', callback: () => void): () => void;
+
   /**
    * A hook for the browser tracing integrations to trigger after the pageload span was started.
    * @returns {() => void} A function that, when executed, removes the registered callback.
@@ -800,6 +802,11 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
     traceOptions?: { sentryTrace?: string | undefined; baggage?: string | undefined },
   ): void;
 
+  /**
+   * Emit a hook event for browser tracing integrations to trigger the end of a page load span.
+   */
+  public emit(hook: 'endPageloadSpan'): void;
+
   /**
    * Emit a hook event for browser tracing integrations to trigger aafter the pageload span was started.
    */
diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts
@@ -75,8 +75,16 @@ interface IdleSpanOptions {
    * Defaults to `false`.
    */
   disableAutoFinish?: boolean;
+
   /** Allows to configure a hook that is called when the idle span is ended, before it is processed. */
   beforeSpanEnd?: (span: Span) => void;
+
+  /**
+   * If set to `true`, the idle span will be trimmed to the latest span end timestamp of its children.
+   *
+   * @default `true`.
+   */
+  trimIdleSpanEndTimestamp?: boolean;
 }
 
 /**
@@ -108,6 +116,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
     finalTimeout = TRACING_DEFAULTS.finalTimeout,
     childSpanTimeout = TRACING_DEFAULTS.childSpanTimeout,
     beforeSpanEnd,
+    trimIdleSpanEndTimestamp = true,
   } = options;
 
   const client = getClient();
@@ -151,8 +160,11 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
       // Ensure we end with the last span timestamp, if possible
       const spans = getSpanDescendants(span).filter(child => child !== span);
 
+      const spanJson = spanToJSON(span);
+
       // If we have no spans, we just end, nothing else to do here
-      if (!spans.length) {
+      // Likewise, if users explicitly ended the span, we simply end the span without timestamp adjustment
+      if (!spans.length || !trimIdleSpanEndTimestamp) {
         onIdleSpanEnded(spanEndTimestamp);
         return Reflect.apply(target, thisArg, [spanEndTimestamp, ...rest]);
       }
@@ -174,7 +186,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
       }, undefined);
 
       // In reality this should always exist here, but type-wise it may be undefined...
-      const spanStartTimestamp = spanToJSON(span).start_timestamp;
+      const spanStartTimestamp = spanJson.start_timestamp;
 
       // The final endTimestamp should:
       // * Never be before the span start timestamp
