fix(browser): Improve navigation vs. redirect detection

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-aborting-pageload/init.js

@@ -4,7 +4,7 @@ window.Sentry = Sentry;
 
 Sentry.init({
   dsn: 'https://[email protected]/1337',
-  integrations: [Sentry.browserTracingIntegration({ idleTimeout: 2000 })],
+  integrations: [Sentry.browserTracingIntegration({ idleTimeout: 2000, detectRedirects: false })],
   tracesSampleRate: 1,
 });
 

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/multiple-redirects/init.js

@@ -0,0 +1,36 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  integrations: [Sentry.browserTracingIntegration()],
+  tracesSampleRate: 1,
+});
+
+window.history.pushState({}, '', '/sub-page-redirect-1');
+
+setTimeout(() => {
+  window.history.pushState({}, '', '/sub-page-redirect-2');
+}, 400);
+
+setTimeout(() => {
+  window.history.pushState({}, '', '/sub-page-redirect-3');
+}, 800);
+
+document.getElementById('btn1').addEventListener('click', () => {
+  window.history.pushState({}, '', '/next-page');
+});
+
+setTimeout(() => {
+  document.getElementById('btn1').click();
+  // 1s is still within the 1.5s time window, but the click should trigger a new navigation root span
+}, 1000);
+
+setTimeout(() => {
+  window.history.pushState({}, '', '/next-page-redirect-1');
+}, 1100);
+
+setTimeout(() => {
+  window.history.pushState({}, '', '/next-page-redirect-2');
+}, 2000);

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

@@ -0,0 +1,78 @@
+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/core';
+import { sentryTest } from '../../../../../utils/fixtures';
+import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
+
+sentryTest(
+  'creates a pageload and navigation root spans each with multiple navigation.redirect childspans',
+  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 === '/next-page',
+    );
+
+    await page.goto(url);
+
+    const pageloadRequest = envelopeRequestParser(await pageloadRequestPromise);
+    const navigationRequest = envelopeRequestParser(await navigationRequestPromise);
+
+    expect(pageloadRequest.contexts?.trace?.op).toBe('pageload');
+
+    expect(pageloadRequest.contexts?.trace?.data).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',
+    });
+
+    expect(pageloadRequest.request).toEqual({
+      headers: {
+        'User-Agent': expect.any(String),
+      },
+      url: 'http://sentry-test.io/index.html',
+    });
+
+    const spans = pageloadRequest.spans || [];
+
+    const redirectSpans = spans.filter(span => span.op === 'navigation.redirect');
+    expect(redirectSpans).toHaveLength(3);
+
+    redirectSpans.forEach(redirectSpan => {
+      expect(redirectSpan?.timestamp).toEqual(redirectSpan?.start_timestamp);
+      expect(redirectSpan).toEqual({
+        data: {
+          'sentry.op': 'navigation.redirect',
+          'sentry.origin': 'auto.navigation.browser',
+          'sentry.source': 'url',
+        },
+        description: expect.stringContaining('/sub-page-redirect-'),
+        op: 'navigation.redirect',
+        origin: 'auto.navigation.browser',
+        parent_span_id: pageloadRequest.contexts!.trace!.span_id,
+        span_id: expect.any(String),
+        start_timestamp: expect.any(Number),
+        timestamp: expect.any(Number),
+        trace_id: expect.any(String),
+      });
+    });
+
+    expect(navigationRequest.contexts?.trace?.op).toBe('navigation');
+    expect(navigationRequest.transaction).toEqual('/next-page');
+
+    // 2 subsequent redirects belonging to the navigation root span
+    expect(navigationRequest.spans?.filter(span => span.op === 'navigation.redirect')).toHaveLength(2);
+  },
+);

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/navigation->navigation-click/init.js

@@ -0,0 +1,27 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  integrations: [Sentry.browserTracingIntegration()],
+  tracesSampleRate: 1,
+  debug: true,
+});
+
+document.getElementById('btn1').addEventListener('click', () => {
+  window.history.pushState({}, '', '/sub-page');
+
+  // then trigger redirect inside of this navigation, which should not be detected as a redirect
+  // because the last click was less than 1.5s ago
+  setTimeout(() => {
+    document.getElementById('btn2').click();
+  }, 100);
+});
+
+document.getElementById('btn2').addEventListener('click', () => {
+  setTimeout(() => {
+    // navigation happens ~1100ms after the last navigation
+    window.history.pushState({}, '', '/sub-page-2');
+  }, 1000);
+});

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/navigation->navigation-click/template.html

@@ -0,0 +1,8 @@
+<!doctype html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+  </head>
+  <button id="btn1">Trigger navigation</button>
+  <button id="btn2">Trigger navigation 2</button>
+</html>

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

