fix(browser): Move browserTracingIntegration code to setup hook (#16386)

This PR removes the warning for multiple browserTracingIntegrations
being setup.
We realised this warning was actually just a symptom of us running a
bunch of the integration code in the function itself, not int `setup()`.
This lead to stuff running twice when users passed in a custom
integration in addition to a default one, because the logic is
basically:

```js
const defaultIntergations = [browserTracingIntegration()];
const usedDefinedIntegrations = [browserTracingIntegration()];
// at this point, the function was already executed twice, leading to side effects even without calling `init`!
```

Now, we move all the logic to setup/setupAfterAll, so this should be
safe. This means that the integration will be deduped anyhow (I added a
test to make sure this is the case), and we no longer need the warning.

fixes #16369

Author: mydea

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/multiple-integrations/init.js

@@ -0,0 +1,14 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  integrations: [Sentry.browserTracingIntegration()],
+  tracePropagationTargets: ['http://sentry-test-site.example'],
+  tracesSampleRate: 1,
+  autoSessionTracking: false,
+});
+
+// fetch directly after init
+fetch('http://sentry-test-site.example/0');

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/multiple-integrations/test.ts

@@ -0,0 +1,40 @@
+import { expect } from '@playwright/test';
+import { sentryTest } from '../../../../utils/fixtures';
+import {
+  envelopeRequestParser,
+  shouldSkipTracingTest,
+  waitForTransactionRequestOnUrl,
+} from '../../../../utils/helpers';
+
+sentryTest('should create spans for fetch requests called directly after init', async ({ getLocalTestUrl, page }) => {
+  if (shouldSkipTracingTest()) {
+    sentryTest.skip();
+  }
+
+  await page.route('http://sentry-test-site.example/*', route => route.fulfill({ body: 'ok' }));
+
+  const url = await getLocalTestUrl({ testDir: __dirname });
+
+  const req = await waitForTransactionRequestOnUrl(page, url);
+  const tracingEvent = envelopeRequestParser(req);
+
+  const requestSpans = tracingEvent.spans?.filter(({ op }) => op === 'http.client');
+
+  expect(requestSpans).toHaveLength(1);
+
+  expect(requestSpans![0]).toMatchObject({
+    description: 'GET http://sentry-test-site.example/0',
+    parent_span_id: tracingEvent.contexts?.trace?.span_id,
+    span_id: expect.stringMatching(/[a-f0-9]{16}/),
+    start_timestamp: expect.any(Number),
+    timestamp: expect.any(Number),
+    trace_id: tracingEvent.contexts?.trace?.trace_id,
+    data: {
+      'http.method': 'GET',
+      'http.url': 'http://sentry-test-site.example/0',
+      url: 'http://sentry-test-site.example/0',
+      'server.address': 'sentry-test-site.example',
+      type: 'fetch',
+    },
+  });
+});

dev-packages/browser-integration-tests/suites/tracing/request/fetch-immediate/init.js

