fix(tracing): Pass tracePropagationTargets to `instrumentOutgoingRe…

…quests` (#6259)

Fix a bug in our `BrowserTracing` integration which caused the new `tracePropagationTargets` option not to be passed to `instrumentOutgoingRequests` where it was needed to decide if our tracing headers should be attached to outgoing requests or not. 

Because we never passed this value to the instrumentation function, custom-defined `tracePropagationTargets` values were not respected by the SDK and headers were attached to requests whose URLs matched the default targets or custom specified `tracingOrigins`. 

With this fix, we also make a change how we internally handle the co-existance between the deprecated `tracingOrigins` and `tracePropagationTargets` options. We now simply overwrite the default `tracePropagationTargets` values with custom `tracingOrigins` (if available and no custom `tracePropagationTargets` were set). This enables us to internally only rely on `tracePropagationTargets`. 
Note that we still have to keep setting `tracingOrigins` to `browserTracing.options`, as removing this field or changing the type would break users. This field however is not used internally anymore.

This patch also adds a bunch of unit and integration tests to make sure, `tracePropagationTargets` works as expected this time. Also, the tests check that `tracingOrigins` is still respected properly.

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTargets/init.js

@@ -0,0 +1,10 @@
+import * as Sentry from '@sentry/browser';
+import { Integrations } from '@sentry/tracing';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [new Integrations.BrowserTracing({ tracePropagationTargets: ['http://example.com'] })],
+  tracesSampleRate: 1,
+});

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTargets/subject.js

@@ -0,0 +1 @@
+fetch('http://example.com/0').then(fetch('http://example.com/1').then(fetch('http://example.com/2')));

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTargets/test.ts

@@ -0,0 +1,27 @@
+import { expect, Request } from '@playwright/test';
+
+import { sentryTest } from '../../../../../utils/fixtures';
+
+sentryTest(
+  'should attach `sentry-trace` and `baggage` header to request matching tracePropagationTargets',
+  async ({ getLocalTestPath, page }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    const requests = (
+      await Promise.all([
+        page.goto(url),
+        Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://example.com/${idx}`))),
+      ])
+    )[1];
+
+    expect(requests).toHaveLength(3);
+
+    requests?.forEach(async (request: Request) => {
+      const requestHeaders = await request.allHeaders();
+      expect(requestHeaders).toMatchObject({
+        'sentry-trace': expect.any(String),
+        baggage: expect.any(String),
+      });
+    });
+  },
+);

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTargetsAndOrigins/init.js

@@ -0,0 +1,12 @@
+import * as Sentry from '@sentry/browser';
+import { Integrations } from '@sentry/tracing';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [
+    new Integrations.BrowserTracing({ tracePropagationTargets: [], tracingOrigins: ['http://example.com'] }),
+  ],
+  tracesSampleRate: 1,
+});

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTargetsAndOrigins/subject.js

@@ -0,0 +1 @@
+fetch('http://example.com/0').then(fetch('http://example.com/1').then(fetch('http://example.com/2')));

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTargetsAndOrigins/test.ts

@@ -0,0 +1,27 @@
+import { expect, Request } from '@playwright/test';
+
+import { sentryTest } from '../../../../../utils/fixtures';
+
+sentryTest(
+  '[pre-v8] should prefer custom tracePropagationTargets over tracingOrigins',
+  async ({ getLocalTestPath, page }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    const requests = (
+      await Promise.all([
+        page.goto(url),
+        Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://example.com/${idx}`))),
+      ])
+    )[1];
+
+    expect(requests).toHaveLength(3);
+
+    requests?.forEach(async (request: Request) => {
+      const requestHeaders = await request.allHeaders();
+      expect(requestHeaders).not.toMatchObject({
+        'sentry-trace': expect.any(String),
+        baggage: expect.any(String),
+      });
+    });
+  },
+);

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTracingOrigins/init.js

@@ -0,0 +1,10 @@
+import * as Sentry from '@sentry/browser';
+import { Integrations } from '@sentry/tracing';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [new Integrations.BrowserTracing({ tracingOrigins: ['http://example.com'] })],
+  tracesSampleRate: 1,
+});

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTracingOrigins/subject.js

@@ -0,0 +1 @@
+fetch('http://example.com/0').then(fetch('http://example.com/1').then(fetch('http://example.com/2')));

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTracingOrigins/test.ts

@@ -0,0 +1,27 @@
+import { expect, Request } from '@playwright/test';
+
+import { sentryTest } from '../../../../../utils/fixtures';
+
+sentryTest(
+  '[pre-v8] should attach `sentry-trace` and `baggage` header to request matching tracingOrigins',
+  async ({ getLocalTestPath, page }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    const requests = (
+      await Promise.all([
+        page.goto(url),
+        Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://example.com/${idx}`))),
+      ])
+    )[1];
+
+    expect(requests).toHaveLength(3);
+
+    requests?.forEach(async (request: Request) => {
+      const requestHeaders = await request.allHeaders();
+      expect(requestHeaders).toMatchObject({
+        'sentry-trace': expect.any(String),
+        baggage: expect.any(String),
+      });
+    });
+  },
+);

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/init.js

