From fecdc3e1552ef9cbd0092943f837804ba9c23716 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 28 Oct 2022 00:18:24 -0700 Subject: [PATCH] fix(tracing): Fix `tracingOrigins` not applying (#6079) In the process of working on https://github.com/getsentry/sentry-javascript/issues/5285, we missed the fact that the first two PRs (https://github.com/getsentry/sentry-javascript/pull/6039 and https://github.com/getsentry/sentry-javascript/pull/6041) were interdependent, in that the former accidentally introduced a bug (https://github.com/getsentry/sentry-javascript/issues/6077) which the latter then inadvertently fixed. This would have been fine, except that we published a release after merging the bug-creating PR but before merging the bug-fixing PR. Whoops. This patch pulls just the bug-fixing part out of the second PR. It also adds tests to cover the buggy cases, using `it.each` to cover all of the different combinations of outcomes for `shouldCreateSpanForRequest` and `shouldAttachHeaders`. Finally, since I was already in the test file, I reorganized it a little: - `it('does not create span if shouldCreateSpan returns false')` -> absorbed into the `it.each()` - `it('does not create span if there is no fetch data in handler data')` -> added header check, became `it('adds neither fetch request spans nor fetch request headers if there is no fetch data in handler data')` - `it('does not add fetch request spans if tracing is disabled')` and `it('does not add fetch request headers if tracing is disabled` -> combined into `it('adds neither fetch request spans nor fetch request headers if tracing is disabled')` - `it('adds sentry-trace header to fetch requests')` -> absorbed into the `it.each()` - Similar changes made to XHR tests Co-authored-by: Tim Fish --- packages/tracing/src/browser/request.ts | 30 ++- packages/tracing/test/browser/request.test.ts | 235 +++++++++++------- 2 files changed, 161 insertions(+), 104 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 567128d743ab..d463a1353acc 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -5,6 +5,7 @@ import { BAGGAGE_HEADER_NAME, dynamicSamplingContextToSentryBaggageHeader, isInstanceOf, + isMatchingPattern, } from '@sentry/utils'; import { getActiveTransaction, hasTracingEnabled } from '../utils'; @@ -102,8 +103,7 @@ export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions /** Registers span creators for xhr and fetch requests */ export function instrumentOutgoingRequests(_options?: Partial): void { - // eslint-disable-next-line @typescript-eslint/unbound-method - const { traceFetch, traceXHR, shouldCreateSpanForRequest } = { + const { traceFetch, traceXHR, tracingOrigins, shouldCreateSpanForRequest } = { ...defaultRequestInstrumentationOptions, ..._options, }; @@ -111,17 +111,19 @@ export function instrumentOutgoingRequests(_options?: Partial true; + const shouldAttachHeaders = (url: string): boolean => tracingOrigins.some(origin => isMatchingPattern(url, origin)); + const spans: Record = {}; if (traceFetch) { addInstrumentationHandler('fetch', (handlerData: FetchData) => { - fetchCallback(handlerData, shouldCreateSpan, spans); + fetchCallback(handlerData, shouldCreateSpan, shouldAttachHeaders, spans); }); } if (traceXHR) { addInstrumentationHandler('xhr', (handlerData: XHRData) => { - xhrCallback(handlerData, shouldCreateSpan, spans); + xhrCallback(handlerData, shouldCreateSpan, shouldAttachHeaders, spans); }); } } @@ -132,6 +134,7 @@ export function instrumentOutgoingRequests(_options?: Partial boolean, + shouldAttachHeaders: (url: string) => boolean, spans: Record, ): void { if (!hasTracingEnabled() || !(handlerData.fetchData && shouldCreateSpan(handlerData.fetchData.url))) { @@ -181,14 +184,16 @@ export function fetchCallback( // eslint-disable-next-line @typescript-eslint/no-explicit-any const options: { [key: string]: any } = handlerData.args[1]; - options.headers = addTracingHeadersToFetchRequest( - request, - activeTransaction.getDynamicSamplingContext(), - span, - options, - ); + if (shouldAttachHeaders(handlerData.fetchData.url)) { + options.headers = addTracingHeadersToFetchRequest( + request, + activeTransaction.getDynamicSamplingContext(), + span, + options, + ); - activeTransaction.metadata.propagations += 1; + activeTransaction.metadata.propagations += 1; + } } } @@ -262,6 +267,7 @@ function addTracingHeadersToFetchRequest( export function xhrCallback( handlerData: XHRData, shouldCreateSpan: (url: string) => boolean, + shouldAttachHeaders: (url: string) => boolean, spans: Record, ): void { if ( @@ -307,7 +313,7 @@ export function xhrCallback( handlerData.xhr.__sentry_xhr_span_id__ = span.spanId; spans[handlerData.xhr.__sentry_xhr_span_id__] = span; - if (handlerData.xhr.setRequestHeader) { + if (handlerData.xhr.setRequestHeader && shouldAttachHeaders(handlerData.xhr.__sentry_xhr__.url)) { try { handlerData.xhr.setRequestHeader('sentry-trace', span.toTraceparent()); diff --git a/packages/tracing/test/browser/request.test.ts b/packages/tracing/test/browser/request.test.ts index 0c11a2bee515..b69d9b1e5664 100644 --- a/packages/tracing/test/browser/request.test.ts +++ b/packages/tracing/test/browser/request.test.ts @@ -3,7 +3,7 @@ import { Hub, makeMain } from '@sentry/core'; import * as utils from '@sentry/utils'; import { Span, spanStatusfromHttpCode, Transaction } from '../../src'; -import { fetchCallback, FetchData, instrumentOutgoingRequests, xhrCallback } from '../../src/browser/request'; +import { fetchCallback, FetchData, instrumentOutgoingRequests, xhrCallback, XHRData } from '../../src/browser/request'; import { addExtensionMethods } from '../../src/hubextensions'; import * as tracingUtils from '../../src/utils'; import { getDefaultBrowserClientOptions } from '../testutils'; @@ -48,29 +48,9 @@ describe('callbacks', () => { let hub: Hub; let transaction: Transaction; const alwaysCreateSpan = () => true; - const neverCreateSpan = () => false; + const alwaysAttachHeaders = () => true; const startTimestamp = 1356996072000; const endTimestamp = 1356996072000; - const fetchHandlerData: FetchData = { - args: ['http://dogs.are.great/', {}], - fetchData: { url: 'http://dogs.are.great/', method: 'GET' }, - startTimestamp, - }; - const xhrHandlerData = { - xhr: { - __sentry_xhr__: { - method: 'GET', - url: 'http://dogs.are.great/', - status_code: 200, - data: {}, - }, - __sentry_xhr_span_id__: '1231201211212012', - // eslint-disable-next-line @typescript-eslint/unbound-method - // setRequestHeader: XMLHttpRequest.prototype.setRequestHeader, - setRequestHeader, - }, - startTimestamp, - }; beforeAll(() => { const options = getDefaultBrowserClientOptions({ tracesSampleRate: 1 }); @@ -83,52 +63,91 @@ describe('callbacks', () => { hub.configureScope(scope => scope.setSpan(transaction)); }); - describe('fetchCallback()', () => { - it('does not create span if shouldCreateSpan returns false', () => { - const spans = {}; + afterEach(() => { + jest.clearAllMocks(); + }); - fetchCallback(fetchHandlerData, neverCreateSpan, spans); + describe('fetchCallback()', () => { + let fetchHandlerData: FetchData; - expect(spans).toEqual({}); + const fetchSpan = { + data: { + method: 'GET', + url: 'http://dogs.are.great/', + type: 'fetch', + }, + description: 'GET http://dogs.are.great/', + op: 'http.client', + parentSpanId: expect.any(String), + spanId: expect.any(String), + startTimestamp: expect.any(Number), + traceId: expect.any(String), + }; + + beforeEach(() => { + fetchHandlerData = { + args: ['http://dogs.are.great/', {}], + fetchData: { url: 'http://dogs.are.great/', method: 'GET' }, + startTimestamp, + }; }); - it('does not create span if there is no fetch data in handler data', () => { - const noFetchData = { args: fetchHandlerData.args, startTimestamp: fetchHandlerData.startTimestamp }; - const spans = {}; - - fetchCallback(noFetchData, alwaysCreateSpan, spans); - expect(spans).toEqual({}); - }); + it.each([ + // each case is [shouldCreateSpanReturnValue, shouldAttachHeadersReturnValue, expectedSpan, expectedHeaderKeys] + [true, true, expect.objectContaining(fetchSpan), ['sentry-trace', 'baggage']], + [true, false, expect.objectContaining(fetchSpan), []], + // If there's no span then there's no parent span id to stick into a header, so no headers, even if there's a + // `tracingOrigins` match + [false, true, undefined, []], + [false, false, undefined, []], + ])( + 'span creation/header attachment interaction - shouldCreateSpan: %s, shouldAttachHeaders: %s', + (shouldCreateSpanReturnValue, shouldAttachHeadersReturnValue, expectedSpan, expectedHeaderKeys) => { + fetchCallback( + fetchHandlerData, + () => shouldCreateSpanReturnValue, + () => shouldAttachHeadersReturnValue, + {}, + ); + + // spans[0] is the transaction itself + const newSpan = transaction.spanRecorder?.spans[1] as Span; + expect(newSpan).toEqual(expectedSpan); + + const headers = (fetchHandlerData.args[1].headers as Record) || {}; + expect(Object.keys(headers)).toEqual(expectedHeaderKeys); + }, + ); - it('does not add fetch request spans if tracing is disabled', () => { - hasTracingEnabled.mockReturnValueOnce(false); + it('adds neither fetch request spans nor fetch request headers if there is no fetch data in handler data', () => { + delete fetchHandlerData.fetchData; const spans = {}; - fetchCallback(fetchHandlerData, alwaysCreateSpan, spans); + fetchCallback(fetchHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); + expect(spans).toEqual({}); + + const headers = (fetchHandlerData.args[1].headers as Record) || {}; + expect(Object.keys(headers)).toEqual([]); }); - it('does not add fetch request headers if tracing is disabled', () => { + it('adds neither fetch request spans nor fetch request headers if tracing is disabled', () => { hasTracingEnabled.mockReturnValueOnce(false); + const spans = {}; - // make a local copy so the global one doesn't get mutated - const handlerData: FetchData = { - args: ['http://dogs.are.great/', {}], - fetchData: { url: 'http://dogs.are.great/', method: 'GET' }, - startTimestamp: 1353501072000, - }; + fetchCallback(fetchHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); - fetchCallback(handlerData, alwaysCreateSpan, {}); + expect(spans).toEqual({}); - const headers = (handlerData.args[1].headers as Record) || {}; - expect(headers['sentry-trace']).not.toBeDefined(); + const headers = (fetchHandlerData.args[1].headers as Record) || {}; + expect(Object.keys(headers)).toEqual([]); }); it('creates and finishes fetch span on active transaction', () => { const spans = {}; // triggered by request being sent - fetchCallback(fetchHandlerData, alwaysCreateSpan, spans); + fetchCallback(fetchHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); const newSpan = transaction.spanRecorder?.spans[1] as Span; @@ -149,7 +168,7 @@ describe('callbacks', () => { }; // triggered by response coming back - fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, spans); + fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); expect(newSpan.endTimestamp).toBeDefined(); }); @@ -158,7 +177,7 @@ describe('callbacks', () => { const spans: Record = {}; // triggered by request being sent - fetchCallback(fetchHandlerData, alwaysCreateSpan, spans); + fetchCallback(fetchHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); const newSpan = transaction.spanRecorder?.spans[1] as Span; @@ -171,7 +190,7 @@ describe('callbacks', () => { }; // triggered by response coming back - fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, spans); + fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); expect(newSpan.status).toBe(spanStatusfromHttpCode(404)); }); @@ -186,7 +205,7 @@ describe('callbacks', () => { }; // in that case, the response coming back will be ignored - fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, {}); + fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, alwaysAttachHeaders, {}); const newSpan = transaction.spanRecorder?.spans[1]; @@ -199,57 +218,89 @@ describe('callbacks', () => { expect(transaction.metadata.propagations).toBe(0); - fetchCallback(firstReqData, alwaysCreateSpan, {}); + fetchCallback(firstReqData, alwaysCreateSpan, alwaysAttachHeaders, {}); expect(transaction.metadata.propagations).toBe(1); - fetchCallback(secondReqData, alwaysCreateSpan, {}); + fetchCallback(secondReqData, alwaysCreateSpan, alwaysAttachHeaders, {}); expect(transaction.metadata.propagations).toBe(2); }); - - it('adds sentry-trace header to fetch requests', () => { - // TODO - }); }); describe('xhrCallback()', () => { - it('does not create span if shouldCreateSpan returns false', () => { - const spans = {}; - - xhrCallback(xhrHandlerData, neverCreateSpan, spans); + let xhrHandlerData: XHRData; - expect(spans).toEqual({}); + const xhrSpan = { + data: { + method: 'GET', + url: 'http://dogs.are.great/', + type: 'xhr', + }, + description: 'GET http://dogs.are.great/', + op: 'http.client', + parentSpanId: expect.any(String), + spanId: expect.any(String), + startTimestamp: expect.any(Number), + traceId: expect.any(String), + }; + + beforeEach(() => { + xhrHandlerData = { + xhr: { + __sentry_xhr__: { + method: 'GET', + url: 'http://dogs.are.great/', + status_code: 200, + data: {}, + }, + __sentry_xhr_span_id__: '1231201211212012', + setRequestHeader, + }, + startTimestamp, + }; }); - it('does not add xhr request spans if tracing is disabled', () => { - hasTracingEnabled.mockReturnValueOnce(false); - const spans = {}; - - xhrCallback(xhrHandlerData, alwaysCreateSpan, spans); - expect(spans).toEqual({}); - }); + it.each([ + // each case is [shouldCreateSpanReturnValue, shouldAttachHeadersReturnValue, expectedSpan, expectedHeaderKeys] + [true, true, expect.objectContaining(xhrSpan), ['sentry-trace', 'baggage']], + [true, false, expect.objectContaining(xhrSpan), []], + // If there's no span then there's no parent span id to stick into a header, so no headers, even if there's a + // `tracingOrigins` match + [false, true, undefined, []], + [false, false, undefined, []], + ])( + 'span creation/header attachment interaction - shouldCreateSpan: %s, shouldAttachHeaders: %s', + (shouldCreateSpanReturnValue, shouldAttachHeadersReturnValue, expectedSpan, expectedHeaderKeys) => { + xhrCallback( + xhrHandlerData, + () => shouldCreateSpanReturnValue, + () => shouldAttachHeadersReturnValue, + {}, + ); + + // spans[0] is the transaction itself + const newSpan = transaction.spanRecorder?.spans[1] as Span; + expect(newSpan).toEqual(expectedSpan); + + const headerKeys = setRequestHeader.mock.calls.map(header => header[0]); + expect(headerKeys).toEqual(expectedHeaderKeys); + }, + ); - it('does not add xhr request headers if tracing is disabled', () => { + it('adds neither xhr request spans nor xhr request headers if tracing is disabled', () => { hasTracingEnabled.mockReturnValueOnce(false); + const spans = {}; - xhrCallback(xhrHandlerData, alwaysCreateSpan, {}); + xhrCallback(xhrHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); + expect(spans).toEqual({}); expect(setRequestHeader).not.toHaveBeenCalled(); }); - it('adds sentry-trace header to XHR requests', () => { - xhrCallback(xhrHandlerData, alwaysCreateSpan, {}); - - expect(setRequestHeader).toHaveBeenCalledWith( - 'sentry-trace', - expect.stringMatching(tracingUtils.TRACEPARENT_REGEXP), - ); - }); - it('creates and finishes XHR span on active transaction', () => { const spans = {}; // triggered by request being sent - xhrCallback(xhrHandlerData, alwaysCreateSpan, spans); + xhrCallback(xhrHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); const newSpan = transaction.spanRecorder?.spans[1] as Span; @@ -261,8 +312,8 @@ describe('callbacks', () => { }); expect(newSpan.description).toBe('GET http://dogs.are.great/'); expect(newSpan.op).toBe('http.client'); - expect(xhrHandlerData.xhr.__sentry_xhr_span_id__).toBeDefined(); - expect(xhrHandlerData.xhr.__sentry_xhr_span_id__).toEqual(newSpan?.spanId); + expect(xhrHandlerData.xhr?.__sentry_xhr_span_id__).toBeDefined(); + expect(xhrHandlerData.xhr?.__sentry_xhr_span_id__).toEqual(newSpan?.spanId); const postRequestXHRHandlerData = { ...xhrHandlerData, @@ -270,7 +321,7 @@ describe('callbacks', () => { }; // triggered by response coming back - xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, spans); + xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); expect(newSpan.endTimestamp).toBeDefined(); }); @@ -279,7 +330,7 @@ describe('callbacks', () => { const spans = {}; // triggered by request being sent - xhrCallback(xhrHandlerData, alwaysCreateSpan, spans); + xhrCallback(xhrHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); const newSpan = transaction.spanRecorder?.spans[1] as Span; @@ -289,10 +340,10 @@ describe('callbacks', () => { ...xhrHandlerData, endTimestamp, }; - postRequestXHRHandlerData.xhr.__sentry_xhr__.status_code = 404; + postRequestXHRHandlerData.xhr!.__sentry_xhr__!.status_code = 404; // triggered by response coming back - xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, spans); + xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); expect(newSpan.status).toBe(spanStatusfromHttpCode(404)); }); @@ -303,7 +354,7 @@ describe('callbacks', () => { const postRequestXHRHandlerData = { ...{ xhr: { - __sentry_xhr__: xhrHandlerData.xhr.__sentry_xhr__, + __sentry_xhr__: xhrHandlerData.xhr?.__sentry_xhr__, }, }, startTimestamp, @@ -311,7 +362,7 @@ describe('callbacks', () => { }; // in that case, the response coming back will be ignored - xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, {}); + xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, alwaysAttachHeaders, {}); const newSpan = transaction.spanRecorder?.spans[1]; @@ -324,10 +375,10 @@ describe('callbacks', () => { expect(transaction.metadata.propagations).toBe(0); - xhrCallback(firstReqData, alwaysCreateSpan, {}); + xhrCallback(firstReqData, alwaysCreateSpan, alwaysAttachHeaders, {}); expect(transaction.metadata.propagations).toBe(1); - xhrCallback(secondReqData, alwaysCreateSpan, {}); + xhrCallback(secondReqData, alwaysCreateSpan, alwaysAttachHeaders, {}); expect(transaction.metadata.propagations).toBe(2); }); });