From ac435ddda1f8d7d7ed9e6a3686a05a146d009b1d Mon Sep 17 00:00:00 2001
From: Abhijeet Prasad <aprasad@sentry.io>
Date: Tue, 27 Jun 2023 09:23:49 -0400
Subject: [PATCH] feat(tracing): Favour client options tracePropagationTargets
 (#8399)

ref https://github.com/getsentry/sentry-javascript/issues/8352

As we work toward adding tracing without performance support, this PR
updates the `BrowserTracing` integration to use and favour the top level
`tracePropagationTargets` option if it exists.

This option was made top level in
https://github.com/getsentry/sentry-javascript/pull/8395

`tracePropagationTargets` is now part of the unified API for distributed
tracing. It's also expected that electron/react native will behave the
same way as well. This also leaves us the flexibility to extract tracing
out of BrowserTracing, or create a new integration that just does
tracing but no performance monitoring.

We can make sure this migration is smooth and easy to understand with a
good set of docs, which is what I will be working on next. In these docs
changes, we'll be updating the automatic instrumentation sections, and
formally documented `tracePropagationTargets` as a high level option.
---
 .../src/browser/browsertracing.ts             | 32 +++++++++-
 .../test/browser/browsertracing.test.ts       | 59 +++++++++++++++++++
 2 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/packages/tracing-internal/src/browser/browsertracing.ts b/packages/tracing-internal/src/browser/browsertracing.ts
index deb240ec4233..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<BrowserTracingOptions>) {
     addTracingExtensions();
 
+    if (__DEBUG_BUILD__) {
+      this._hasSetTracePropagationTargets = !!(
+        _options &&
+        // eslint-disable-next-line deprecation/deprecation
+        (_options.tracePropagationTargets || _options.tracingOrigins)
+      );
+    }
+
     this.options = {
       ...DEFAULT_BROWSER_TRACING_OPTIONS,
       ..._options,
@@ -214,6 +224,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 +235,28 @@ export class BrowserTracing implements Integration {
       markBackgroundTransactions,
       traceFetch,
       traceXHR,
-      tracePropagationTargets,
       shouldCreateSpanForRequest,
       _experiments,
     } = this.options;
 
+    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) => {
         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..c7cec8a54735 100644
--- a/packages/tracing-internal/test/browser/browsertracing.test.ts
+++ b/packages/tracing-internal/test/browser/browsertracing.test.ts
@@ -250,6 +250,65 @@ 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'],
+        });
+      });
+
+      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<BrowserTracingOptions>) => {
+          jest.clearAllMocks();
+          const inst = createBrowserTracing(true, {
+            routingInstrumentation: customInstrumentRouting,
+            ...options,
+          });
+
+          // @ts-ignore accessing private property
+          expect(inst._hasSetTracePropagationTargets).toBe(hasSet);
+        },
+      );
     });
 
     describe('beforeNavigate', () => {