fix(nextjs|sveltekit/v7): Ensure we can pass browserTracingIntegration (#11765)

Turns out the logic to get the options was not correct, because we used
the `options` that the integration exposes, but this has
`instrumentPageload: false` & `instrumentNavigation: false` because we
did not "fix" the `options` we expose on the integration. This was not
caught by the tests, because it only happens if passing the
`browserTracingIntegration` from `@sentry/nextjs` or
`@sentry/sveltekit`, not when passing the "original" one (which we had
tests for).

Really fixes #11627

Author: mydea

dev-packages/e2e-tests/test-applications/nextjs-14/sentry.client.config.ts

@@ -6,4 +6,7 @@ Sentry.init({
   tunnel: `http://localhost:3031/`, // proxy server
   tracesSampleRate: 1.0,
   sendDefaultPii: true,
+
+  // Ensure it also works with passing the integration
+  integrations: [Sentry.browserTracingIntegration()],
 });

packages/nextjs/src/client/browserTracingIntegration.ts

@@ -5,7 +5,7 @@ import {
   startBrowserTracingNavigationSpan,
   startBrowserTracingPageLoadSpan,
 } from '@sentry/react';
-import type { Integration, StartSpanOptions } from '@sentry/types';
+import type { StartSpanOptions } from '@sentry/types';
 import { nextRouterInstrumentation } from '../index.client';
 
 /**
@@ -42,7 +42,7 @@ export class BrowserTracing extends OriginalBrowserTracing {
  */
 export function browserTracingIntegration(
   options?: Parameters<typeof originalBrowserTracingIntegration>[0],
-): Integration {
+): ReturnType<typeof originalBrowserTracingIntegration> {
   const browserTracingIntegrationInstance = originalBrowserTracingIntegration({
     // eslint-disable-next-line deprecation/deprecation
     tracingOrigins:
@@ -61,8 +61,16 @@ export function browserTracingIntegration(
     instrumentPageLoad: false,
   });
 
+  const fullOptions = {
+    ...browserTracingIntegrationInstance.options,
+    instrumentPageLoad: true,
+    instrumentNavigation: true,
+    ...options,
+  };
+
   return {
     ...browserTracingIntegrationInstance,
+    options: fullOptions,
     afterAllSetup(client) {
       const startPageloadCallback = (startSpanOptions: StartSpanOptions): void => {
         startBrowserTracingPageLoadSpan(client, startSpanOptions);
@@ -80,7 +88,7 @@ export function browserTracingIntegration(
       nextRouterInstrumentation(
         () => undefined,
         false,
-        options?.instrumentNavigation,
+        fullOptions.instrumentNavigation,
         startPageloadCallback,
         startNavigationCallback,
       );
@@ -90,7 +98,7 @@ export function browserTracingIntegration(
       // eslint-disable-next-line deprecation/deprecation
       nextRouterInstrumentation(
         () => undefined,
-        options?.instrumentPageLoad,
+        fullOptions.instrumentPageLoad,
         false,
         startPageloadCallback,
         startNavigationCallback,

packages/nextjs/test/clientSdk.test.ts

@@ -1,4 +1,10 @@
-import { BaseClient, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, getActiveSpan, spanToJSON } from '@sentry/core';
+import {
+  BaseClient,
+  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
+  getActiveSpan,
+  getIsolationScope,
+  spanToJSON,
+} from '@sentry/core';
 import * as SentryReact from '@sentry/react';
 import type { BrowserClient } from '@sentry/react';
 import { browserTracingIntegration } from '@sentry/react';
@@ -7,7 +13,13 @@ import type { Integration } from '@sentry/types';
 import { logger } from '@sentry/utils';
 import { JSDOM } from 'jsdom';
 
-import { BrowserTracing, breadcrumbsIntegration, init, nextRouterInstrumentation } from '../src/client';
+import {
+  BrowserTracing,
+  breadcrumbsIntegration,
+  browserTracingIntegration as nextjsBrowserTracingIntegration,
+  init,
+  nextRouterInstrumentation,
+} from '../src/client';
 
 const reactInit = jest.spyOn(SentryReact, 'init');
 const captureEvent = jest.spyOn(BaseClient.prototype, 'captureEvent');
@@ -35,6 +47,11 @@ function findIntegrationByName(integrations: Integration[] = [], name: string):
 const TEST_DSN = 'https://[email protected]/1337';
 
 describe('Client init()', () => {
+  beforeEach(() => {
+    getCurrentScope().clear();
+    getIsolationScope().clear();
+  });
+
   afterEach(() => {
     jest.clearAllMocks();
     WINDOW.__SENTRY__.hub = undefined;
@@ -141,9 +158,16 @@ describe('Client init()', () => {
     });
 
     it('forces correct router instrumentation if user provides `browserTracingIntegration`', () => {
+      const beforeStartSpan = jest.fn(options => options);
       init({
         dsn: TEST_DSN,
-        integrations: [browserTracingIntegration({ finalTimeout: 10 })],
+        integrations: [
+          browserTracingIntegration({
+            finalTimeout: 10,
+            instrumentNavigation: false,
+            beforeStartSpan,
+          }),
+        ],
         enableTracing: true,
       });
 
@@ -156,14 +180,58 @@ describe('Client init()', () => {
       // It is a "new" browser tracing integration
       expect(typeof integration?.afterAllSetup).toBe('function');
 
+      // the hooks is correctly invoked
+      expect(beforeStartSpan).toHaveBeenCalledTimes(1);
+      expect(beforeStartSpan).toHaveBeenCalledWith(
+        expect.objectContaining({
+          name: '/',
+          op: 'pageload',
+        }),
+      );
+
+      // it correctly starts the page load span
+      expect(getActiveSpan()).toBeDefined();
+      expect(spanToJSON(getActiveSpan()!).data?.[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]).toEqual(
+        'auto.pageload.nextjs.app_router_instrumentation',
+      );
+
       // This shows that the user-configured options are still here
-      expect(integration?.options?.finalTimeout).toEqual(10);
+      expect(integration?.options.finalTimeout).toEqual(10);
+      expect(integration?.options.instrumentNavigation).toBe(false);
+      expect(integration?.options.instrumentPageLoad).toBe(true);
+    });
 
-      // it is the svelte kit variety
+    it('forces correct router instrumentation if user provides Next.js `browserTracingIntegration` ', () => {
+      init({
+        dsn: TEST_DSN,
+        integrations: [
+          nextjsBrowserTracingIntegration({
+            finalTimeout: 10,
+            instrumentNavigation: false,
+          }),
+        ],
+        enableTracing: true,
+      });
+
+      const client = getClient<BrowserClient>()!;
+      // eslint-disable-next-line deprecation/deprecation
+      const integration = client.getIntegrationByName<ReturnType<typeof browserTracingIntegration>>('BrowserTracing');
+
+      expect(integration).toBeDefined();
+
+      // It is a "new" browser tracing integration
+      expect(typeof integration?.afterAllSetup).toBe('function');
+
+      // it correctly starts the pageload span
       expect(getActiveSpan()).toBeDefined();
       expect(spanToJSON(getActiveSpan()!).data?.[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]).toEqual(
         'auto.pageload.nextjs.app_router_instrumentation',
       );
+
+      // This shows that the user-configured options are still here
+      expect(integration?.options.finalTimeout).toEqual(10);
+      expect(integration?.options.instrumentNavigation).toBe(false);
+      expect(integration?.options.instrumentPageLoad).toBe(true);
     });
 
     it('forces correct router instrumentation if user provides `BrowserTracing` in a function', () => {
@@ -187,10 +255,10 @@ describe('Client init()', () => {
 
       expect(browserTracingIntegration?.options).toEqual(
         expect.objectContaining({
+          startTransactionOnPageLoad: true,
+          startTransactionOnLocationChange: false,
           // eslint-disable-next-line deprecation/deprecation
           routingInstrumentation: nextRouterInstrumentation,
-          // This proves it's still the user's copy
-          startTransactionOnLocationChange: false,
         }),
       );
     });

packages/sveltekit/src/client/browserTracingIntegration.ts

@@ -9,7 +9,7 @@ import {
   startBrowserTracingPageLoadSpan,
   startInactiveSpan,
 } from '@sentry/svelte';
-import type { Client, Integration, Span } from '@sentry/types';
+import type { Client, Span } from '@sentry/types';
 import { svelteKitRoutingInstrumentation } from './router';
 
 /**
@@ -36,7 +36,7 @@ export class BrowserTracing extends OriginalBrowserTracing {
  */
 export function browserTracingIntegration(
   options: Parameters<typeof originalBrowserTracingIntegration>[0] = {},
-): Integration {
+): ReturnType<typeof originalBrowserTracingIntegration> {
   const integration = {
     ...originalBrowserTracingIntegration({
       ...options,
@@ -45,16 +45,24 @@ export function browserTracingIntegration(
     }),
   };
 
+  const fullOptions = {
+    ...integration.options,
+    instrumentPageLoad: true,
+    instrumentNavigation: true,
+    ...options,
+  };
+
   return {
     ...integration,
+    options: fullOptions,
     afterAllSetup: client => {
       integration.afterAllSetup(client);
 
-      if (options.instrumentPageLoad !== false) {
+      if (fullOptions.instrumentPageLoad) {
         _instrumentPageload(client);
       }
 
-      if (options.instrumentNavigation !== false) {
+      if (fullOptions.instrumentNavigation) {
         _instrumentNavigations(client);
       }
     },

packages/sveltekit/test/client/sdk.test.ts

@@ -1,10 +1,21 @@
-import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, getActiveSpan, getClient, getCurrentScope, spanToJSON } from '@sentry/core';
+import {
+  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
+  getActiveSpan,
+  getClient,
+  getCurrentScope,
+  getIsolationScope,
+  spanToJSON,
+} from '@sentry/core';
 import type { BrowserClient } from '@sentry/svelte';
 import * as SentrySvelte from '@sentry/svelte';
 import { SDK_VERSION, WINDOW, browserTracingIntegration } from '@sentry/svelte';
 import { vi } from 'vitest';
 
-import { BrowserTracing, init } from '../../src/client';
+import {
+  BrowserTracing,
+  browserTracingIntegration as sveltekitBrowserTracingIntegration,
+  init,
+} from '../../src/client';
 import { svelteKitRoutingInstrumentation } from '../../src/client/router';
 
 const svelteInit = vi.spyOn(SentrySvelte, 'init');
@@ -16,6 +27,11 @@ describe('Sentry client SDK', () => {
       WINDOW.__SENTRY__.hub = undefined;
     });
 
+    beforeEach(() => {
+      getCurrentScope().clear();
+      getIsolationScope().clear();
+    });
+
     it('adds SvelteKit metadata to the SDK options', () => {
       expect(svelteInit).not.toHaveBeenCalled();
 
@@ -99,7 +115,7 @@ describe('Sentry client SDK', () => {
         init({
           dsn: 'https://[email protected]/1337',
           // eslint-disable-next-line deprecation/deprecation
-          integrations: [new BrowserTracing({ finalTimeout: 10 })],
+          integrations: [new BrowserTracing({ finalTimeout: 10, startTransactionOnLocationChange: false })],
           enableTracing: true,
         });
 
@@ -118,34 +134,65 @@ describe('Sentry client SDK', () => {
         // But we force the routing instrumentation to be ours
         // eslint-disable-next-line deprecation/deprecation
         expect(options.routingInstrumentation).toEqual(svelteKitRoutingInstrumentation);
+        expect(options.startTransactionOnPageLoad).toEqual(true);
+        expect(options.startTransactionOnLocationChange).toEqual(false);
       });
 
       it('Merges a user-provided browserTracingIntegration with the automatically added one', () => {
         init({
           dsn: 'https://[email protected]/1337',
-          integrations: [browserTracingIntegration({ finalTimeout: 10 })],
+          integrations: [browserTracingIntegration({ finalTimeout: 10, instrumentNavigation: false })],
           enableTracing: true,
         });
 
         const browserTracing =
           getClient<BrowserClient>()?.getIntegrationByName<ReturnType<typeof browserTracingIntegration>>(
             'BrowserTracing',
           );
-        const options = browserTracing?.options;
 
         expect(browserTracing).toBeDefined();
 
         // It is a "new" browser tracing integration
         expect(typeof browserTracing?.afterAllSetup).toBe('function');
 
+        // it is the svelte kit variety
+        expect(getActiveSpan()).toBeDefined();
+        expect(spanToJSON(getActiveSpan()!).data?.[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]).toEqual(
+          'auto.pageload.sveltekit',
+        );
+
         // This shows that the user-configured options are still here
-        expect(options?.finalTimeout).toEqual(10);
+        expect(browserTracing?.options.finalTimeout).toEqual(10);
+        expect(browserTracing?.options.instrumentPageLoad).toEqual(true);
+        expect(browserTracing?.options.instrumentNavigation).toEqual(false);
+      });
 
-        // it is the svelte kit variety
+      it('forces correct router instrumentation if user provides Sveltekit `browserTracingIntegration` ', () => {
+        init({
+          dsn: 'https://[email protected]/1337',
+          integrations: [sveltekitBrowserTracingIntegration({ finalTimeout: 10, instrumentNavigation: false })],
+          enableTracing: true,
+        });
+
+        const client = getClient<BrowserClient>()!;
+        // eslint-disable-next-line deprecation/deprecation
+        const integration = client.getIntegrationByName<ReturnType<typeof browserTracingIntegration>>('BrowserTracing');
+
+        expect(integration).toBeDefined();
+
+        // It is a "new" browser tracing integration
+        expect(typeof integration?.afterAllSetup).toBe('function');
+
+        // it correctly starts the pageload span
         expect(getActiveSpan()).toBeDefined();
         expect(spanToJSON(getActiveSpan()!).data?.[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]).toEqual(
           'auto.pageload.sveltekit',
         );
+
+        // This shows that the user-configured options are still here
+        expect(integration?.options.finalTimeout).toEqual(10);
+        expect(integration?.options.instrumentNavigation).toBe(false);
+        expect(integration?.options.instrumentPageLoad).toBe(true);
       });
     });
   });