Skip to content

Commit

Permalink
ref(nextjs): Stop reinitializing the server SDK unnecessarily (#3860)
Browse files Browse the repository at this point in the history 
In the nextjs SDK, we bundle the user's `Sentry.init()` code with every API route and with the `_app` page, which is the basis of all user-provided pages. This is necessary because, when deployed on Vercel, each API route (and optionally each individual page) is turned into its own serverless function, so each one has to independently be able to start up the SDK.

That said, a) not all nextjs apps are deployed to Vercel, and b) even those that are sometimes have multiple serverless API routes combined into a single serverless function. As a result, `Sentry.init()` can end up getting called over and over again, as different routes are hit on the same running server process. While we manage hubs and clients and scopes in such a way that this doesn't break anything, it's also a total waste of time - the startup code that we bundle with each route comes from a single source (`sentry.server.config.js`) and therefore is the same for every route; once the SDK is started with the given options, it's started. No reason to do it multiple times. 

This thus introduces a check when calling the server-side `Sentry.init()`: if there is a client already defined on the current hub, it means the startup process has already run, and so it bails. 

There are also changes to the tests. TL;DR, because I needed the real `init()` from the node SDK to run (in order to create an already-initialized client to check), I switched the mocking of `@sentry/node` from a direct mock to a pair of spies. I further did two renamings - `s/mockInit/nodeInit`, to make it clear which `init()` function we're talking about, and `s/reactInitOptions/nodeInitOptions`, since this is the backend initialization we're testing rather than the front end.
  • Loading branch information
@lobsterkatie
lobsterkatie authored Aug 3, 2021
1 parent efa6c45 commit 9ca4c5b
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 51 deletions.
21 changes: 20 additions & 1 deletion packages/nextjs/src/index.server.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { RewriteFrames } from '@sentry/integrations';
import { configureScope, init as nodeInit, Integrations } from '@sentry/node';
import { configureScope, getCurrentHub, init as nodeInit, Integrations } from '@sentry/node';
import { logger } from '@sentry/utils';

import { instrumentServer } from './utils/instrumentServer';
import { MetadataBuilder } from './utils/metadataBuilder';
Expand All @@ -14,6 +15,17 @@ export { ErrorBoundary, withErrorBoundary } from '@sentry/react';

