fix(tracing): Make shouldAttachHeaders not fall back to default values

Author: Lms24

packages/tracing/src/browser/request.ts

@@ -21,15 +21,15 @@ export interface RequestInstrumentationOptions {
    * Use `shouldCreateSpanForRequest` to control span creation and `tracePropagationTargets` to control
    * trace header attachment.
    */
-  tracingOrigins: Array<string | RegExp>;
+  tracingOrigins?: Array<string | RegExp>;
 
   /**
    * List of strings and/or regexes used to determine which outgoing requests will have `sentry-trace` and `baggage`
    * headers attached.
    *
    * Default: ['localhost', /^\//] {@see DEFAULT_TRACE_PROPAGATION_TARGETS}
    */
-  tracePropagationTargets: Array<string | RegExp>;
+  tracePropagationTargets?: Array<string | RegExp>;
 
   /**
    * Flag to disable patching all together for fetch requests.
@@ -107,8 +107,6 @@ type PolymorphicRequestHeaders =
 export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions = {
   traceFetch: true,
   traceXHR: true,
-  tracingOrigins: DEFAULT_TRACING_ORIGINS,
-  tracePropagationTargets: DEFAULT_TRACE_PROPAGATION_TARGETS,
 };
 
 /** Registers span creators for xhr and fetch requests  */
@@ -122,8 +120,7 @@ export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumenta
   const shouldCreateSpan =
     typeof shouldCreateSpanForRequest === 'function' ? shouldCreateSpanForRequest : (_: string) => true;
 
-  const shouldAttachHeaders = (url: string): boolean =>
-    stringMatchesSomePattern(url, tracingOrigins) || stringMatchesSomePattern(url, tracePropagationTargets);
+  const shouldAttachHeaders = makeShouldAttachHeaders(tracingOrigins, tracePropagationTargets);
 
   const spans: Record<string, Span> = {};
 
@@ -140,6 +137,23 @@ export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumenta
   }
 }
 
+/**
+ * Creates a function that determines whether to attach tracing headers to a request.
+ * This was extracted from `instrumentOutgoingRequests` to make it easier to test shouldAttachHeaders.
+ * TODO (v8): Remove `tracingOrigins` which should drastically simplify this function.
+ */
+export function makeShouldAttachHeaders(
+  tracePropagationTargets: (string | RegExp)[] | undefined,
+  tracingOrigins: (string | RegExp)[] | undefined,
+) {
+  return (url: string): boolean => {
+    if (tracePropagationTargets || tracingOrigins) {
+      return stringMatchesSomePattern(url, tracePropagationTargets || tracingOrigins);
+    }
+    return stringMatchesSomePattern(url, DEFAULT_TRACE_PROPAGATION_TARGETS);
+  };
+}
+
 /**
  * Create and track fetch request spans
  */

packages/tracing/test/browser/request.test.ts

@@ -3,7 +3,14 @@ import { Hub, makeMain } from '@sentry/core';
 import * as utils from '@sentry/utils';
 
 import { Span, spanStatusfromHttpCode, Transaction } from '../../src';
-import { fetchCallback, FetchData, instrumentOutgoingRequests, xhrCallback, XHRData } from '../../src/browser/request';
+import {
+  fetchCallback,
+  FetchData,
+  instrumentOutgoingRequests,
+  makeShouldAttachHeaders,
+  xhrCallback,
+  XHRData,
+} from '../../src/browser/request';
 import { addExtensionMethods } from '../../src/hubextensions';
 import * as tracingUtils from '../../src/utils';
 import { getDefaultBrowserClientOptions } from '../testutils';
@@ -383,3 +390,52 @@ describe('callbacks', () => {
     });
   });
 });
+
+// TODO (v8): Adapt these tests once we remove `tracingOrigins`
+describe('[pre-v8]: shouldAttachHeaders', () => {
+  describe('prefer `tracePropagationTargets` over `tracingOrigins`', () => {
+    it('should return `true` if the url matches the new tracePropagationTargets', () => {
+      const shouldAttachHeaders = makeShouldAttachHeaders(['example.com'], undefined);
+      expect(shouldAttachHeaders('http://example.com')).toBe(true);
+    });
+    it('should return `false` if tracePropagationTargets array is empty', () => {
+      const shouldAttachHeaders = makeShouldAttachHeaders([], ['localhost']);
+      expect(shouldAttachHeaders('http://localhost:3000/test')).toBe(false);
+    });
+    it("should return `false` if tracePropagationTargets array doesn't match", () => {
+      const shouldAttachHeaders = makeShouldAttachHeaders(['example.com'], ['localhost']);
+      expect(shouldAttachHeaders('http://localhost:3000/test')).toBe(false);
+    });
+  });
+
+  describe('tracingOrigins backwards compatibility (tracePropagationTargets not defined)', () => {
+    it('should return `true` if the url matches tracingOrigns', () => {
+      const shouldAttachHeaders = makeShouldAttachHeaders(undefined, ['example.com']);
+      expect(shouldAttachHeaders('http://example.com')).toBe(true);
+    });
+    it('should return `false` if tracePropagationTargets array is empty', () => {
+      const shouldAttachHeaders = makeShouldAttachHeaders(undefined, []);
+      expect(shouldAttachHeaders('http://localhost:3000/test')).toBe(false);
+    });
+    it("should return `false` if tracePropagationTargets array doesn't match", () => {
+      const shouldAttachHeaders = makeShouldAttachHeaders(undefined, ['example.com']);
+      expect(shouldAttachHeaders('http://localhost:3000/test')).toBe(false);
+    });
+  });
+
+  describe('should fall back to defaults if no options are specified', () => {
+    it.each([
+      '/api/test',
+      'http://localhost:3000/test',
+      'http://somewhere.com/test/localhost/123',
+      'http://somewhere.com/test?url=localhost:3000&test=123',
+    ])('return `true` for urls matching defaults (%s)', url => {
+      const shouldAttachHeaders = makeShouldAttachHeaders(undefined, undefined);
+      expect(shouldAttachHeaders(url)).toBe(true);
+    });
+    it.each(['notmydoman/api/test', 'example.com'])('return `false` for urls not matching defaults (%s)', url => {
+      const shouldAttachHeaders = makeShouldAttachHeaders(undefined, undefined);
+      expect(shouldAttachHeaders(url)).toBe(false);
+    });
+  });
+});