@@ -0,0 +1,47 @@
+import { expect } from '@playwright/test';
+import { sentryTest } from '../../../../../utils/fixtures';
+import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
+
+sentryTest(
+  'creates navigation root span if click happened within 1.5s of the last navigation',
+  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 navigation2RequestPromise = waitForTransactionRequest(
+      page,
+      event => event.contexts?.trace?.op === 'navigation' && event.transaction === '/sub-page-2',
+    );
+
+    await page.goto(url);
+
+    await pageloadRequestPromise;
+
+    // Now trigger navigation (since no span is active), and then a redirect in the navigation, with
+    await page.click('#btn1');
+
+    const navigationRequest = envelopeRequestParser(await navigationRequestPromise);
+    const navigation2Request = envelopeRequestParser(await navigation2RequestPromise);
+
+    expect(navigationRequest.contexts?.trace?.op).toBe('navigation');
+    expect(navigationRequest.transaction).toEqual('/sub-page');
+
+    const spans = (navigationRequest.spans || []).filter(s => s.op === 'navigation.redirect');
+
+    expect(spans).toHaveLength(0);
+
+    expect(navigation2Request.contexts?.trace?.op).toBe('navigation');
+    expect(navigation2Request.transaction).toEqual('/sub-page-2');
+
+    const spans2 = (navigation2Request.spans || []).filter(s => s.op === 'navigation.redirect');
+    expect(spans2).toHaveLength(0);
+  },
+);

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/click-early/init.js → dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/navigation->navigation-keypress/init.js

@@ -21,3 +21,10 @@ document.getElementById('btn1').addEventListener('click', () => {
     }, 100);
   }, 400);
 });
+
+document.getElementById('btn2').addEventListener('click', () => {
+  // Trigger navigation later than click, so the last click is more than 300ms ago
+  setTimeout(() => {
+    window.history.pushState({}, '', '/sub-page-2');
+  }, 400);
+});

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/keypress-early/template.html → dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/navigation->navigation-keypress/template.html

@@ -4,4 +4,5 @@
     <meta charset="utf-8" />
   </head>
   <button id="btn1">Trigger navigation</button>
+  <button id="btn2">Trigger navigation</button>
 </html>

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

@@ -0,0 +1,54 @@
+import { expect } from '@playwright/test';
+import { sentryTest } from '../../../../../utils/fixtures';
+import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
+
+sentryTest(
+  'creates a navigation root span if a keypress happened within the last 1.5s',
+  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 navigationRequest2Promise = waitForTransactionRequest(
+      page,
+      event => event.contexts?.trace?.op === 'navigation' && event.transaction === '/sub-page-2',
+    );
+
+    await page.goto(url);
+
+    await pageloadRequestPromise;
+
+    await page.focus('#btn1');
+    await page.keyboard.press('Enter');
+
+    await page.waitForTimeout(500);
+
+    await page.focus('#btn2');
+    await page.keyboard.press('Enter');
+
+    const navigationRequest = envelopeRequestParser(await navigationRequestPromise);
+    const navigationRequest2 = envelopeRequestParser(await navigationRequest2Promise);
+
+    expect(navigationRequest.contexts?.trace?.op).toBe('navigation');
+    expect(navigationRequest.transaction).toEqual('/sub-page');
+
+    const redirectSpans = navigationRequest.spans?.filter(span => span.op === 'navigation.redirect') || [];
+    expect(redirectSpans).toHaveLength(1);
+
+    expect(redirectSpans[0].description).toEqual('/sub-page-redirect');
+
+    expect(navigationRequest2.contexts?.trace?.op).toBe('navigation');
+    expect(navigationRequest2.transaction).toEqual('/sub-page-2');
+
+    const redirectSpans2 = navigationRequest2.spans?.filter(span => span.op === 'navigation.redirect') || [];
+    expect(redirectSpans2).toHaveLength(0);
+  },
+);

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/navigation->redirect/init.js

@@ -0,0 +1,20 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  integrations: [Sentry.browserTracingIntegration()],
+  tracesSampleRate: 1,
+  debug: true,
+});
+
+document.getElementById('btn1').addEventListener('click', () => {
+  window.history.pushState({}, '', '/sub-page');
+
+  // then trigger redirect inside of this navigation, which should be detected as a redirect
+  // because the last navigation was less than 1.5s ago
+  setTimeout(() => {
+    window.history.pushState({}, '', '/sub-page-redirect');
+  }, 750);
+});

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/click-early/test.ts → dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-redirect/navigation->redirect/test.ts

@@ -3,7 +3,7 @@ import { sentryTest } from '../../../../../utils/fixtures';
 import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
 
 sentryTest(
-  'should create a navigation.redirect span if a click happened more than 300ms before navigation',
+  'creates a navigation root span and redirect child span if no click happened within the last 1.5s',
   async ({ getLocalTestUrl, page }) => {
     if (shouldSkipTracingTest()) {
       sentryTest.skip();
@@ -21,7 +21,7 @@ sentryTest(
 
     await pageloadRequestPromise;
 
-    // Now trigger navigation, and then a redirect in the navigation, with
+    // Now trigger navigation (since no span is active), and then a redirect in the navigation, with
     await page.click('#btn1');
 
     const navigationRequest = envelopeRequestParser(await navigationRequestPromise);

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

@@ -3,7 +3,7 @@ import { sentryTest } from '../../../../../utils/fixtures';
 import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../../utils/helpers';
 
 sentryTest(
-  'should not create a navigation.redirect span if `detectRedirects` is set to false',
+  "doesn't create a navigation.redirect span if `detectRedirects` is set to false",
   async ({ getLocalTestUrl, page }) => {
     if (shouldSkipTracingTest()) {
       sentryTest.skip();