[@sentry/nextjs] @sentry/cli always bundled in production

belgattitude commented 3 years ago

[x] Review the documentation: https://docs.sentry.io/
[x] Search for existing issues: https://github.com/getsentry/sentry-javascript/issues
[x] Use the latest release: https://github.com/getsentry/sentry-javascript/releases
[x] Provide a link to the affected event from your Sentry account

Package + Version

[x] @sentry/nextjs - v 6.10.0
- [x] @sentry/webpack-plugin - v 1.16.0

Version:

6.10.0

Description

Right now the @sentry/webpack-plugin (/ @sentry/cli) is required as regular dep, meaning we can't get rid off it in production.

This makes prod bundle much bigger and lead to

Exceeding lambdas limit of 50Mb (in this case the lambda deployment would fail)
Lower performance and slow cold start for lambdas (and maybe some 504...)
Longer and (non-cacheable) installs (needs to download binary at build)

As an example, see a vercel debug based on an example repo https://github.com/belgattitude/nextjs-monorepo-example

Edit: Try a deploy on vercel and check the log with NEXT_DEBUG_FUNCTION_SIZE=1

00:42:46.970 | Serverless Function's pages: api/graphql-sdl.js, api/hello.js, api/rest/poem.js, api/rest/post.js, api/rest/post/[id].js
-- | --
00:42:46.980 | Large Dependencies                    Uncompressed size  Compressed size
00:42:46.980 | node_modules/.prisma/client                     43.4 MB          15.2 MB
00:42:46.980 | node_modules/sharp/vendor                       18.4 MB          7.66 MB
00:42:46.980 | node_modules/@sentry/cli                        17.9 MB          6.78 MB
00:42:46.980 | node_modules/next/dist                          31.9 MB          6.33 MB
00:42:46.980 | node_modules/sass/sass.dart.js                  4.09 MB           610 kB
00:42:46.980 | node_modules/@mui/material                      1.84 MB           567 kB
00:42:46.981 | node_modules/@prisma/client                     1.66 MB           336 kB
00:42:46.981 | node_modules/react-dom/cjs                      1.18 MB           289 kB
00:42:46.981 | node_modules/caniuse-lite/data                   719 kB           278 kB
00:42:46.981 | node_modules/next/node_modules                   740 kB           194 kB
00:42:46.981 | node_modules/micro/node_modules                  360 kB           190 kB
00:42:46.981 | node_modules/encoding/node_modules               329 kB           179 kB
00:42:46.981 | node_modules/iconv-lite/encodings                303 kB           172 kB
00:42:46.981 | node_modules/@next/react-dev-overlay             450 kB           131 kB
00:42:46.981 | node_modules/styled-jsx/node_modules             570 kB           110 kB
00:42:46.981 | All dependencies                                 133 MB          41.4 MB

Fixing

It's not totally clear to me what would be the best to do.

Option 1:

@sentry/nextjs could move @sentry/html-plugin to devDependencies (or peerDeps/optional), but I'm not sure as it's included in nextjs.config.js so maybe need some more work to be sure the plugin is not required for production builds. A proposal here: https://github.com/getsentry/sentry-javascript/pull/3866

Option 2:

@sentry/html-webpack plugin, move @sentry/cli to devDependencie (or peerDeps/optional)... That would be sufficient as the it's actually the one that increase size significantly (17Mb).

Edit

Option 3:

Ditch @sentry/cli in favour of a custom lightweight sourcemap uploader (That would be the more universal solution)

Option 4:

Possible too, add an option to vercel/nft / nextjs to ignore sentry binary... Would solve vercel problem, but then quid of serverless, netlify...

I could provide some P/R's but let me know what would the recommended approach...

kamilogorek commented 3 years ago

webpack plugin has been moved to dev deps. Let me know if this issue will still persist after next release.

lobsterkatie commented 3 years ago

webpack plugin has been moved to dev deps. Let me know if this issue will still persist after next release.

Hmmm. Not having seen this, I moved it back, because SDK users need it as part of building their app, and if it's in our dev dependencies, it'll only install for us, not for folks who yarn install our SDK. ~That said, @belgattitude, can you help me understand how you're ending up with it in your bundle? It's only used as part of the build process, so there's no reason that I can think of why webpack would be including it in your built app.~

