-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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): Inject init code in _app
and API routes
#3786
Merged
lobsterkatie
merged 3 commits into
master
from
kmclb-nextjs-inject-init-code-in-api-routes
Jul 9, 2021
Merged
ref(nextjs): Inject init code in _app
and API routes
#3786
lobsterkatie
merged 3 commits into
master
from
kmclb-nextjs-inject-init-code-in-api-routes
Jul 9, 2021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lobsterkatie
force-pushed
the
kmclb-nextjs-inject-init-code-in-api-routes
branch
from
July 8, 2021 07:17
cb73a2a
to
5fbe134
Compare
size-limit report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything else LGTM, and note that the missing PR description may answer my early questions.
AbhiPrasad
reviewed
Jul 8, 2021
lobsterkatie
force-pushed
the
kmclb-nextjs-pre-inject-init-in-api-routes-clean-up
branch
from
July 8, 2021 18:00
d8f4a82
to
7362042
Compare
Base automatically changed from
kmclb-nextjs-pre-inject-init-in-api-routes-clean-up
to
master
July 8, 2021 18:21
AbhiPrasad
reviewed
Jul 8, 2021
lobsterkatie
force-pushed
the
kmclb-nextjs-inject-init-code-in-api-routes
branch
2 times, most recently
from
July 9, 2021 06:27
f24f33d
to
23c7797
Compare
AbhiPrasad
approved these changes
Jul 9, 2021
lobsterkatie
changed the title
ref(nextjs): Inject init code in
ref(nextjs): Inject init code in Jul 9, 2021
_app
and API routes_app
and API routes WEB-26
lobsterkatie
changed the title
ref(nextjs): Inject init code in
ref(nextjs): Inject init code in Jul 9, 2021
_app
and API routes WEB-26_app
and API routes
lobsterkatie
changed the title
ref(nextjs): Inject init code in
ref(nextjs): Inject init code in Jul 9, 2021
_app
and API routes_app
and API routes WEB-26
lobsterkatie
changed the title
ref(nextjs): Inject init code in
ref(nextjs): Inject init code in Jul 9, 2021
_app
and API routes WEB-26_app
and API routes
High five tracking down the issues with AWS Lambda, they're not easy eggs to catch. 👍 |
This was referenced Jul 12, 2021
9 tasks
Closed
6 tasks
lobsterkatie
added a commit
that referenced
this pull request
Jul 20, 2021
As discussed in more detail in the penultimate paragraph of #3786, when deployed to vercel, API route lambdas have been shutting down too soon for sentry events to get reliably sent to our servers (in spite of the use of `flush`). This fixes that by wrapping the response's `.end()` method (which is what triggers the lambda shutdown) such that it waits for `flush` to finish before emitting its `finished` signal. Logs from API routes now consistently look like this: ``` [GET] /api/hello Sentry Logger [Log]: [Tracing] Starting http.server transaction - GET /api/hello Sentry Logger [Log]: [Tracing] Finishing http.server transaction - GET /api/hello Sentry Logger [Log]: Flushing events... Sentry Logger [Log]: Done flushing events ```
This was referenced Jul 22, 2021
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TL;DR:
This PR changes how we include the user's
sentry.server.config.js
andsentry.client.config.js
code (which is what callsSentry.init()
) in server and browser bundles, in order both to fix apps deployed to Vercel and to eliminate some semi-gross hacks included in the current implementation.Background
Because of the way nextjs apps behave when deployed to Vercel, our current system of initializing the SDK during server start up doesn't work for API routes. (Vercel splits such routes into AWS lambdas, and in order to keep the size of each lambda small, it uses a stripped-down version of the server when it does so. As a result, half of our current monkeypatching doesn't even run, including the part which starts the SDK.)
Further, the current system is ugly in two ways. Neither one of them is a blocker, but they're both less than ideal.
On the server side, we currently create a separate bundle out of the user's
sentry.server.config.js
file. In order to be sure we can find it later, at build time we log this bundle's location to.env.local
(which contains environment variables that nextjs automatically makes available at runtime). This isn't great, because it forces us to worry about whether they already have such a file (so we can merge rather than overwrite it), and if they don't, creates a surprise file in the user's project directory with no explanation.On the client side, we currently inject the user's
sentry.client.config.js
file into themain
bundle. This is fine, except in cases where nextjs's backwards-compatibility-preserving code ends up overwriting the change. (See here for more details and the hack we use to work around the problem.) The hack we have works, but isn't pretty.This PR fixes all three of those problems.
Changes
It turns out the easiest way to fix things is to avoid the server question (stripped down or not) altogether. This PR therefore:
Stops creating a separate server-init bundle, writing its location to disk, and monkeypatching the nextjs server to dynamically require it when it starts up. Instead, it injects the initializing code directly into the API route files. By making sure to add it specifically to the beginning of each API entry point, we can ensure that the SDK will be up and running by the time any code in the route file runs.
Stops injecting the client-init code into
main
and instead injects it intopages/_app
, which is the container wrapping all user-created pages, and whose code therefore runs before theirs does, similarly guaranteeing the SDK will have started up by the time theirs runs.Does the same thing (injecting init code into
pages/app
) on the server side. Though the SDK isn't yet totally functional in non-API routes, this at least means that unhandled errors and promise rejections can be captured.Notes
Because we're no longer injecting into nextjs framework code (the server itself and, on the client side, the
main
bundle), we no longer need to protect againstpublicRuntimeConfig
being undefined, since it's loaded when the framework starts up. By the time_app
is rendered, or API handlers are run, that start-up process has long since completed.In my testing on Vercel, I sometimes ran into the situation where transactions for API routes appeared not to be getting successfully sent to Sentry. After a great deal of experimentation, I finally discovered that in these cases, the sending wasn't blocked but merely delayed - the transaction would in fact send itself, but not until the next time the same route was hit (provided it was soon enough that the route's lambda function hadn't been garbage collected for being idle too long; in those cases, the transaction was simply lost). Thanks to @kamilogorek, I tracked it down to the AWS
callbackWaitsForEmptyEventLoop
option, which nextjs sets tofalse
by default, thereby causing the lambda to shut down as soon as the API response is finished.Though it's possible to override that setting, it will require reformatting[UPDATE: UpdatingwithSentry
to itself be a lambda, and in order to not complicate things further, that work will happen in a future PR.withSentry
to return a lambda directly turned out to be quite complicated, so in the end we're instead monkeypatchingres.end()
to wait to fire until all events have been sent to Sentry.)This PR also doesn't fix the problem of subsequent requests to other API routes somehow thinking they're a continuation of the same trace. That problem remains a mystery (though I suspect it has something to do with scope bleed), and will also need to be solved in a future PR. [UPDATE: Though it doesn't solve all of our scope problems, this PR fixes this false positive continuation issue.)
Fixes #3723
Fixes #3683
Fixes #3777
Fixes #3648
Fixes #3688
Fixes #3691
Fixes #3748