fix(nextjs): Delay error propagation until withSentry is done#4027
Merged
lobsterkatie merged 10 commits intomasterfrom Nov 15, 2021
Merged
fix(nextjs): Delay error propagation until withSentry is done#4027lobsterkatie merged 10 commits intomasterfrom
withSentry is done#4027lobsterkatie merged 10 commits intomasterfrom
Conversation
Contributor
size-limit report
|
54db08b to
5d1ad31
Compare
5 tasks
2 tasks
|
Thank you for this PR 🙏 Is this likely to get merged soon? This is a big issue right now, with no monitoring on our production apps routes deployed to Vercel. If not, it would be great to update the docs of @sentry/nextjs saying that it does not work on Vercel. |
9 tasks
5d1ad31 to
1bb2edb
Compare
iker-barriocanal
suggested changes
Nov 10, 2021
Contributor
iker-barriocanal
left a comment
There was a problem hiding this comment.
Blocking because of an infinte loop in a test.
1bb2edb to
5c89c28
Compare
| const captureExceptionSpy = jest.spyOn(Sentry, 'captureException').mockImplementation(jest.fn()); | ||
| const loggerSpy = jest.spyOn(utils.logger, 'log'); | ||
| const flushSpy = jest.spyOn(Sentry, 'flush').mockImplementation(async () => { | ||
| // simulate the time it takes time to flush all events |
Contributor
There was a problem hiding this comment.
Suggested change
| // simulate the time it takes time to flush all events | |
| // simulate the time it takes to flush all events |
iker-barriocanal
approved these changes
Nov 15, 2021
Member
Author
|
Discussed IRL - agreed to merge this. |
lobsterkatie
added a commit
that referenced
this pull request
Nov 15, 2021
In our nextjs API route wrapper `withSentry`, we capture any errors thrown by the original handler, and then once we've captured them, we rethrow them, so that our capturing of them doesn't interfere with whatever other error handling might go on. Until recently, that was fine, as nextjs didn't actually propagate the error any farther, and so it didn't interfere with our processing pipeline and didn't prevent `res.end()` (on which we rely for finishing the transaction and flushing events to Sentry) from running. However, Vercel released a change[1] which caused said errors to begin propagating if the API route is running on Vercel. (Technically, it's if the server is running in minimal mode, but all API handlers on vercel do.) A side effect of this change is that when there's an error, `res.end()` is no longer called. As a result, the SDK's work is cut short, and neither errors in API route handlers nor transactions tracing such routes make it to Sentry. This fixes that, by moving the work of finishing the transaction and flushing events into its own function and calling it not only in `res.end()` but also before we rethrow the error. (Note: In the cases where there is an error and the server is not running in minimal mode, this means that function will be hit twice, but that's okay, since the second time around it will just no-op, since `transaction.finish()` bails immediately if the transaction is already finished, and `flush()` returns immediately if there's nothing to flush.) H/t to @jmurty for his work in #4044, which helped me fix some problems in my first approach to solving this problem. Fixes #3917. [1] vercel/next.js#26875
4 tasks
onurtemizkan
pushed a commit
that referenced
this pull request
Dec 19, 2021
In our nextjs API route wrapper `withSentry`, we capture any errors thrown by the original handler, and then once we've captured them, we rethrow them, so that our capturing of them doesn't interfere with whatever other error handling might go on. Until recently, that was fine, as nextjs didn't actually propagate the error any farther, and so it didn't interfere with our processing pipeline and didn't prevent `res.end()` (on which we rely for finishing the transaction and flushing events to Sentry) from running. However, Vercel released a change[1] which caused said errors to begin propagating if the API route is running on Vercel. (Technically, it's if the server is running in minimal mode, but all API handlers on vercel do.) A side effect of this change is that when there's an error, `res.end()` is no longer called. As a result, the SDK's work is cut short, and neither errors in API route handlers nor transactions tracing such routes make it to Sentry. This fixes that, by moving the work of finishing the transaction and flushing events into its own function and calling it not only in `res.end()` but also before we rethrow the error. (Note: In the cases where there is an error and the server is not running in minimal mode, this means that function will be hit twice, but that's okay, since the second time around it will just no-op, since `transaction.finish()` bails immediately if the transaction is already finished, and `flush()` returns immediately if there's nothing to flush.) H/t to @jmurty for his work in #4044, which helped me fix some problems in my first approach to solving this problem. Fixes #3917. [1] vercel/next.js#26875
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
In our nextjs API route wrapper
withSentry, we capture any errors thrown by the original handler, and then once we've captured them, we rethrow them, so that our capturing of them doesn't interfere with whatever other error handling might go on. Until recently, that was fine, as nextjs didn't actually propagate the error any farther, and so it didn't interfere with our processing pipeline and didn't preventres.end()(on which we rely for finishing the transaction and flushing events to Sentry) from running.However, Vercel released a change which caused said errors to begin propagating if the API route is running on Vercel. (Technically, it's if the server is running in minimal mode, but all API handlers on vercel do.) A side effect of this change is that when there's an error,
res.end()is no longer called. As a result, the SDK's work is cut short, and neither errors in API route handlers nor transactions tracing such routes make it to Sentry.This fixes that, by moving the work of finishing the transaction and flushing events into its own function and calling it not only in
res.end()but also before we rethrow the error.(Note: In the cases where there is an error and the server is not running in minimal mode, this means that function will be hit twice, but that's okay, since the second time around it will just no-op, since
transaction.finish()bails immediately if the transaction is already finished, andflush()returns immediately if there's nothing to flush.)H/t to @jmurty for his work in #4044, which helped me fix some problems in my first approach to solving this problem.
Fixes #3917.