From 63e43e6c2cf74ba3817c0d607d446b9ff1c49081 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 21 Nov 2022 12:57:28 +0100 Subject: [PATCH] fix(tracing): Make `shouldAttachHeaders` not fall back to default values (#6238) This patch fixes a bug in our `shouldAttachHeaders` function that determines internally if outgoing requests should have headers attached or not. Previously, we assigned the default values to `tracingOrigins` as well as `tracePropagationTargets` and overwrote these defaults, if users provided one (or (unlikely) both) options. In `shouldAttachHeaders`, we then first tested `tracingOrigins` for a match and in case there was no match, we'd test `tracePropagationTargets`. This, however, also meant that if users specified `tracingOrigins` and an URL didn't match them, there was still a chance that headers would be attached - in case the url matched the `tracePropagationTargets`' default values. With this fix, we don't assign default values to either option but check for definedness of them and only if they're both undefined, well fall back to defaults. ### Example: url: `/api/test/123` tracingOrigins: `[]` tracePropTargets: `['localhost', /^\//]` // default values `shouldAttachHeaders` returned `true` but should have returned `false`. This PR fixes this behaviour. In case, both options are defined, we prefer `tracePropagationTargets` over `tracingOrigins`, as defined in the [dev specification](https://develop.sentry.dev/sdk/performance/#example). Furthermore, this patch extracts the `shouldAttachHeaders` function by wrapping it in a factory function so that it's easier to test this behaviour. And it adds some tests to verify correct behaviour. We can get rid of some code around this in v8 when we finally remove `tracingOrigins`. Side-note: Because we're exporting `defaultRequestInstrumentationOptions` as public API, I decided to leave the optional value assignment in place but to simply not take the default values from it anymore in `instrumentOutgoingRequests` (see [aed40a6](https://github.com/getsentry/sentry-javascript/pull/6238/commits/aed40a61dcc4a96416e084a6a8cfb588576b389b)). --- packages/tracing/src/browser/request.ts | 36 +++++++++--- packages/tracing/test/browser/request.test.ts | 55 ++++++++++++++++++- 2 files changed, 82 insertions(+), 9 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index c3ebc1e00240..dce0d95044d5 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 */ @@ -107,7 +105,8 @@ type PolymorphicRequestHeaders = export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions = { traceFetch: true, traceXHR: true, - tracingOrigins: DEFAULT_TRACING_ORIGINS, + // TODO (v8): Remove this property + tracingOrigins: DEFAULT_TRACE_PROPAGATION_TARGETS, tracePropagationTargets: DEFAULT_TRACE_PROPAGATION_TARGETS, }; @@ -115,31 +114,52 @@ export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions 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, }; const shouldCreateSpan = typeof shouldCreateSpanForRequest === 'function' ? shouldCreateSpanForRequest : (_: string) => true; - const shouldAttachHeaders = (url: string): boolean => - stringMatchesSomePattern(url, tracingOrigins) || stringMatchesSomePattern(url, 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); }); } } +/** + * 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); +} + /** * 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..4e41e22a9a51 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, + shouldAttachHeaders, + xhrCallback, + XHRData, +} from '../../src/browser/request'; import { addExtensionMethods } from '../../src/hubextensions'; import * as tracingUtils from '../../src/utils'; import { getDefaultBrowserClientOptions } from '../testutils'; @@ -383,3 +390,49 @@ 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', () => { + expect(shouldAttachHeaders('http://example.com', ['example.com'], undefined)).toBe(true); + }); + + it('should return `false` if tracePropagationTargets array is empty', () => { + expect(shouldAttachHeaders('http://localhost:3000/test', [], ['localhost'])).toBe(false); + }); + + it("should return `false` if tracePropagationTargets array doesn't match", () => { + 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', () => { + expect(shouldAttachHeaders('http://example.com', undefined, ['example.com'])).toBe(true); + }); + + it('should return `false` if tracePropagationTargets array is empty', () => { + expect(shouldAttachHeaders('http://localhost:3000/test', undefined, [])).toBe(false); + }); + + it("should return `false` if tracePropagationTargets array doesn't match", () => { + expect(shouldAttachHeaders('http://localhost:3000/test', undefined, ['example.com'])).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 => { + expect(shouldAttachHeaders(url, undefined, undefined)).toBe(true); + }); + + it.each(['notmydoman/api/test', 'example.com'])('return `false` for urls not matching defaults (%s)', url => { + expect(shouldAttachHeaders(url, undefined, undefined)).toBe(false); + }); + }); +});