/** Inits the Sentry NextJS SDK on node. */
export function init(options: NextjsOptions): void {
if (options.debug) {
logger.enable();
}

logger.log('Initializing SDK...');

if (sdkAlreadyInitialized()) {
logger.log('SDK already initialized');
return;
}

const metadataBuilder = new MetadataBuilder(options, ['nextjs', 'node']);
metadataBuilder.addSdkMetadata();
options.environment = options.environment || process.env.NODE_ENV;
Expand All @@ -26,6 +38,13 @@ export function init(options: NextjsOptions): void {
configureScope(scope => {
scope.setTag('runtime', 'node');
});

logger.log('SDK successfully initialized');
}

function sdkAlreadyInitialized(): boolean {
const hub = getCurrentHub();
return !!hub.getClient();
}

const SOURCEMAP_FILENAME_REGEX = /^.*\/\.next\//;
Expand Down
104 changes: 54 additions & 50 deletions packages/nextjs/test/index.server.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,48 +1,52 @@
import { RewriteFrames } from '@sentry/integrations';
import { Integrations } from '@sentry/node';
import * as SentryNode from '@sentry/node';
import { Integration } from '@sentry/types';
import { getGlobalObject } from '@sentry/utils';

import { init, Scope } from '../src/index.server';
import { NextjsOptions } from '../src/utils/nextjsOptions';

const mockInit = jest.fn();
let configureScopeCallback: (scope: Scope) => void = () => undefined;
const { Integrations } = SentryNode;

jest.mock('@sentry/node', () => {
const actual = jest.requireActual('@sentry/node');
return {
...actual,
init: (options: NextjsOptions) => {
mockInit(options);
},
configureScope: (callback: (scope: Scope) => void) => {
configureScopeCallback = callback;
},
};
});
const global = getGlobalObject();

let configureScopeCallback: (scope: Scope) => void = () => undefined;
jest.spyOn(SentryNode, 'configureScope').mockImplementation(callback => (configureScopeCallback = callback));
const nodeInit = jest.spyOn(SentryNode, 'init');

describe('Server init()', () => {
afterEach(() => {
mockInit.mockClear();
nodeInit.mockClear();
configureScopeCallback = () => undefined;
global.__SENTRY__.hub = undefined;
});

it('inits the Node SDK', () => {
expect(mockInit).toHaveBeenCalledTimes(0);
expect(nodeInit).toHaveBeenCalledTimes(0);
init({});
expect(mockInit).toHaveBeenCalledTimes(1);
expect(mockInit).toHaveBeenLastCalledWith({
_metadata: {
sdk: {
name: 'sentry.javascript.nextjs',
version: expect.any(String),
packages: expect.any(Array),
expect(nodeInit).toHaveBeenCalledTimes(1);
expect(nodeInit).toHaveBeenLastCalledWith(
expect.objectContaining({
_metadata: {
sdk: {
name: 'sentry.javascript.nextjs',
version: expect.any(String),
packages: expect.any(Array),
},
},
},
autoSessionTracking: false,
environment: 'test',
integrations: [expect.any(RewriteFrames)],
});
autoSessionTracking: false,
environment: 'test',
integrations: [expect.any(RewriteFrames)],
}),
);
});

it("doesn't reinitialize the node SDK if already initialized", () => {
expect(nodeInit).toHaveBeenCalledTimes(0);
init({});
expect(nodeInit).toHaveBeenCalledTimes(1);
init({});
expect(nodeInit).toHaveBeenCalledTimes(1);
});

it('sets runtime on scope', () => {
Expand All @@ -57,45 +61,45 @@ describe('Server init()', () => {
it('adds RewriteFrames integration by default', () => {
init({});

const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0];
expect(reactInitOptions.integrations).toHaveLength(1);
const integrations = reactInitOptions.integrations as Integration[];
const nodeInitOptions: NextjsOptions = nodeInit.mock.calls[0][0]!;
expect(nodeInitOptions.integrations).toHaveLength(1);
const integrations = nodeInitOptions.integrations as Integration[];
expect(integrations[0]).toEqual(expect.any(RewriteFrames));
});

it('adds Http integration by default if tracesSampleRate is set', () => {
init({ tracesSampleRate: 1.0 });

const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0];
expect(reactInitOptions.integrations).toHaveLength(2);
const integrations = reactInitOptions.integrations as Integration[];
const nodeInitOptions: NextjsOptions = nodeInit.mock.calls[0][0]!;
expect(nodeInitOptions.integrations).toHaveLength(2);
const integrations = nodeInitOptions.integrations as Integration[];
expect(integrations[1]).toEqual(expect.any(Integrations.Http));
});

it('adds Http integration by default if tracesSampler is set', () => {
init({ tracesSampler: () => true });

const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0];
expect(reactInitOptions.integrations).toHaveLength(2);
const integrations = reactInitOptions.integrations as Integration[];
const nodeInitOptions: NextjsOptions = nodeInit.mock.calls[0][0]!;
expect(nodeInitOptions.integrations).toHaveLength(2);
const integrations = nodeInitOptions.integrations as Integration[];
expect(integrations[1]).toEqual(expect.any(Integrations.Http));
});

it('adds Http integration with tracing true', () => {
init({ tracesSampleRate: 1.0 });
const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0];
expect(reactInitOptions.integrations).toHaveLength(2);
const nodeInitOptions: NextjsOptions = nodeInit.mock.calls[0][0]!;
expect(nodeInitOptions.integrations).toHaveLength(2);

const integrations = reactInitOptions.integrations as Integration[];
const integrations = nodeInitOptions.integrations as Integration[];
expect((integrations[1] as any)._tracing).toBe(true);
});

it('supports passing integration through options', () => {
init({ tracesSampleRate: 1.0, integrations: [new Integrations.Console()] });
const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0];
expect(reactInitOptions.integrations).toHaveLength(3);
const nodeInitOptions: NextjsOptions = nodeInit.mock.calls[0][0]!;
expect(nodeInitOptions.integrations).toHaveLength(3);

const integrations = reactInitOptions.integrations as Integration[];
const integrations = nodeInitOptions.integrations as Integration[];
expect(integrations).toEqual([
expect.any(Integrations.Console),
expect.any(RewriteFrames),
Expand All @@ -110,9 +114,9 @@ describe('Server init()', () => {
integrations: [new Integrations.Http({ tracing: false })],
});

const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0];
expect(reactInitOptions.integrations).toHaveLength(2);
const integrations = reactInitOptions.integrations as Integration[];
const nodeInitOptions: NextjsOptions = nodeInit.mock.calls[0][0]!;
expect(nodeInitOptions.integrations).toHaveLength(2);
const integrations = nodeInitOptions.integrations as Integration[];
expect(integrations[0] as InstanceType<typeof Integrations.Http>).toEqual(
expect.objectContaining({ _breadcrumbs: true, _tracing: true, name: 'Http' }),
);
Expand All @@ -124,9 +128,9 @@ describe('Server init()', () => {
integrations: [new Integrations.Http({ tracing: false })],
});

const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0];
expect(reactInitOptions.integrations).toHaveLength(2);
const integrations = reactInitOptions.integrations as Integration[];
const nodeInitOptions: NextjsOptions = nodeInit.mock.calls[0][0]!;
expect(nodeInitOptions.integrations).toHaveLength(2);
const integrations = nodeInitOptions.integrations as Integration[];
expect(integrations[0] as InstanceType<typeof Integrations.Http>).toEqual(
expect.objectContaining({ _breadcrumbs: true, _tracing: true, name: 'Http' }),
);
Expand Down

0 comments on commit 9ca4c5b

Please sign in to comment.