Skip to content

fix(nextjs): Disable server instrumentation on Vercel #4255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Dec 10, 2021
Merged

fix(nextjs): Disable server instrumentation on Vercel #4255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
43fb013
only call instrumentServer at runtime and off of vercel
lobsterkatie Dec 7, 2021
1a69729
Merge branch 'master' into kmclb-nextjs-no-instrumentserver-on-vercel
iker-barriocanal Dec 10, 2021
586f120
Remove test to check `vercel` tag when running on vercel
iker-barriocanal Dec 10, 2021
2aa7519
Delete `VERCEL` env var after each unit test
iker-barriocanal Dec 10, 2021
9fa8b1b
Clean up comments
rhcarvalho Dec 10, 2021
ededc3f
Import TS file if JS file doesn't exist, on server instrumentation
iker-barriocanal Dec 10, 2021
6b8f494
Merge branch 'kmclb-nextjs-no-instrumentserver-on-vercel' of github.c…
iker-barriocanal Dec 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 33 additions & 4 deletions packages/nextjs/src/index.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { escapeStringForRegex, logger } from '@sentry/utils';
import * as domainModule from 'domain';
import * as path from 'path';

import { instrumentServer } from './utils/instrumentServer';
import { MetadataBuilder } from './utils/metadataBuilder';
import { NextjsOptions } from './utils/nextjsOptions';
import { addIntegration } from './utils/userIntegrations';
Expand All @@ -20,6 +19,17 @@ export { ErrorBoundary, withErrorBoundary } from '@sentry/react';
type GlobalWithDistDir = typeof global & { __rewriteFramesDistDir__: string };
const domain = domainModule as typeof domainModule & { active: (domainModule.Domain & Carrier) | null };

// During build, the main process is invoked by
// `node next build`
// and child processes are invoked as
// `node <path>/node_modules/jest-worker/build/workers/processChild.js`,
// whereas at runtime the process is invoked as
// `node next start`
// or
// `node /var/runtime/index.js`.
const isBuild = new RegExp('build').test(process.argv.toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern is a fixed string, there's no need to use a RegExp -- we could use String.indexOf or String.includes.

Also, process.argv.toString() seems brittle -- this could produce the incorrect output if "build" appears somewhere on the path / args by chance.

Suggested change
const isBuild = new RegExp('build').test(process.argv.toString());
const isBuild = process.argv.toString().indexOf('build') >= 0;

I could give a better suggestion once I understand what exactly was the intent here (are we trying to cover the first and second cases of the comment?).

const isVercel = !!process.env.VERCEL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we look for VERCEL=1 specifically? !!process.env.VERCEL means isVercel is true as long as the environment variable is defined (even for values that would be surprising to consider true, such as VERCEL=0, VERCEL=no and VERCEL=false).

https://vercel.com/docs/concepts/projects/environment-variables#system-environment-variables

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ignore this for now as it's been like this for sometime already and we can address it later.


/** Inits the Sentry NextJS SDK on node. */
export function init(options: NextjsOptions): void {
if (options.debug) {
Expand Down Expand Up @@ -54,7 +64,7 @@ export function init(options: NextjsOptions): void {

configureScope(scope => {
scope.setTag('runtime', 'node');
if (process.env.VERCEL) {
if (isVercel) {
scope.setTag('vercel', true);
}

Expand Down Expand Up @@ -119,5 +129,24 @@ function filterTransactions(event: Event): Event | null {
export { withSentryConfig } from './config';
export { withSentry } from './utils/withSentry';

// wrap various server methods to enable error monitoring and tracing
instrumentServer();
// Wrap various server methods to enable error monitoring and tracing. (Note: This only happens for non-Vercel
// deployments, because the current method of doing the wrapping a) crashes Next 12 apps deployed to Vercel and
// b) doesn't work on those apps anyway. We also don't do it during build, because there's no server running in that
// phase.)
if (!isVercel && !isBuild) {
// Dynamically require the file because even importing from it causes Next 12 to crash on Vercel.
// In environments where the JS file doesn't exist, such as testing, import the TS file.
try {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { instrumentServer } = require('./utils/instrumentServer.js');
instrumentServer();
} catch {
try {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { instrumentServer } = require('./utils/instrumentServer.ts');
instrumentServer();
} catch {
// Server not instrumented. Not adding logs to avoid noise.
}
}
}
16 changes: 4 additions & 12 deletions packages/nextjs/test/index.server.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ describe('Server init()', () => {
afterEach(() => {
jest.clearAllMocks();
global.__SENTRY__.hub = undefined;
delete process.env.VERCEL;
});

it('inits the Node SDK', () => {
Expand Down Expand Up @@ -66,18 +67,9 @@ describe('Server init()', () => {
expect(currentScope._tags).toEqual({ runtime: 'node' });
});

it('applies `vercel` tag when running on vercel', () => {
const currentScope = getCurrentHub().getScope();

process.env.VERCEL = '1';

init({});

// @ts-ignore need access to protected _tags attribute
expect(currentScope._tags.vercel).toEqual(true);

delete process.env.VERCEL;
});
// TODO: test `vercel` tag when running on Vercel
// Can't just add the test and set env variables, since the value in `index.server.ts`
// is resolved when importing.
Copy link
Contributor

@rhcarvalho rhcarvalho Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can patch the value of isVercel (doesn't have to be now)


it('does not apply `vercel` tag when not running on vercel', () => {
const currentScope = getCurrentHub().getScope();
Expand Down