Skip to content
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.

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

feat(nextjs): Default to VERCEL_ENV as environment #7227

Merged
merged 3 commits into from Feb 20, 2023

Conversation

lforst
Copy link
Member

@lforst lforst commented Feb 20, 2023

Fixes #6993

This PR is just a quick win that should improve the ootb experience for Vercel on Next.js by setting the environment option to a sensible default provided by the hosting provider.

The vercel- prefix is so you can discern the vercel development environment from local development because there the NODE_ENV var with development is likely gonna be picked up.


options.environment =
options.environment ||
(process.env.NEXT_PUBLIC_VERCEL_ENV ? `vercel-${process.env.NEXT_PUBLIC_VERCEL_ENV}` : undefined) ||
Copy link
Member

@mydea mydea Feb 20, 2023

Choose a reason for hiding this comment

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

l: Can we maybe put this into a small util like:

function getVercelEnv(): string | undefined {
  const vercelEnv = process.env.NEXT_PUBLIC_VERCEL_ENV;
  return vercelEnv && `vercel-${vercelEnv}`;
}

Feels better to me IMHO than to sprinkle this through the rest of the code - then we can just do e.g.

options.environment = options.environment || getVercelEnv() || process.env.NODE_ENV;

Copy link
Member Author

Choose a reason for hiding this comment

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

It would just deduplicate this once because we have VERCEL_ENV for node and edge and NEXT_PUBLIC_VERCEL_ENV for the client.

Copy link
Member

Choose a reason for hiding this comment

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

can't we just dynamically chose via a boolean?

I would also like to see this as a func because this is totally something that where we only update two, but forgot to update one (because of merge conflicts, quick fix PR etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Democracy wins :p dfc5d20

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.05 KB (+0.01% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 62.14 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.68 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified) 55.29 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.41 KB (0%)
@sentry/browser - Webpack (minified) 66.73 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.44 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.85 KB (+0.15% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.93 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.2 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 42.57 KB (+0.01% 🔺)
@sentry/replay - Webpack (gzipped + minified) 36.78 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 60.2 KB (+0.01% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 53.8 KB (+0.01% 🔺)

@lforst lforst marked this pull request as ready for review February 20, 2023 09:14
@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2023

Replay SDK metrics 🚀

    Plain +Sentry +Replay
Revision Value Value Diff Ratio Value Diff Ratio
LCP This PR c46c56c 70.62 ms 92.44 ms +21.82 ms +30.89 % 136.08 ms +65.45 ms +92.68 %
Previous 7f4c4ec 78.28 ms 106.41 ms +28.13 ms +35.94 % 134.91 ms +56.64 ms +72.35 %
CLS This PR c46c56c 0.06 ms 0.06 ms -0.00 ms -0.01 % 0.06 ms -0.00 ms -0.49 %
Previous 7f4c4ec 0.06 ms 0.06 ms +0.00 ms +0.03 % 0.06 ms -0.00 ms -0.36 %
CPU This PR c46c56c 12.93 % 13.56 % +0.63 pp +4.89 % 18.31 % +5.38 pp +41.58 %
Previous 7f4c4ec 19.73 % 19.05 % -0.68 pp -3.44 % 25.31 % +5.57 pp +28.25 %
JS heap avg This PR c46c56c 1.94 MB 1.99 MB +51.45 kB +2.65 % 2.87 MB +930.26 kB +47.99 %
Previous 7f4c4ec 1.94 MB 2 MB +58.06 kB +2.99 % 2.87 MB +927.42 kB +47.76 %
JS heap max This PR c46c56c 2.3 MB 2.56 MB +257 kB +11.16 % 3.37 MB +1.07 MB +46.32 %
Previous 7f4c4ec 2.3 MB 2.56 MB +255.81 kB +11.11 % 3.36 MB +1.06 MB +45.97 %
netTx This PR c46c56c 0 B 0 B 0 B n/a 2.21 kB +2.21 kB n/a
Previous 7f4c4ec 0 B 0 B 0 B n/a 2.21 kB +2.21 kB n/a
netRx This PR c46c56c 0 B 0 B 0 B n/a 41 B +41 B n/a
Previous 7f4c4ec 0 B 0 B 0 B n/a 41 B +41 B n/a
netCount This PR c46c56c 0 0 0 n/a 1 +1 n/a
Previous 7f4c4ec 0 0 0 n/a 1 +1 n/a
netTime This PR c46c56c 0.00 ms 0.00 ms 0.00 ms n/a 91.29 ms +91.29 ms n/a
Previous 7f4c4ec 0.00 ms 0.00 ms 0.00 ms n/a 110.83 ms +110.83 ms n/a

Previous results on branch: develop

RevisionLCPCLSCPUJS heap avgJS heap maxnetTxnetRxnetCountnetTime
7f4c4ec+56.64 ms-0.00 ms+5.57 pp+927.42 kB+1.06 MB+2.21 kB+41 B+1+110.83 ms
00d2360+55.18 ms+0.00 ms+2.23 pp+934.14 kB+1.05 MB+2.22 kB+41 B+1+71.65 ms

*) pp - percentage points - an absolute difference between two percentages.
Last updated: Mon, 20 Feb 2023 09:46:38 GMT

Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

i-love-democracy

@lforst lforst merged commit fad6c48 into develop Feb 20, 2023
@lforst lforst deleted the lforst-vercel-env-environment branch February 20, 2023 09:58
@IGassmann
Copy link

@lforst I'm not sure I fully understand the reason behind the prefix. I'm also concerned that it will now mark new production events as vercel-production. Meaning that there will be two production environments in the Sentry dashboard that should actually be the same: production for the old events and vercel-production for new events. I guess that's probably not the expected behavior.

BTW, you can just use NEXT_PUBLIC_VERCEL_ENV. NEXT_PUBLIC_VERCEL_ENV is available both on the browser and on the server.

@lforst
Copy link
Member Author

lforst commented Feb 20, 2023

I'm not sure I fully understand the reason behind the prefix. I'm also concerned that it will now mark new production events as vercel-production. Meaning that there will be two production environments in the Sentry dashboard that should actually be the same: production for the old events and vercel-production for new events. I guess that's probably not the expected behavior.

I say the prefix is useful to discriminate between the vercel environment a build gets deployed to and the local environment where it is likely that NODE_ENV is picked up. You probably don't want local errors to end up in your prod environment filter. There will now be a transition period of max 90 days where there will be two selectable environments but I say this is fine since environments are really just filters and you can select multiple ones even on the free plan.

People will have the option to override this default by setting the environment option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong environment configuration when in Vercel preview environment
4 participants