~How are you getting the logs you included in the PR description?~

UPDATE: Stumbled across an undocumented NEXT_DEBUG_FUNCTION_SIZE env var which outputs the size breakdown you posted, and I now realize that a) you were talking about your serverless lambdas, not your browser bundle, and b) I'm also ending up with @sentry/cli included (which makes it clear that on this score, the bundle analyzer is only really helpful for the front end). Boo.

I notice that we're also both ending up with caniuse data, which I'd assume similarly has no reason to be there. I tried this trick, but it didn't seem to make a difference. Am I missing something, though? Why aren't these things getting treeshaken out? It does seem like the lambda-creation process is separate from the server bundle creation process, but still...

In any case, as it happens, I'm actually trying to get more insight into how the lamdas are created for other reasons (was working on it just today, in fact), so perhaps getting a handle on that will shed some light here as well.

lobsterkatie commented 3 years ago

Reopening this because it isn't solved now that I undid the change.

belgattitude commented 3 years ago

@lobsterkatie sorry for long delays,

I haven't looked thoroughly, but yes moving to devDeps is quite a strong statement somehow :)

Something that might help you:

Working with NEXT_DEBUG_FUNCTION_SIZE is not very easy when going testing. Personally I created a little wrapper over https://github.com/vercel/nft to trace what is included in the server build. Very convenient.

I haven't had the time to look into the code, but I'll try next week.

stnwk commented 3 years ago

Spend the past couple days to debug this exact issue :/

I'm working on a NextJS app, deployed on Vercel. We're using the @sentry/nextjs integration and have one serverless api function that makes screenshots using puppeteer-core and chrome-aws-lambda. Both of those packages together are barely under 50MB so that you can use it in a serverless function. So far, so good.

But because of this issue sentry is adding a couple megabytes to the compressed lambda function, which brings it over the max limit of 50MB and therefore fails the build. Even worse, the sentry plugin seems to include a lot more unrelated dependencies in the lambda function.

This issue a is total roadblock for us: Is there any workaround or do I need to stop using sentry?

belgattitude commented 3 years ago

@stnwk @lobsterkatie

Is there any workaround or do I need to stop using sentry?

If you're hitting serverless limits the @sentry/nextjs is not the way to go IMHO. (I would add if you're going close to 40Mb, lambdas would already start giving 504 from time to time, cold starts are too long depending where you host them).

As workaround it's possible to use sentry for the browser part only, you'll have to go back in time in the examples, ie https://github.com/vercel/next.js/tree/6aef4b8445472c53c4a4c39005319c2fc73e44de/examples/with-sentry. that's more or less the direction I took for sandboxes (prod is on docker/k8s so it's less a problem). For monitoring api routes we actually send to datadog.

But I feel there must be some more thinking related to @sentry/nextjs.

Personally and if it's only used to upload sourcemaps I would ditch @sentry/cli totally in favour of lightweigtht js code (or even wasm). I fear that It would be almost impossible to shrink the size of the binaries in the current state (not even talking that on alpine the wrong binary is picked when installing and the fact that sentry.properties looks weird for js devs, etc).

Nextjs, lambdas, serverless... paradigms requires deps to be light as possible. @sentry/nextjs should consider this as a requirement to ease adoption.

Cause both sentry and nextjs are wonderful products :smile:

PS: @stnwk your use case with pupeeter might look very specific... But actually in many setup we'll add sharp, prisma, graphql and why not redis ?.. add @sentry/nextjs and the limit is easily reached.

stnwk commented 3 years ago

PS: @stnwk your use case with pupeeter might look very specific... But actually in many setup we'll add prisma, graphql and why not redis ?.. add @sentry/nextjs and the limit is easily reached.

Yeah, we have all of those too 😅 It's definitely an issue that many people will run into. Thanks for your quick response!

I'm currently trying to find a way without @sentry/nextjs, but instead using the @sentry/webpack-plugin directly to upload source maps and the classic webpack-alias solution for @sentry/node and @sentry/browser.

So far, so good.

Only nextjs api routes remain a problem, let's see. I remain optimistic there is a much more lightweight solution to this package :)

belgattitude commented 3 years ago

@lobsterkatie

Why aren't these things getting treeshaken out? It does seem like the lambda-creation process is separate from the server bundle creation process, but still...

