From cd93c1c090fd89ef2a458f1ab3f715327dd5fe08 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 23 Jun 2023 13:06:18 -0400 Subject: [PATCH 1/2] feat(tracing): Favour client options tracePropagationTargets --- .../src/browser/browsertracing.ts | 7 ++++++- .../test/browser/browsertracing.test.ts | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/tracing-internal/src/browser/browsertracing.ts b/packages/tracing-internal/src/browser/browsertracing.ts index deb240ec4233..202f4fe7a5f4 100644 --- a/packages/tracing-internal/src/browser/browsertracing.ts +++ b/packages/tracing-internal/src/browser/browsertracing.ts @@ -214,6 +214,9 @@ export class BrowserTracing implements Integration { */ public setupOnce(_: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void { this._getCurrentHub = getCurrentHub; + const hub = getCurrentHub(); + const client = hub.getClient(); + const clientOptions = client && client.getOptions(); const { routingInstrumentation: instrumentRouting, @@ -222,11 +225,13 @@ export class BrowserTracing implements Integration { markBackgroundTransactions, traceFetch, traceXHR, - tracePropagationTargets, shouldCreateSpanForRequest, _experiments, } = this.options; + const tracePropagationTargets = + (clientOptions && clientOptions.tracePropagationTargets) || this.options.tracePropagationTargets; + instrumentRouting( (context: TransactionContext) => { const transaction = this._createRouteTransaction(context); diff --git a/packages/tracing-internal/test/browser/browsertracing.test.ts b/packages/tracing-internal/test/browser/browsertracing.test.ts index 669e79e3c097..0e8a80f54ad4 100644 --- a/packages/tracing-internal/test/browser/browsertracing.test.ts +++ b/packages/tracing-internal/test/browser/browsertracing.test.ts @@ -250,6 +250,22 @@ describe('BrowserTracing', () => { tracePropagationTargets: ['something'], }); }); + + it('uses `tracePropagationTargets` set by client over integration set targets', () => { + jest.clearAllMocks(); + hub.getClient()!.getOptions().tracePropagationTargets = ['something-else']; + const sampleTracePropagationTargets = ['something']; + createBrowserTracing(true, { + routingInstrumentation: customInstrumentRouting, + tracePropagationTargets: sampleTracePropagationTargets, + }); + + expect(instrumentOutgoingRequestsMock).toHaveBeenCalledWith({ + traceFetch: true, + traceXHR: true, + tracePropagationTargets: ['something-else'], + }); + }); }); describe('beforeNavigate', () => { From b0981889be9f0f38f9d837b38c2d742bce1a3b01 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 26 Jun 2023 13:21:40 -0400 Subject: [PATCH 2/2] Add warning if both options are set --- .../src/browser/browsertracing.ts | 29 ++++++++++++- .../test/browser/browsertracing.test.ts | 43 +++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/packages/tracing-internal/src/browser/browsertracing.ts b/packages/tracing-internal/src/browser/browsertracing.ts index 202f4fe7a5f4..06c531b7941f 100644 --- a/packages/tracing-internal/src/browser/browsertracing.ts +++ b/packages/tracing-internal/src/browser/browsertracing.ts @@ -177,9 +177,19 @@ export class BrowserTracing implements Integration { private _collectWebVitals: () => void; + private _hasSetTracePropagationTargets: boolean = false; + public constructor(_options?: Partial) { addTracingExtensions(); + if (__DEBUG_BUILD__) { + this._hasSetTracePropagationTargets = !!( + _options && + // eslint-disable-next-line deprecation/deprecation + (_options.tracePropagationTargets || _options.tracingOrigins) + ); + } + this.options = { ...DEFAULT_BROWSER_TRACING_OPTIONS, ..._options, @@ -229,8 +239,23 @@ export class BrowserTracing implements Integration { _experiments, } = this.options; - const tracePropagationTargets = - (clientOptions && clientOptions.tracePropagationTargets) || this.options.tracePropagationTargets; + const clientOptionsTracePropagationTargets = clientOptions && clientOptions.tracePropagationTargets; + // There are three ways to configure tracePropagationTargets: + // 1. via top level client option `tracePropagationTargets` + // 2. via BrowserTracing option `tracePropagationTargets` + // 3. via BrowserTracing option `tracingOrigins` (deprecated) + // + // To avoid confusion, favour top level client option `tracePropagationTargets`, and fallback to + // BrowserTracing option `tracePropagationTargets` and then `tracingOrigins` (deprecated). + // This is done as it minimizes bundle size (we don't have to have undefined checks). + // + // If both 1 and either one of 2 or 3 are set (from above), we log out a warning. + const tracePropagationTargets = clientOptionsTracePropagationTargets || this.options.tracePropagationTargets; + if (__DEBUG_BUILD__ && this._hasSetTracePropagationTargets && clientOptionsTracePropagationTargets) { + logger.warn( + '[Tracing] The `tracePropagationTargets` option was set in the BrowserTracing integration and top level `Sentry.init`. The top level `Sentry.init` value is being used.', + ); + } instrumentRouting( (context: TransactionContext) => { diff --git a/packages/tracing-internal/test/browser/browsertracing.test.ts b/packages/tracing-internal/test/browser/browsertracing.test.ts index 0e8a80f54ad4..c7cec8a54735 100644 --- a/packages/tracing-internal/test/browser/browsertracing.test.ts +++ b/packages/tracing-internal/test/browser/browsertracing.test.ts @@ -266,6 +266,49 @@ describe('BrowserTracing', () => { tracePropagationTargets: ['something-else'], }); }); + + it.each([ + [true, 'tracePropagationTargets', 'defined', { tracePropagationTargets: ['something'] }], + [false, 'tracePropagationTargets', 'undefined', { tracePropagationTargets: undefined }], + [true, 'tracingOrigins', 'defined', { tracingOrigins: ['something'] }], + [false, 'tracingOrigins', 'undefined', { tracingOrigins: undefined }], + [ + true, + 'tracePropagationTargets and tracingOrigins', + 'defined', + { tracePropagationTargets: ['something'], tracingOrigins: ['something-else'] }, + ], + [ + false, + 'tracePropagationTargets and tracingOrigins', + 'undefined', + { tracePropagationTargets: undefined, tracingOrigins: undefined }, + ], + [ + true, + 'tracePropagationTargets and tracingOrigins', + 'defined and undefined', + { tracePropagationTargets: ['something'], tracingOrigins: undefined }, + ], + [ + true, + 'tracePropagationTargets and tracingOrigins', + 'undefined and defined', + { tracePropagationTargets: undefined, tracingOrigins: ['something'] }, + ], + ])( + 'sets `_hasSetTracePropagationTargets` to %s if %s is %s', + (hasSet: boolean, _: string, __: string, options: Partial) => { + jest.clearAllMocks(); + const inst = createBrowserTracing(true, { + routingInstrumentation: customInstrumentRouting, + ...options, + }); + + // @ts-ignore accessing private property + expect(inst._hasSetTracePropagationTargets).toBe(hasSet); + }, + ); }); describe('beforeNavigate', () => {