@@ -0,0 +1,10 @@
+import * as Sentry from '@sentry/browser';
+import { Integrations } from '@sentry/tracing';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [new Integrations.BrowserTracing()],
+  tracesSampleRate: 1,
+});

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/subject.js

@@ -0,0 +1 @@
+fetch('http://localhost:4200/0').then(fetch('http://localhost:4200/1').then(fetch('http://localhost:4200/2')));

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/test.ts

@@ -0,0 +1,27 @@
+import { expect, Request } from '@playwright/test';
+
+import { sentryTest } from '../../../../../utils/fixtures';
+
+sentryTest(
+  'should attach `sentry-trace` and `baggage` header to request matching default tracePropagationTargets',
+  async ({ getLocalTestPath, page }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    const requests = (
+      await Promise.all([
+        page.goto(url),
+        Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://localhost:4200/${idx}`))),
+      ])
+    )[1];
+
+    expect(requests).toHaveLength(3);
+
+    requests?.forEach(async (request: Request) => {
+      const requestHeaders = await request.allHeaders();
+      expect(requestHeaders).toMatchObject({
+        'sentry-trace': expect.any(String),
+        baggage: expect.any(String),
+      });
+    });
+  },
+);

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/init.js

@@ -0,0 +1,10 @@
+import * as Sentry from '@sentry/browser';
+import { Integrations } from '@sentry/tracing';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [new Integrations.BrowserTracing()],
+  tracesSampleRate: 1,
+});

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/subject.js

@@ -0,0 +1 @@
+fetch('http://example.com/0').then(fetch('http://example.com/1').then(fetch('http://example.com/2')));

packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/test.ts

@@ -0,0 +1,27 @@
+import { expect, Request } from '@playwright/test';
+
+import { sentryTest } from '../../../../../utils/fixtures';
+
+sentryTest(
+  'should not attach `sentry-trace` and `baggage` header to request not matching default tracePropagationTargets',
+  async ({ getLocalTestPath, page }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    const requests = (
+      await Promise.all([
+        page.goto(url),
+        Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://example.com/${idx}`))),
+      ])
+    )[1];
+
+    expect(requests).toHaveLength(3);
+
+    requests?.forEach(async (request: Request) => {
+      const requestHeaders = await request.allHeaders();
+      expect(requestHeaders).not.toMatchObject({
+        'sentry-trace': expect.any(String),
+        baggage: expect.any(String),
+      });
+    });
+  },
+);

packages/tracing/src/browser/browsertracing.ts

@@ -112,7 +112,7 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions {
   ): void;
 }
 