There's two builds, one for server code, one for client code.

I guess standard webpack and tree-shaking is not used to optimize lambdas "as is".

(anyway if a package require @sentry/cli and in return @sentry/cli loads a binary it would be hard for the tree-shaking to happen. There might be a clue here.)

I'm not sure how nextjs does, but I assume they use @vercel/nft package to do a tracing of files (not packages, but files present in node_modules) that are required from the route (lambda, ie: /api/test).

From there they will probably try to remove all (node_modules) files that aren't actually needed.(node_modules is a black hole, so without this lambdas wouldn't possible), then keep the required ones and do a specific bundle with them.

I assume the reason why the sentry binary is included is that there's a require of it (cause process.env.NODE_ENV is 'production' when you're in the build phase)

There might be something to do here.

I don't know how vercel does, but you can have a look of how serverless does here: https://github.com/serverless-nextjs/serverless-next.js/blob/master/packages/libs/lambda-at-edge/src/build.ts.... It's possible that other providers use this method (netlify...). I haven't investigated cause I'm using docker/k8s for prod.

So technically it would be possible to ditch the binary by adding exclusion in @vercel/nft and then coordinating with nextjs, serveless, aws-amplify....

But IMHO, if it's just to upload the sourcemaps I would create a lightweight js script, for those reasons:

