From c5de662f68f2f52697fca54ef363c0a70c46e831 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 18 Nov 2022 16:33:24 +0100 Subject: [PATCH 1/4] fix(tracing): Make `shouldAttachHeaders` not fall back to default values --- packages/tracing/src/browser/request.ts | 26 +++++++-- packages/tracing/test/browser/request.test.ts | 58 ++++++++++++++++++- 2 files changed, 77 insertions(+), 7 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index c3ebc1e00240..6434f16fe9a1 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -21,7 +21,7 @@ export interface RequestInstrumentationOptions { * Use `shouldCreateSpanForRequest` to control span creation and `tracePropagationTargets` to control * trace header attachment. */ - tracingOrigins: Array; + tracingOrigins?: Array; /** * List of strings and/or regexes used to determine which outgoing requests will have `sentry-trace` and `baggage` @@ -29,7 +29,7 @@ export interface RequestInstrumentationOptions { * * Default: ['localhost', /^\//] {@see DEFAULT_TRACE_PROPAGATION_TARGETS} */ - tracePropagationTargets: Array; + tracePropagationTargets?: Array; /** * 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 true; - const shouldAttachHeaders = (url: string): boolean => - stringMatchesSomePattern(url, tracingOrigins) || stringMatchesSomePattern(url, tracePropagationTargets); + const shouldAttachHeaders = makeShouldAttachHeaders(tracingOrigins, tracePropagationTargets); const spans: Record = {}; @@ -140,6 +137,23 @@ export function instrumentOutgoingRequests(_options?: Partial { + if (tracePropagationTargets || tracingOrigins) { + return stringMatchesSomePattern(url, tracePropagationTargets || tracingOrigins); + } + return stringMatchesSomePattern(url, DEFAULT_TRACE_PROPAGATION_TARGETS); + }; +} + /** * Create and track fetch request spans */ diff --git a/packages/tracing/test/browser/request.test.ts b/packages/tracing/test/browser/request.test.ts index b69d9b1e5664..fade81f4a48e 100644 --- a/packages/tracing/test/browser/request.test.ts +++ b/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); + }); + }); +}); From 3dd0cce62a1df8b385b7613d2497c868102b60c8 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 21 Nov 2022 10:41:49 +0100 Subject: [PATCH 2/4] apply review suggestions --- packages/tracing/src/browser/request.ts | 5 +++-- packages/tracing/test/browser/request.test.ts | 5 +++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 6434f16fe9a1..173fb5c42f71 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -10,8 +10,6 @@ import { import { getActiveTransaction, hasTracingEnabled } from '../utils'; -// TODO (v8): Remove `tracingOrigins` -export const DEFAULT_TRACING_ORIGINS = ['localhost', /^\//]; export const DEFAULT_TRACE_PROPAGATION_TARGETS = ['localhost', /^\//]; /** Options for Request Instrumentation */ @@ -140,6 +138,7 @@ export function instrumentOutgoingRequests(_options?: Partial { + // TODO (v8): Replace the code below with this one-liner: + // return stringMatchesSomePattern(url, tracePropagationTargets || DEFAULT_TRACE_PROPAGATION_TARGETS); if (tracePropagationTargets || tracingOrigins) { return stringMatchesSomePattern(url, tracePropagationTargets || tracingOrigins); } diff --git a/packages/tracing/test/browser/request.test.ts b/packages/tracing/test/browser/request.test.ts index fade81f4a48e..43e6eead129b 100644 --- a/packages/tracing/test/browser/request.test.ts +++ b/packages/tracing/test/browser/request.test.ts @@ -398,10 +398,12 @@ describe('[pre-v8]: shouldAttachHeaders', () => { 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); @@ -413,10 +415,12 @@ describe('[pre-v8]: shouldAttachHeaders', () => { 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); @@ -433,6 +437,7 @@ describe('[pre-v8]: shouldAttachHeaders', () => { 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); From aed40a61dcc4a96416e084a6a8cfb588576b389b Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 21 Nov 2022 11:47:08 +0100 Subject: [PATCH 3/4] avoid breaking change --- packages/tracing/src/browser/request.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 173fb5c42f71..84db6f5d8d4d 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -19,7 +19,7 @@ export interface RequestInstrumentationOptions { * Use `shouldCreateSpanForRequest` to control span creation and `tracePropagationTargets` to control * trace header attachment. */ - tracingOrigins?: Array; + tracingOrigins: Array; /** * List of strings and/or regexes used to determine which outgoing requests will have `sentry-trace` and `baggage` @@ -27,7 +27,7 @@ export interface RequestInstrumentationOptions { * * Default: ['localhost', /^\//] {@see DEFAULT_TRACE_PROPAGATION_TARGETS} */ - tracePropagationTargets?: Array; + tracePropagationTargets: Array; /** * Flag to disable patching all together for fetch requests. @@ -105,13 +105,17 @@ type PolymorphicRequestHeaders = export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions = { traceFetch: true, traceXHR: true, + // TODO (v8): Remove this property + tracingOrigins: DEFAULT_TRACE_PROPAGATION_TARGETS, + tracePropagationTargets: DEFAULT_TRACE_PROPAGATION_TARGETS, }; /** Registers span creators for xhr and fetch requests */ export function instrumentOutgoingRequests(_options?: Partial): void { // eslint-disable-next-line deprecation/deprecation const { traceFetch, traceXHR, tracingOrigins, tracePropagationTargets, shouldCreateSpanForRequest } = { - ...defaultRequestInstrumentationOptions, + traceFetch: defaultRequestInstrumentationOptions.traceFetch, + traceXHR: defaultRequestInstrumentationOptions.traceXHR, ..._options, }; From d981da001ff8af82c12c249e50ba6450e12689d1 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 21 Nov 2022 11:58:42 +0100 Subject: [PATCH 4/4] inline factory function --- packages/tracing/src/browser/request.ts | 29 ++++++++++--------- packages/tracing/test/browser/request.test.ts | 26 ++++++----------- 2 files changed, 24 insertions(+), 31 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 84db6f5d8d4d..dce0d95044d5 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -122,41 +122,42 @@ export function instrumentOutgoingRequests(_options?: Partial true; - const shouldAttachHeaders = makeShouldAttachHeaders(tracingOrigins, tracePropagationTargets); + const shouldAttachHeadersWithTargets = (url: string): boolean => + shouldAttachHeaders(url, tracingOrigins, tracePropagationTargets); const spans: Record = {}; if (traceFetch) { addInstrumentationHandler('fetch', (handlerData: FetchData) => { - fetchCallback(handlerData, shouldCreateSpan, shouldAttachHeaders, spans); + fetchCallback(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans); }); } if (traceXHR) { addInstrumentationHandler('xhr', (handlerData: XHRData) => { - xhrCallback(handlerData, shouldCreateSpan, shouldAttachHeaders, spans); + xhrCallback(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans); }); } } /** - * Creates a function that determines whether to attach tracing headers to a request. + * 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 makeShouldAttachHeaders( +export function shouldAttachHeaders( + url: string, tracePropagationTargets: (string | RegExp)[] | undefined, tracingOrigins: (string | RegExp)[] | undefined, -) { - return (url: string): boolean => { - // TODO (v8): Replace the 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); - }; +): 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); } /** diff --git a/packages/tracing/test/browser/request.test.ts b/packages/tracing/test/browser/request.test.ts index 43e6eead129b..4e41e22a9a51 100644 --- a/packages/tracing/test/browser/request.test.ts +++ b/packages/tracing/test/browser/request.test.ts @@ -7,7 +7,7 @@ import { fetchCallback, FetchData, instrumentOutgoingRequests, - makeShouldAttachHeaders, + shouldAttachHeaders, xhrCallback, XHRData, } from '../../src/browser/request'; @@ -395,35 +395,29 @@ describe('callbacks', () => { 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); + expect(shouldAttachHeaders('http://example.com', ['example.com'], undefined)).toBe(true); }); it('should return `false` if tracePropagationTargets array is empty', () => { - const shouldAttachHeaders = makeShouldAttachHeaders([], ['localhost']); - expect(shouldAttachHeaders('http://localhost:3000/test')).toBe(false); + expect(shouldAttachHeaders('http://localhost:3000/test', [], ['localhost'])).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); + expect(shouldAttachHeaders('http://localhost:3000/test', ['example.com'], ['localhost'])).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); + expect(shouldAttachHeaders('http://example.com', undefined, ['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); + expect(shouldAttachHeaders('http://localhost:3000/test', undefined, [])).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); + expect(shouldAttachHeaders('http://localhost:3000/test', undefined, ['example.com'])).toBe(false); }); }); @@ -434,13 +428,11 @@ describe('[pre-v8]: shouldAttachHeaders', () => { '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); + expect(shouldAttachHeaders(url, undefined, undefined)).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); + expect(shouldAttachHeaders(url, undefined, undefined)).toBe(false); }); }); });