fix(nextjs): Disable server instrumentation on Vercel #4255
Conversation
iker-barriocanal
requested review from
lobsterkatie,
a team and
sl0thentr0py
and removed request for
a team
size-limit report
|
| // or | ||
| // `node /var/runtime/index.js`. | ||
| const isBuild = new RegExp('build').test(process.argv.toString()); | ||
| const isVercel = !!process.env.VERCEL; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Let's ignore this for now as it's been like this for sometime already and we can address it later.
| // `node next start` | ||
| // or | ||
| // `node /var/runtime/index.js`. | ||
| const isBuild = new RegExp('build').test(process.argv.toString()); |
There was a problem hiding this comment.
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.
| 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?).
The condition to check whether the `vercel` tag is added to vercel is resolved in `index.server.ts` at the module level. The `VERCEL` env var should be set (or not) when the module is imported, instead of importing it and then initing the app.
iker-barriocanal
changed the title
Nothing, it disables server instrumentation on every version. It's relevant to Next 12 because it's the first version that crashes with that instrumentation. |
| }); | ||
| // 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. |
There was a problem hiding this comment.
We can patch the value of isVercel (doesn't have to be now)
…om:getsentry/sentry-javascript into kmclb-nextjs-no-instrumentserver-on-vercel
iker-barriocanal
deleted the
kmclb-nextjs-no-instrumentserver-on-vercel
branch
On Vercel, the server instrumentation has no effect. Errors and transactions are captured through the [custom `_error.js` page](https://docs.sentry.io/platforms/javascript/guides/nextjs/manual-setup/#create-a-custom-_error-page) and the [`withSentry` handler](https://docs.sentry.io/platforms/javascript/guides/nextjs/#configure). Not instrumenting the server (to not import any modules from `next`) when an app is deployed on Vercel should fix the issue.
This work has been done by @lobsterkatie.
Resolves #4207 and #4103.
On Vercel, the server instrumentation has no effect. Errors and transactions are captured through the custom
_error.jspage and thewithSentryhandler. Not instrumenting the server (to not import any modules fromnext) when an app is deployed on Vercel should fix the issue.