@@ -3,7 +3,6 @@ import type { Client, IntegrationFn, Span, StartSpanOptions, TransactionSource,
 import {
   addNonEnumerableProperty,
   browserPerformanceTimeOrigin,
-  consoleSandbox,
   generateTraceId,
   getClient,
   getCurrentScope,
@@ -233,8 +232,6 @@ const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = {
   ...defaultRequestInstrumentationOptions,
 };
 
-let _hasBeenInitialized = false;
-
 /**
  * The Browser Tracing integration automatically instruments browser pageload/navigation
  * actions as transactions, and captures requests, metrics and errors as spans.
@@ -245,23 +242,17 @@ let _hasBeenInitialized = false;
  * We explicitly export the proper type here, as this has to be extended in some cases.
  */
 export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptions> = {}) => {
-  if (_hasBeenInitialized) {
-    consoleSandbox(() => {
-      // eslint-disable-next-line no-console
-      console.warn('Multiple browserTracingIntegration instances are not supported.');
-    });
-  }
-
-  _hasBeenInitialized = true;
+  const latestRoute: RouteInfo = {
+    name: undefined,
+    source: undefined,
+  };
 
   /**
    * This is just a small wrapper that makes `document` optional.
    * We want to be extra-safe and always check that this exists, to ensure weird environments do not blow up.
    */
   const optionalWindowDocument = WINDOW.document as (typeof WINDOW)['document'] | undefined;
 
-  registerSpanErrorInstrumentation();
-
   const {
     enableInp,
     enableLongTask,
@@ -287,31 +278,7 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
     ..._options,
   };
 
-  const _collectWebVitals = startTrackingWebVitals({ recordClsStandaloneSpans: enableStandaloneClsSpans || false });
-
-  if (enableInp) {
-    startTrackingINP();
-  }
-
-  if (
-    enableLongAnimationFrame &&
-    GLOBAL_OBJ.PerformanceObserver &&
-    PerformanceObserver.supportedEntryTypes &&
-    PerformanceObserver.supportedEntryTypes.includes('long-animation-frame')
-  ) {
-    startTrackingLongAnimationFrames();
-  } else if (enableLongTask) {
-    startTrackingLongTasks();
-  }
-
-  if (enableInteractions) {
-    startTrackingInteractions();
-  }
-
-  const latestRoute: RouteInfo = {
-    name: undefined,
-    source: undefined,
-  };
+  let _collectWebVitals: undefined | (() => void);
 
   /** Create routing idle transaction. */
   function _createRouteSpan(client: Client, startSpanOptions: StartSpanOptions): void {
@@ -340,7 +307,9 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
       // should wait for finish signal if it's a pageload transaction
       disableAutoFinish: isPageloadTransaction,
       beforeSpanEnd: span => {
-        _collectWebVitals();
+        // This will generally always be defined here, because it is set in `setup()` of the integration
+        // but technically, it is optional, so we guard here to be extra safe
+        _collectWebVitals?.();
         addPerformanceEntries(span, { recordClsOnPageloadSpan: !enableStandaloneClsSpans });
         setActiveIdleSpan(client, undefined);
 
@@ -378,8 +347,29 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
 
   return {
     name: BROWSER_TRACING_INTEGRATION_ID,
-    afterAllSetup(client) {
-      let startingUrl: string | undefined = getLocationHref();
+    setup(client) {
+      registerSpanErrorInstrumentation();
+
+      _collectWebVitals = startTrackingWebVitals({ recordClsStandaloneSpans: enableStandaloneClsSpans || false });
+
+      if (enableInp) {
+        startTrackingINP();
+      }
+
+      if (
+        enableLongAnimationFrame &&
+        GLOBAL_OBJ.PerformanceObserver &&
+        PerformanceObserver.supportedEntryTypes &&
+        PerformanceObserver.supportedEntryTypes.includes('long-animation-frame')
+      ) {
+        startTrackingLongAnimationFrames();
+      } else if (enableLongTask) {
+        startTrackingLongTasks();
+      }
+
+      if (enableInteractions) {
+        startTrackingInteractions();
+      }
 
       function maybeEndActiveSpan(): void {
         const activeSpan = getActiveIdleSpan(client);
@@ -440,6 +430,9 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
           ...startSpanOptions,
         });
       });
+    },
+    afterAllSetup(client) {
+      let startingUrl: string | undefined = getLocationHref();
 
       if (linkPreviousTrace !== 'off') {
         linkTraces(client, { linkPreviousTrace, consistentTraceSampling });

dev-packages/browser-integration-tests/suites/tracing/request/fetch-immediate/test.ts

@@ -22,14 +22,12 @@ class MockIntegration implements Integration {
   // Only for testing - tag to keep separate instances straight when testing deduplication
   public tag?: string;
 
+  public setupOnce = vi.fn(() => {});
+
   public constructor(name: string, tag?: string) {
     this.name = name;
     this.tag = tag;
   }
-
-  public setupOnce(): void {
-    // noop
-  }
 }
 
 type TestCase = [
@@ -74,6 +72,31 @@ describe('getIntegrationsToSetup', () => {
       });
       expect(integrations.map(i => i.name)).toEqual(expected);
     });
+
+    test('it uses passed integration over default intergation', () => {
+      const integrationDefault = new MockIntegration('ChaseSquirrels');
+      const integration1 = new MockIntegration('ChaseSquirrels');
+
+      const integrations = getIntegrationsToSetup({
+        defaultIntegrations: [integrationDefault],
+        integrations: [integration1],
+      });
+
+      expect(integrations).toEqual([integration1]);
+    });
+
+    test('it uses last passed integration only', () => {
+      const integrationDefault = new MockIntegration('ChaseSquirrels');
+      const integration1 = new MockIntegration('ChaseSquirrels');
+      const integration2 = new MockIntegration('ChaseSquirrels');
+
+      const integrations = getIntegrationsToSetup({
+        defaultIntegrations: [integrationDefault],
+        integrations: [integration1, integration2],
+      });
+
+      expect(integrations).toEqual([integration2]);
+    });
   });
 
   describe('deduping', () => {

packages/browser/src/tracing/browserTracingIntegration.ts

@@ -236,7 +236,9 @@ export function createReactRouterV6CompatibleTracingIntegration(
 
   return {
     ...integration,
-    setup() {
+    setup(client) {
+      integration.setup(client);
+
       _useEffect = useEffect;
       _useLocation = useLocation;
       _useNavigationType = useNavigationType;