Skip to content

Commit

Permalink
fix(nextjs|sveltekit/v7): Ensure we can pass `browserTracingIntegrati…
Browse files Browse the repository at this point in the history 
…on` (#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
  • Loading branch information
@mydea
mydea committed Apr 24, 2024
1 parent cfaa9be commit 5d045eb
Show file tree
Hide file tree
Showing 5 changed files with 156 additions and 22 deletions.
3 changes: 3 additions & 0 deletions dev-packages/e2e-tests/test-applications/nextjs-14/sentry.client.config.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()],
});
16 changes: 12 additions & 4 deletions packages/nextjs/src/client/browserTracingIntegration.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -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:
Expand All @@ -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);
Expand All @@ -80,7 +88,7 @@ export function browserTracingIntegration(
nextRouterInstrumentation(
() => undefined,
false,
options?.instrumentNavigation,
fullOptions.instrumentNavigation,
startPageloadCallback,
startNavigationCallback,
);
Expand All @@ -90,7 +98,7 @@ export function browserTracingIntegration(
// eslint-disable-next-line deprecation/deprecation
nextRouterInstrumentation(
() => undefined,
options?.instrumentPageLoad,
fullOptions.instrumentPageLoad,
false,
startPageloadCallback,
startNavigationCallback,
Expand Down
82 changes: 75 additions & 7 deletions packages/nextjs/test/clientSdk.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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');
Expand Down Expand Up @@ -35,6 +47,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;
Expand Down Expand Up @@ -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,
});

Expand All @@ -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', () => {
Expand All @@ -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,
}),
);
});
Expand Down
16 changes: 12 additions & 4 deletions packages/sveltekit/src/client/browserTracingIntegration.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand All @@ -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,
Expand All @@ -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);
}
},
Expand Down
61 changes: 54 additions & 7 deletions packages/sveltekit/test/client/sdk.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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');
Expand All @@ -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();

Expand Down Expand Up @@ -99,7 +115,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,
});

Expand All @@ -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://public@dsn.ingest.sentry.io/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://public@dsn.ingest.sentry.io/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);
});
});
});
Expand Down

0 comments on commit 5d045eb

Please sign in to comment.