the sentry/cli with sentry/properties is not very engaging for nextjs devs. the .env(.local...) is well established (I know it's very minimal). I guess it was done to standardize stacks (java, rust...)
downloading a binary on postinstall is not very "ecologically green" (if it's a regular plain js it can't be part of the yarn cache for example). Can be a source of failure too
binaries -> platform support -> and in some platform we'll get problems (alpine arm)...

It's just some comments I have in my mind. but no real solution, hope you'll find one :100:

github-actions[bot] commented 3 years ago

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

stnwk commented 3 years ago

Leave it.

belgattitude commented 3 years ago

Ah I'm following https://github.com/vercel/next.js/pull/30401, it seems there's some move, sentry at its best :smile:

Thx a lot @lobsterkatie , if anytime I can help to test ping me.

JavierMartinz commented 2 years ago

We are having this same issue (Sentry + Puppeteer + ChromeAWSLambda) with 2 projects and so far the only solution is to use the classic webpack-alias solution 😢 Let us know if there is other possibility to take advantage of this official solution

psteinroe commented 2 years ago

Same issue here! Any updated? And @stnwk, could you clarify on

I'm currently trying to find a way without @sentry/nextjs, but instead using the @sentry/webpack-plugin directly to upload source maps and the classic webpack-alias solution for @sentry/node and @sentry/browser.

and maybe provide an example?

stnwk commented 2 years ago

Sorry, I no longer work in this context and don't remember the exact details from August last year.

What I meant with this:

I'm currently trying to find a way without @sentry/nextjs, but instead using the @sentry/webpack-plugin directly to upload source maps and the classic webpack-alias solution for @sentry/node and @sentry/browser.

is, that @sentry/nextjs is just a fancy wrapper around the webpack-plugin and server/client side imports for @sentry/node and @sentry/browser.

Back then I was trying to implement a work-around using webpack aliases and the webpack-plugin directly instead of using @sentry/nextjs. But that's about all I remember here.

smeubank commented 2 years ago

Hi folks,

I know this is an outstanding issue, and apologies we have not been able to give it more attention. We are very focused on the work https://github.com/getsentry/sentry-javascript/issues/4882 detailed here and the roadmap linked therein. There will be significant bundle size reduction with the V7 release. Rest assured we are very bundle size sensitive these days 😬

We will revisit this as capacity sees fit!

smeubank commented 2 years ago

we are currently investigating 3 possibilities

If there is some reason that it is being included due Vercel's setup where the binary is pulled in, in which case we can just try to delete it
if it is somehow phantom and the size check is misleading us, because there is no good reason why the binary would be included
having 2 npm package (2 entry points) @sentry/nextjs/webpack & @sentry/nextjs/webpack

The 3rd option being what we agreed on with Vercel, as a fix that can be done prior to/instead of making some tree shaking changes. This does add some complexities, and similar to electron it might have some problem for some users with certain setup. But we are dedicated to finding a solution that will work for most everyone.

stavalfi commented 2 years ago

build is failing....

yarn run v1.18.0
$ next build
warn  - You have enabled experimental feature(s).
warn  - Experimental features are not covered by semver, and may cause unexpected or broken application behavior. Use them at your own risk.

info  - Checking validity of types
info  - Using tsconfig file: ../../tsconfig.json
info  - Creating an optimized production build
Failed to compile.

Sentry CLI Plugin: Command failed: /Users/stavalfi/projects/cvi-swissknife/node_modules/@sentry/cli/sentry-cli releases new vDZZDHQim6sB1uL5oifaP
error: A project slug is required

Add --log-level=[info|debug] or export SENTRY_LOG_LEVEL=[info|debug] to see more output.
Please attach the full debug log to all bug reports.

> Build failed because of webpack errors
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

smeubank commented 2 years ago

Hi all!

We've recently shipped our first beta for the V7 release! We would love to get your feedback on the changes we've made to the SDK and migration guide.

Also important to note the fix for @sentry/cli always bundled in production is one of the improvements. Really hoping to hear back about some bundle size wins :D

Thank you!

stavalfi commented 2 years ago

@smeubank

Hi all!

We've recently shipped our first beta for the V7 release! We would love to get your feedback on the changes we've made to the SDK and migration guide.

Also important to note the fix for @sentry/cli always bundled in production is one of the improvements. Really hoping to hear back about some bundle size wins :D

Thank you!

Very disappointing... Still same error with "@sentry/nextjs": "7.0.0-beta.0"

Sentry CLI Plugin: Command failed: /Users/stavalfi/projects/cvi-swissknife/node_modules/@sentry/cli/sentry-cli releases new development
error: A project slug is required

Add --log-level=[info|debug] or export SENTRY_LOG_LEVEL=[info|debug] to see more output.
Please attach the full debug log to all bug reports.

My nextjs project does not even run locally on development mode.

smeubank commented 2 years ago

@stavalfi hmm yes disappointing :( this is not the same as issue as how this issue started. You are blocked entirely vs just having all the bloat from CLI in prod bundle

kamilogorek commented 2 years ago

@stavalfi you need to provide project name for your cli config, as the error states - https://docs.sentry.io/platforms/javascript/guides/nextjs/manual-setup/#use-configuration-files Not sure why you don't have this automatically set, as I don't know the context, but this will fix it.

belgattitude commented 2 years ago

@smeubank @kamilogorek

Here's some quick tests with @sentry/nextjs@7.0.0-beta.1

On the server side

sentry-cli is well removed :100:, congrats ! Tested on vercel (lambdas) with latest nextjs 12.1.6

Unrelated to this issue, but worth to share:

Throttling

When uploading sourcemaps the requests are often throttled

On the browser side

7.0.0-beta.1 made my bundle size happier (with tracing disabled). Amazing

Modest P/R example here: https://github.com/belgattitude/nextjs-monorepo-example/pull/1854

Keep up the fantastic work !

AbhiPrasad commented 2 years ago

@belgattitude - https://docs.sentry.io/platforms/javascript/guides/nextjs/configuration/tree-shaking/ should give you even more savings! Thanks for the feedback, this is something we'll keep improving on!

belgattitude commented 2 years ago

@AbhiPrasad should give you even more savings!

I guess @sentry/nextjs:7.0.0-beta.1 does already the job.

Attempt here https://github.com/belgattitude/nextjs-monorepo-example/pull/1863, but haven't seen any significant difference (58kb vs 59kb for sentry ). Tested with swcMinify: true. Not tested with terser.

Always open to ideas, I'll keep giving some feedbacks along the beta's, let me know if you want to try out things.

Keep up shining.

smeubank commented 2 years ago

sentry-cli is well removed 💯, congrats ! Tested on vercel (lambdas) with latest nextjs 12.1.6

closing this issue now that the fix is shipped and folks are reporting success 😄

for issues around how to better take advantage of the bundle size reductions in V7, including tree shaking, we might be better off moving those talks here https://github.com/getsentry/sentry-javascript/issues/4240 to include more folks

Always open to ideas, I'll keep giving some feedbacks along the beta's, let me know if you want to try out things.

Keep up shining.

many thanks for your support and feedback @belgattitude! :) it's been a long road to V7, happy folks appreciate the work

getsentry / sentry-javascript