From 29a72d843d045dc5692a4b67c57a3da0883fae40 Mon Sep 17 00:00:00 2001 From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Fri, 21 May 2021 18:01:40 +0200 Subject: [PATCH 1/4] feat(nextjs): Automatic performance monitoring --- packages/nextjs/src/index.client.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/nextjs/src/index.client.ts b/packages/nextjs/src/index.client.ts index db1aba8413fe..3b1551806cea 100644 --- a/packages/nextjs/src/index.client.ts +++ b/packages/nextjs/src/index.client.ts @@ -1,7 +1,10 @@ import { configureScope, init as reactInit } from '@sentry/react'; +import { Integrations } from '@sentry/tracing'; +import { nextRouterInstrumentation } from './performance/client'; import { MetadataBuilder } from './utils/metadataBuilder'; import { NextjsOptions } from './utils/nextjsOptions'; +import { addIntegration } from './utils/userIntegrations'; export * from '@sentry/react'; export { nextRouterInstrumentation } from './performance/client'; @@ -11,8 +14,21 @@ export function init(options: NextjsOptions): void { const metadataBuilder = new MetadataBuilder(options, ['nextjs', 'react']); metadataBuilder.addSdkMetadata(); options.environment = options.environment || process.env.NODE_ENV; + addClientIntegrations(options); reactInit(options); configureScope(scope => { scope.setTag('runtime', 'browser'); }); } + +const defaultBrowserTracingIntegration = new Integrations.BrowserTracing({ + routingInstrumentation: nextRouterInstrumentation, +}); + +function addClientIntegrations(options: NextjsOptions): void { + if (options.integrations) { + addIntegration(defaultBrowserTracingIntegration, options.integrations); + } else { + options.integrations = [defaultBrowserTracingIntegration]; + } +} From 45d478178cb99009f48ecc815ed1fa875fd32ebf Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 21 May 2021 14:20:58 -0400 Subject: [PATCH 2/4] test: Write tests for auto client perf --- packages/nextjs/src/index.client.ts | 28 ++++-- packages/nextjs/src/utils/userIntegrations.ts | 2 +- packages/nextjs/test/index.client.test.ts | 97 +++++++++++++++++++ .../test/{ => utils}/userIntegrations.test.ts | 2 +- 4 files changed, 119 insertions(+), 10 deletions(-) create mode 100644 packages/nextjs/test/index.client.test.ts rename packages/nextjs/test/{ => utils}/userIntegrations.test.ts (95%) diff --git a/packages/nextjs/src/index.client.ts b/packages/nextjs/src/index.client.ts index 3b1551806cea..022963cb27ef 100644 --- a/packages/nextjs/src/index.client.ts +++ b/packages/nextjs/src/index.client.ts @@ -4,31 +4,43 @@ import { Integrations } from '@sentry/tracing'; import { nextRouterInstrumentation } from './performance/client'; import { MetadataBuilder } from './utils/metadataBuilder'; import { NextjsOptions } from './utils/nextjsOptions'; -import { addIntegration } from './utils/userIntegrations'; +import { addIntegration, UserIntegrations } from './utils/userIntegrations'; export * from '@sentry/react'; export { nextRouterInstrumentation } from './performance/client'; +const { BrowserTracing } = Integrations; + /** Inits the Sentry NextJS SDK on the browser with the React SDK. */ export function init(options: NextjsOptions): void { const metadataBuilder = new MetadataBuilder(options, ['nextjs', 'react']); metadataBuilder.addSdkMetadata(); options.environment = options.environment || process.env.NODE_ENV; - addClientIntegrations(options); - reactInit(options); + reactInit({ + ...options, + integrations: createClientIntegrations(options.integrations), + }); configureScope(scope => { scope.setTag('runtime', 'browser'); }); } -const defaultBrowserTracingIntegration = new Integrations.BrowserTracing({ +const defaultBrowserTracingIntegration = new BrowserTracing({ routingInstrumentation: nextRouterInstrumentation, }); -function addClientIntegrations(options: NextjsOptions): void { - if (options.integrations) { - addIntegration(defaultBrowserTracingIntegration, options.integrations); +function createClientIntegrations(integrations?: UserIntegrations): UserIntegrations { + if (integrations) { + const newIntegrations = addIntegration(defaultBrowserTracingIntegration, integrations); + if (Array.isArray(newIntegrations)) { + newIntegrations.forEach(i => { + if (i.name === 'BrowserTracing') { + (i as InstanceType).options.routingInstrumentation = nextRouterInstrumentation; + } + }); + } + return newIntegrations; } else { - options.integrations = [defaultBrowserTracingIntegration]; + return [defaultBrowserTracingIntegration]; } } diff --git a/packages/nextjs/src/utils/userIntegrations.ts b/packages/nextjs/src/utils/userIntegrations.ts index 006b5ebc8971..0e2bdc0d2bc8 100644 --- a/packages/nextjs/src/utils/userIntegrations.ts +++ b/packages/nextjs/src/utils/userIntegrations.ts @@ -1,7 +1,7 @@ import { Integration } from '@sentry/types'; export type UserFunctionIntegrations = (integrations: Integration[]) => Integration[]; -type UserIntegrations = Integration[] | UserFunctionIntegrations; +export type UserIntegrations = Integration[] | UserFunctionIntegrations; /** * Retrieves the patched integrations with the provided integration. diff --git a/packages/nextjs/test/index.client.test.ts b/packages/nextjs/test/index.client.test.ts new file mode 100644 index 000000000000..46a129b34447 --- /dev/null +++ b/packages/nextjs/test/index.client.test.ts @@ -0,0 +1,97 @@ +import { Integrations as TracingIntegrations } from '@sentry/tracing'; +import { Integration } from '@sentry/types'; + +import { init, Integrations, nextRouterInstrumentation, Scope } from '../src/index.client'; +import { NextjsOptions } from '../src/utils/nextjsOptions'; + +const { BrowserTracing } = TracingIntegrations; + +const mockInit = jest.fn(); +let configureScopeCallback: (scope: Scope) => void = () => undefined; + +jest.mock('@sentry/react', () => { + const actual = jest.requireActual('@sentry/react'); + return { + ...actual, + init: (options: NextjsOptions) => { + mockInit(options); + }, + configureScope: (callback: (scope: Scope) => void) => { + configureScopeCallback = callback; + }, + }; +}); + +describe('Client init()', () => { + afterEach(() => { + mockInit.mockClear(); + configureScopeCallback = () => undefined; + }); + + it('inits the React SDK', () => { + expect(mockInit).toHaveBeenCalledTimes(0); + init({}); + expect(mockInit).toHaveBeenCalledTimes(1); + expect(mockInit).toHaveBeenLastCalledWith({ + _metadata: { + sdk: { + name: 'sentry.javascript.nextjs', + version: expect.any(String), + packages: expect.any(Array), + }, + }, + environment: 'test', + integrations: expect.any(Array), + }); + }); + + it('sets runtime on scope', () => { + const mockScope = new Scope(); + init({}); + configureScopeCallback(mockScope); + // @ts-ignore need access to protected _tags attribute + expect(mockScope._tags).toEqual({ runtime: 'browser' }); + }); + + describe('integrations', () => { + it('adds BrowserTracing integration by default', () => { + init({}); + + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; + expect(reactInitOptions.integrations).toHaveLength(1); + + const integrations = reactInitOptions.integrations as Integration[]; + expect(integrations[0]).toEqual(expect.any(BrowserTracing)); + // eslint-disable-next-line @typescript-eslint/unbound-method + expect((integrations[0] as InstanceType).options.routingInstrumentation).toEqual( + nextRouterInstrumentation, + ); + }); + + it('supports passing integration through options', () => { + init({ integrations: [new Integrations.Breadcrumbs({ console: false })] }); + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; + expect(reactInitOptions.integrations).toHaveLength(2); + + const integrations = reactInitOptions.integrations as Integration[]; + expect(integrations).toEqual([expect.any(Integrations.Breadcrumbs), expect.any(BrowserTracing)]); + }); + + it('uses custom BrowserTracing but uses nextRouterInstrumentation', () => { + init({ + integrations: [new BrowserTracing({ idleTimeout: 5000, startTransactionOnLocationChange: false })], + }); + + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; + expect(reactInitOptions.integrations).toHaveLength(1); + const integrations = reactInitOptions.integrations as Integration[]; + expect((integrations[0] as InstanceType).options).toEqual( + expect.objectContaining({ + idleTimeout: 5000, + startTransactionOnLocationChange: false, + routingInstrumentation: nextRouterInstrumentation, + }), + ); + }); + }); +}); diff --git a/packages/nextjs/test/userIntegrations.test.ts b/packages/nextjs/test/utils/userIntegrations.test.ts similarity index 95% rename from packages/nextjs/test/userIntegrations.test.ts rename to packages/nextjs/test/utils/userIntegrations.test.ts index 011143151a45..b6f603be5cd0 100644 --- a/packages/nextjs/test/userIntegrations.test.ts +++ b/packages/nextjs/test/utils/userIntegrations.test.ts @@ -1,7 +1,7 @@ import { RewriteFrames } from '@sentry/integrations'; import { Integration } from '@sentry/types'; -import { addIntegration, UserFunctionIntegrations } from '../src/utils/userIntegrations'; +import { addIntegration, UserFunctionIntegrations } from '../../src/utils/userIntegrations'; const testIntegration = new RewriteFrames(); From 5f05c5b4ab70b2617a6e5c1925d3f516b3af284b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 21 May 2021 15:38:52 -0400 Subject: [PATCH 3/4] ref: Always use nextRouterInstrumentation For ease of use, I make it so that for client-side tracing nextRouterInstrumentation is always turned on --- packages/nextjs/src/index.client.ts | 12 +--- packages/nextjs/src/utils/userIntegrations.ts | 64 +++++++++++++++++-- packages/nextjs/test/index.client.test.ts | 19 +++++- 3 files changed, 79 insertions(+), 16 deletions(-) diff --git a/packages/nextjs/src/index.client.ts b/packages/nextjs/src/index.client.ts index 022963cb27ef..d3d301cecf65 100644 --- a/packages/nextjs/src/index.client.ts +++ b/packages/nextjs/src/index.client.ts @@ -31,15 +31,9 @@ const defaultBrowserTracingIntegration = new BrowserTracing({ function createClientIntegrations(integrations?: UserIntegrations): UserIntegrations { if (integrations) { - const newIntegrations = addIntegration(defaultBrowserTracingIntegration, integrations); - if (Array.isArray(newIntegrations)) { - newIntegrations.forEach(i => { - if (i.name === 'BrowserTracing') { - (i as InstanceType).options.routingInstrumentation = nextRouterInstrumentation; - } - }); - } - return newIntegrations; + return addIntegration(defaultBrowserTracingIntegration, integrations, { + BrowserTracing: { keyPath: 'options.routingInstrumentation', value: nextRouterInstrumentation }, + }); } else { return [defaultBrowserTracingIntegration]; } diff --git a/packages/nextjs/src/utils/userIntegrations.ts b/packages/nextjs/src/utils/userIntegrations.ts index 0e2bdc0d2bc8..9f45e6c3d362 100644 --- a/packages/nextjs/src/utils/userIntegrations.ts +++ b/packages/nextjs/src/utils/userIntegrations.ts @@ -3,6 +3,35 @@ import { Integration } from '@sentry/types'; export type UserFunctionIntegrations = (integrations: Integration[]) => Integration[]; export type UserIntegrations = Integration[] | UserFunctionIntegrations; +type Options = { + [integrationName: string]: + | { + keyPath: string; + value: unknown; + } + | undefined; +}; + +/** + * Recursively traverses an object to update an existing nested key. + * Note: The provided key path must include existing properties, + * the function will not create objects while traversing. + * + * @param obj An object to update + * @param value The value to update the nested key with + * @param keyPath The path to the key to update ex. fizz.buzz.foo + */ +// eslint-disable-next-line @typescript-eslint/no-explicit-any +function setNestedKey(obj: Record, keyPath: string, value: unknown): void { + // Ex. foo.bar.zoop will extract foo and bar.zoop + const match = keyPath.match(/([a-z]+)\.(.*)/i); + if (match === null) { + obj[keyPath] = value; + } else { + setNestedKey(obj[match[1]], match[2], value); + } +} + /** * Retrieves the patched integrations with the provided integration. * @@ -12,18 +41,40 @@ export type UserIntegrations = Integration[] | UserFunctionIntegrations; * * @param integration The integration to patch, if necessary. * @param userIntegrations Integrations defined by the user. + * @param options options to update for a particular integration * @returns Final integrations, patched if necessary. */ -export function addIntegration(integration: Integration, userIntegrations: UserIntegrations): UserIntegrations { +export function addIntegration( + integration: Integration, + userIntegrations: UserIntegrations, + options: Options = {}, +): UserIntegrations { if (Array.isArray(userIntegrations)) { - return addIntegrationToArray(integration, userIntegrations); + return addIntegrationToArray(integration, userIntegrations, options); } else { - return addIntegrationToFunction(integration, userIntegrations); + return addIntegrationToFunction(integration, userIntegrations, options); } } -function addIntegrationToArray(integration: Integration, userIntegrations: Integration[]): Integration[] { - if (userIntegrations.map(int => int.name).includes(integration.name)) { +function addIntegrationToArray( + integration: Integration, + userIntegrations: Integration[], + options: Options, +): Integration[] { + let includesName = false; + // eslint-disable-next-line @typescript-eslint/prefer-for-of + for (let x = 0; x < userIntegrations.length; x++) { + if (userIntegrations[x].name === integration.name) { + includesName = true; + } + + const op = options[userIntegrations[x].name]; + if (op) { + setNestedKey(userIntegrations[x], op.keyPath, op.value); + } + } + + if (includesName) { return userIntegrations; } return [...userIntegrations, integration]; @@ -32,10 +83,11 @@ function addIntegrationToArray(integration: Integration, userIntegrations: Integ function addIntegrationToFunction( integration: Integration, userIntegrationsFunc: UserFunctionIntegrations, + options: Options, ): UserFunctionIntegrations { const wrapper: UserFunctionIntegrations = defaultIntegrations => { const userFinalIntegrations = userIntegrationsFunc(defaultIntegrations); - return addIntegrationToArray(integration, userFinalIntegrations); + return addIntegrationToArray(integration, userFinalIntegrations, options); }; return wrapper; } diff --git a/packages/nextjs/test/index.client.test.ts b/packages/nextjs/test/index.client.test.ts index 46a129b34447..dadd780c0900 100644 --- a/packages/nextjs/test/index.client.test.ts +++ b/packages/nextjs/test/index.client.test.ts @@ -77,7 +77,7 @@ describe('Client init()', () => { expect(integrations).toEqual([expect.any(Integrations.Breadcrumbs), expect.any(BrowserTracing)]); }); - it('uses custom BrowserTracing but uses nextRouterInstrumentation', () => { + it('uses custom BrowserTracing with array option with nextRouterInstrumentation', () => { init({ integrations: [new BrowserTracing({ idleTimeout: 5000, startTransactionOnLocationChange: false })], }); @@ -93,5 +93,22 @@ describe('Client init()', () => { }), ); }); + + it('uses custom BrowserTracing with function option with nextRouterInstrumentation', () => { + init({ + integrations: () => [new BrowserTracing({ idleTimeout: 5000, startTransactionOnLocationChange: false })], + }); + + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; + const integrationFunc = reactInitOptions.integrations as () => Integration[]; + const integrations = integrationFunc(); + expect((integrations[0] as InstanceType).options).toEqual( + expect.objectContaining({ + idleTimeout: 5000, + startTransactionOnLocationChange: false, + routingInstrumentation: nextRouterInstrumentation, + }), + ); + }); }); }); From ada791819a6090478f78e8da655c4d42a7611a4d Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 25 May 2021 12:13:23 -0400 Subject: [PATCH 4/4] fix: Only enable BrowserTracing if sampling rate is set --- packages/nextjs/src/index.client.ts | 9 ++++++- packages/nextjs/test/index.client.test.ts | 29 ++++++++++++++++++++--- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/src/index.client.ts b/packages/nextjs/src/index.client.ts index d3d301cecf65..7a55b4d0bce2 100644 --- a/packages/nextjs/src/index.client.ts +++ b/packages/nextjs/src/index.client.ts @@ -16,9 +16,16 @@ export function init(options: NextjsOptions): void { const metadataBuilder = new MetadataBuilder(options, ['nextjs', 'react']); metadataBuilder.addSdkMetadata(); options.environment = options.environment || process.env.NODE_ENV; + + // Only add BrowserTracing if a tracesSampleRate or tracesSampler is set + const integrations = + options.tracesSampleRate === undefined && options.tracesSampler === undefined + ? options.integrations + : createClientIntegrations(options.integrations); + reactInit({ ...options, - integrations: createClientIntegrations(options.integrations), + integrations, }); configureScope(scope => { scope.setTag('runtime', 'browser'); diff --git a/packages/nextjs/test/index.client.test.ts b/packages/nextjs/test/index.client.test.ts index dadd780c0900..738ecc24dc89 100644 --- a/packages/nextjs/test/index.client.test.ts +++ b/packages/nextjs/test/index.client.test.ts @@ -41,7 +41,7 @@ describe('Client init()', () => { }, }, environment: 'test', - integrations: expect.any(Array), + integrations: undefined, }); }); @@ -54,9 +54,30 @@ describe('Client init()', () => { }); describe('integrations', () => { - it('adds BrowserTracing integration by default', () => { + it('does not add BrowserTracing integration by default if tracesSampleRate is not set', () => { init({}); + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; + expect(reactInitOptions.integrations).toBeUndefined(); + }); + + it('adds BrowserTracing integration by default if tracesSampleRate is set', () => { + init({ tracesSampleRate: 1.0 }); + + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; + expect(reactInitOptions.integrations).toHaveLength(1); + + const integrations = reactInitOptions.integrations as Integration[]; + expect(integrations[0]).toEqual(expect.any(BrowserTracing)); + // eslint-disable-next-line @typescript-eslint/unbound-method + expect((integrations[0] as InstanceType).options.routingInstrumentation).toEqual( + nextRouterInstrumentation, + ); + }); + + it('adds BrowserTracing integration by default if tracesSampler is set', () => { + init({ tracesSampler: () => true }); + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; expect(reactInitOptions.integrations).toHaveLength(1); @@ -69,7 +90,7 @@ describe('Client init()', () => { }); it('supports passing integration through options', () => { - init({ integrations: [new Integrations.Breadcrumbs({ console: false })] }); + init({ tracesSampleRate: 1.0, integrations: [new Integrations.Breadcrumbs({ console: false })] }); const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; expect(reactInitOptions.integrations).toHaveLength(2); @@ -79,6 +100,7 @@ describe('Client init()', () => { it('uses custom BrowserTracing with array option with nextRouterInstrumentation', () => { init({ + tracesSampleRate: 1.0, integrations: [new BrowserTracing({ idleTimeout: 5000, startTransactionOnLocationChange: false })], }); @@ -96,6 +118,7 @@ describe('Client init()', () => { it('uses custom BrowserTracing with function option with nextRouterInstrumentation', () => { init({ + tracesSampleRate: 1.0, integrations: () => [new BrowserTracing({ idleTimeout: 5000, startTransactionOnLocationChange: false })], });