From c62591f738c11f4f2c43803a769ed5e832b4817a Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 24 Apr 2024 10:08:45 +0200 Subject: [PATCH 1/2] fix(nextjs|sveltekit): Ensure we can pass browserTracingIntegration more improvements add one e2e test add test for beforestartspan --- .../nextjs-14/sentry.client.config.ts | 3 ++ .../src/client/browserTracingIntegration.ts | 16 ++++++--- packages/nextjs/test/clientSdk.test.ts | 36 ++++++++++++++++--- .../src/client/browserTracingIntegration.ts | 16 ++++++--- packages/sveltekit/test/client/sdk.test.ts | 22 ++++++++++-- 5 files changed, 78 insertions(+), 15 deletions(-) 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 index 85bd765c9c44..0e3922a2b758 100644 --- 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 index af8f59f53b6f..171ecef7efd2 100644 --- 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[0], -): Integration { +): ReturnType { 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 index 1ac61f687e88..50dcfe06db0a 100644 --- 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 index 5e80cdc92f28..7204e2186da4 100644 --- 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[0] = {}, -): Integration { +): ReturnType { 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 index 96d177a5cd84..16c9dc12b41f 100644 --- 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(); From 3d3fdd1ca94467c6968c649b101c110818e6269e Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 24 Apr 2024 10:44:41 +0200 Subject: [PATCH 2/2] more tests --- packages/nextjs/test/clientSdk.test.ts | 54 +++++++++++++++++++--- packages/sveltekit/test/client/sdk.test.ts | 43 ++++++++++++++--- 2 files changed, 84 insertions(+), 13 deletions(-) diff --git a/packages/nextjs/test/clientSdk.test.ts b/packages/nextjs/test/clientSdk.test.ts index 50dcfe06db0a..a670f64f2d0c 100644 --- a/packages/nextjs/test/clientSdk.test.ts +++ b/packages/nextjs/test/clientSdk.test.ts @@ -13,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'); @@ -174,11 +180,7 @@ describe('Client init()', () => { // It is a "new" browser tracing integration expect(typeof integration?.afterAllSetup).toBe('function'); - // 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); - + // the hooks is correctly invoked expect(beforeStartSpan).toHaveBeenCalledTimes(1); expect(beforeStartSpan).toHaveBeenCalledWith( expect.objectContaining({ @@ -187,11 +189,49 @@ describe('Client init()', () => { }), ); - // it is the svelte kit variety + // 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.instrumentNavigation).toBe(false); + expect(integration?.options.instrumentPageLoad).toBe(true); + }); + + 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()!; + // eslint-disable-next-line deprecation/deprecation + const integration = client.getIntegrationByName>('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', () => { diff --git a/packages/sveltekit/test/client/sdk.test.ts b/packages/sveltekit/test/client/sdk.test.ts index 16c9dc12b41f..747e4c901417 100644 --- a/packages/sveltekit/test/client/sdk.test.ts +++ b/packages/sveltekit/test/client/sdk.test.ts @@ -11,7 +11,11 @@ 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'); @@ -145,23 +149,50 @@ describe('Sentry client SDK', () => { getClient()?.getIntegrationByName>( '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(options?.instrumentPageLoad).toEqual(true); - expect(options?.instrumentNavigation).toEqual(false); + 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://public@dsn.ingest.sentry.io/1337', + integrations: [sveltekitBrowserTracingIntegration({ finalTimeout: 10, instrumentNavigation: false })], + enableTracing: true, + }); + + const client = getClient()!; + // eslint-disable-next-line deprecation/deprecation + const integration = client.getIntegrationByName>('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); }); }); });