fix(nextjs|sveltekit): Ensure we can pass browserTracingIntegration

mydea · mydea · commit c62591f738c1 · 2024-04-24T10:08:45.000+02:00
more improvements

add one e2e test

add test for beforestartspan
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-14/sentry.client.config.ts b/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()],
 });
diff --git a/packages/nextjs/src/client/browserTracingIntegration.ts b/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,
diff --git a/packages/nextjs/test/clientSdk.test.ts b/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';
@@ -35,6 +41,11 @@ function findIntegrationByName(integrations: Integration[] = [], name: string):
 const TEST_DSN = 'https://public@dsn.ingest.sentry.io/1337';
 
 describe('Client init()', () => {
+  beforeEach(() => {
+    getCurrentScope().clear();
+    getIsolationScope().clear();
+  });
+
   afterEach(() => {
     jest.clearAllMocks();
     WINDOW.__SENTRY__.hub = undefined;
@@ -141,9 +152,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,
       });
 
@@ -158,6 +176,16 @@ describe('Client init()', () => {
 
       // 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);
+
+      expect(beforeStartSpan).toHaveBeenCalledTimes(1);
+      expect(beforeStartSpan).toHaveBeenCalledWith(
+        expect.objectContaining({
+          name: '/',
+          op: 'pageload',
+        }),
+      );
 
       // it is the svelte kit variety
       expect(getActiveSpan()).toBeDefined();
@@ -187,10 +215,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,
         }),
       );
     });
diff --git a/packages/sveltekit/src/client/browserTracingIntegration.ts b/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);
       }
     },
diff --git a/packages/sveltekit/test/client/sdk.test.ts b/packages/sveltekit/test/client/sdk.test.ts
@@ -1,4 +1,11 @@
-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';
@@ -16,6 +23,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 +111,7 @@ describe('Sentry client SDK', () => {
         init({
           dsn: 'https://public@dsn.ingest.sentry.io/1337',
           // eslint-disable-next-line deprecation/deprecation
-          integrations: [new BrowserTracing({ finalTimeout: 10 })],
+          integrations: [new BrowserTracing({ finalTimeout: 10, startTransactionOnLocationChange: false })],
           enableTracing: true,
         });
 
@@ -118,12 +130,14 @@ 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://public@dsn.ingest.sentry.io/1337',
-          integrations: [browserTracingIntegration({ finalTimeout: 10 })],
+          integrations: [browserTracingIntegration({ finalTimeout: 10, instrumentNavigation: false })],
           enableTracing: true,
         });
 
@@ -140,6 +154,8 @@ describe('Sentry client SDK', () => {
 
         // This shows that the user-configured options are still here
         expect(options?.finalTimeout).toEqual(10);
+        expect(options?.instrumentPageLoad).toEqual(true);
+        expect(options?.instrumentNavigation).toEqual(false);
 
         // it is the svelte kit variety
         expect(getActiveSpan()).toBeDefined();
