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

ref(nextjs): Stop reinitializing the server SDK unnecessarily #3860

Merged
merged 1 commit into from
Aug 3, 2021

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Aug 3, 2021

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 PR 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.

The above change itself should be straightforward to review, but the test changes might be slightly more complicated to look at. 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.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 21.56 KB (+0.01% 🔺)
@sentry/browser - Webpack 22.56 KB (0%)
@sentry/react - Webpack 22.59 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 28.99 KB (0%)

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.

Nice!

Can we please split this up into 2 PRs (will slam ✅ on both). This makes it easier for us to write a changelog and for us to bisect commits when identifying problems in the future.

packages/core/src/integration.ts Outdated Show resolved Hide resolved
@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-stop-redoing-setup-steps branch from f29829d to 4e0d6c9 Compare August 3, 2021 15:54
@lobsterkatie lobsterkatie changed the title fix(core/nextjs): Stop redoing setup steps unnecessarily ref(nextjs): Stop redoing setup steps unnecessarily Aug 3, 2021
@lobsterkatie
Copy link
Member Author

Can we please split this up into 2 PRs (will slam ✅ on both). This makes it easier for us to write a changelog and for us to bisect commits when identifying problems in the future.

Sure. The core changes are now in #3862, and this is now only the nextjs change.

@lobsterkatie lobsterkatie changed the title ref(nextjs): Stop redoing setup steps unnecessarily ref(nextjs): Stop reinitializing the server SDK unnecessarily Aug 3, 2021
@lobsterkatie lobsterkatie merged commit 9ca4c5b into master Aug 3, 2021
@lobsterkatie lobsterkatie deleted the kmclb-nextjs-stop-redoing-setup-steps branch August 3, 2021 17:10
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.

None yet

2 participants