-const DEFAULT_BROWSER_TRACING_OPTIONS = {
+const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = {
   idleTimeout: DEFAULT_IDLE_TIMEOUT,
   finalTimeout: DEFAULT_FINAL_TIMEOUT,
   heartbeatInterval: DEFAULT_HEARTBEAT_INTERVAL,
@@ -153,6 +153,15 @@ export class BrowserTracing implements Integration {
       ..._options,
     };
 
+    // TODO (v8): remove this block after tracingOrigins is removed
+    // Set tracePropagationTargets to tracingOrigins if specified by the user
+    // In case both are specified, tracePropagationTargets takes precedence
+    // eslint-disable-next-line deprecation/deprecation
+    if (_options && !_options.tracePropagationTargets && _options.tracingOrigins) {
+      // eslint-disable-next-line deprecation/deprecation
+      this.options.tracePropagationTargets = _options.tracingOrigins;
+    }
+
     const { _metricOptions } = this.options;
     startTrackingWebVitals(_metricOptions && _metricOptions._reportAllChanges);
     if (this.options._experiments?.enableLongTask) {
@@ -174,8 +183,7 @@ export class BrowserTracing implements Integration {
       markBackgroundTransactions,
       traceFetch,
       traceXHR,
-      // eslint-disable-next-line deprecation/deprecation
-      tracingOrigins,
+      tracePropagationTargets,
       shouldCreateSpanForRequest,
     } = this.options;
 
@@ -189,7 +197,12 @@ export class BrowserTracing implements Integration {
       registerBackgroundTabDetection();
     }
 
-    instrumentOutgoingRequests({ traceFetch, traceXHR, tracingOrigins, shouldCreateSpanForRequest });
+    instrumentOutgoingRequests({
+      traceFetch,
+      traceXHR,
+      tracePropagationTargets,
+      shouldCreateSpanForRequest,
+    });
   }
 
   /** Create routing idle transaction. */

packages/tracing/src/browser/request.ts

@@ -113,7 +113,7 @@ export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions
 /** Registers span creators for xhr and fetch requests  */
 export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumentationOptions>): void {
   // eslint-disable-next-line deprecation/deprecation
-  const { traceFetch, traceXHR, tracingOrigins, tracePropagationTargets, shouldCreateSpanForRequest } = {
+  const { traceFetch, traceXHR, tracePropagationTargets, tracingOrigins, shouldCreateSpanForRequest } = {
     traceFetch: defaultRequestInstrumentationOptions.traceFetch,
     traceXHR: defaultRequestInstrumentationOptions.traceXHR,
     ..._options,
@@ -122,8 +122,11 @@ export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumenta
   const shouldCreateSpan =
     typeof shouldCreateSpanForRequest === 'function' ? shouldCreateSpanForRequest : (_: string) => true;
 
+  // TODO(v8) Remove tracingOrigins here
+  // The only reason we're passing it in here is because this instrumentOutgoingRequests function is publicly exported
+  // and we don't want to break the API. We can remove it in v8.
   const shouldAttachHeadersWithTargets = (url: string): boolean =>
-    shouldAttachHeaders(url, tracingOrigins, tracePropagationTargets);
+    shouldAttachHeaders(url, tracePropagationTargets || tracingOrigins);
 
   const spans: Record<string, Span> = {};
 
@@ -144,20 +147,9 @@ export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumenta
  * A function that determines whether to attach tracing headers to a request.
  * This was extracted from `instrumentOutgoingRequests` to make it easier to test shouldAttachHeaders.
  * We only export this fuction for testing purposes.
- *
- * TODO (v8): Remove `tracingOrigins` which should drastically simplify this function.
  */
-export function shouldAttachHeaders(
-  url: string,
-  tracePropagationTargets: (string | RegExp)[] | undefined,
-  tracingOrigins: (string | RegExp)[] | undefined,
-): boolean {
-  // TODO (v8): Replace the entire code below with this one-liner:
-  // return stringMatchesSomePattern(url, tracePropagationTargets || DEFAULT_TRACE_PROPAGATION_TARGETS);
-  if (tracePropagationTargets || tracingOrigins) {
-    return stringMatchesSomePattern(url, tracePropagationTargets || tracingOrigins);
-  }
-  return stringMatchesSomePattern(url, DEFAULT_TRACE_PROPAGATION_TARGETS);
+export function shouldAttachHeaders(url: string, tracePropagationTargets: (string | RegExp)[] | undefined): boolean {
+  return stringMatchesSomePattern(url, tracePropagationTargets || DEFAULT_TRACE_PROPAGATION_TARGETS);
 }
 
 /**

packages/tracing/test/browser/browsertracing.test.ts

@@ -34,6 +34,15 @@ jest.mock('@sentry/utils', () => {
 
 jest.mock('../../src/browser/metrics');
 
+const instrumentOutgoingRequestsMock = jest.fn();
+jest.mock('./../../src/browser/request', () => {
+  const actual = jest.requireActual('./../../src/browser/request');
+  return {
+    ...actual,
+    instrumentOutgoingRequests: (options: Partial<BrowserTracingOptions>) => instrumentOutgoingRequestsMock(options),
+  };
+});
+
 beforeAll(() => {
   const dom = new JSDOM();
   // @ts-ignore need to override global document
@@ -128,6 +137,7 @@ describe('BrowserTracing', () => {
       expect(transaction.endTimestamp).toBe(span.endTimestamp);
     });
 
+    // TODO (v8): remove these tests
     describe('tracingOrigins', () => {
       it('sets tracing origins if provided and does not warn', () => {
         const sampleTracingOrigins = ['something'];
@@ -152,6 +162,43 @@ describe('BrowserTracing', () => {
       });
     });
 
+    describe('tracePropagationTargets', () => {
+      it('sets tracePropagationTargets if provided', () => {
+        const sampleTracePropagationTargets = ['something'];
+        const inst = createBrowserTracing(true, {
+          routingInstrumentation: customInstrumentRouting,
+          tracePropagationTargets: sampleTracePropagationTargets,
+        });
+
+        expect(inst.options.tracePropagationTargets).toEqual(sampleTracePropagationTargets);
+      });
+
+      it('sets tracePropagationTargets to an empty array and does not warn', () => {
+        const sampleTracePropagationTargets: string[] = [];
+        const inst = createBrowserTracing(true, {
+          routingInstrumentation: customInstrumentRouting,
+          tracePropagationTargets: sampleTracePropagationTargets,
+        });
+
+        expect(inst.options.tracePropagationTargets).toEqual(sampleTracePropagationTargets);
+      });
+
+      it('correctly passes tracePropagationTargets to `instrumentOutgoingRequests` in `setupOnce`', () => {
+        jest.clearAllMocks();
+        const sampleTracePropagationTargets = ['something'];
+        createBrowserTracing(true, {
+          routingInstrumentation: customInstrumentRouting,
+          tracePropagationTargets: sampleTracePropagationTargets,
+        });
+
+        expect(instrumentOutgoingRequestsMock).toHaveBeenCalledWith({
+          traceFetch: true,
+          traceXHR: true,
+          tracePropagationTargets: ['something'],
+        });
+      });
+    });
+
     describe('beforeNavigate', () => {
       it('is called on transaction creation', () => {
         const mockBeforeNavigation = jest.fn().mockReturnValue({ name: 'here/is/my/path' });