fix(nextjs): Delay error propagation until `withSentry` is done by lobsterkatie · Pull Request #4027 · getsentry/sentry-javascript

lobsterkatie · 2021-10-05T03:55:16Z

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

github-actions · 2021-10-05T04:10:25Z

size-limit report

Path	Size
@sentry/browser - CDN Bundle (gzipped)	22.47 KB (0%)
@sentry/browser - Webpack	23.35 KB (0%)
@sentry/react - Webpack	23.39 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped)	29.92 KB (0%)

iker-barriocanal

The approach looks good to me.

joshsny · 2021-11-04T12:27:21Z

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.

iker-barriocanal

Blocking because of an infinte loop in a test.

iker-barriocanal · 2021-11-12T09:24:23Z

+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


Suggested change
// simulate the time it takes time to flush all events
// simulate the time it takes to flush all events

lobsterkatie · 2021-11-15T16:10:41Z

Discussed IRL - agreed to merge this.

@jmurty

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

@jmurty

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

iker-barriocanal reviewed Oct 5, 2021
View reviewed changes

Comment threadpackages/nextjs/src/utils/withSentry.ts Outdated
Comment threadpackages/nextjs/src/utils/withSentry.ts Outdated

lobsterkatie force-pushed the kmclb-nextjs-fix-api-error-propagation branch 2 times, most recently from 54db08b to 5d1ad31Compare October 13, 2021 21:31

lobsterkatie added the Status: In Progress label Nov 2, 2021

lobsterkatie mentioned this pull request Nov 2, 2021
@sentry/nextjs not reporting errors from API (serverless) endpoints in Vercel #3917
Closed
5 tasks

jmurty mentioned this pull request Nov 2, 2021
Ensure withSentry reports API (serverless) errors from Vercel #4044
Closed
2 tasks

dcastil mentioned this pull request Nov 8, 2021
Crash on server side running on Vercel after upgrading to Next.js 12 #4103
Closed
9 tasks

lobsterkatie force-pushed the kmclb-nextjs-fix-api-error-propagation branch from 5d1ad31 to 1bb2edbCompare November 9, 2021 14:33

lobsterkatie marked this pull request as ready for review November 9, 2021 14:46

iker-barriocanal suggested changes Nov 10, 2021
View reviewed changes

lobsterkatie added 9 commits November 10, 2021 16:13

rename variables
92341fe

pull cleanup work out into its own function
517f73e

stop awaiting result of domain-bound handler
4702da7

also flush before rethrowing error
68aa490

manually set error status on response
ec6822d

fix wrapped handler type
87efd29

add flushing tests
da218b3

rework comments in helper function to make it clear the loop ends
ed6d624

give the thrown error a more distinctive name
5c89c28

lobsterkatie force-pushed the kmclb-nextjs-fix-api-error-propagation branch from 1bb2edb to 5c89c28Compare November 11, 2021 00:37

iker-barriocanal reviewed Nov 12, 2021
View reviewed changes

iker-barriocanal approved these changes Nov 15, 2021
View reviewed changes

Merge branch 'master' into kmclb-nextjs-fix-api-error-propagation
1af445e

lobsterkatie merged commit fb66a78 into masterNov 15, 2021

lobsterkatie deleted the kmclb-nextjs-fix-api-error-propagation branch November 15, 2021 16:46

neverether mentioned this pull request Nov 16, 2021
Error to render page on NextJs 12 with fallback = true #4090
Closed
4 tasks

DavidChouinard mentioned this pull request Nov 17, 2021
Update @sentry/nextjs package.json to support Next.js 12 #4163
Closed

lobsterkatie mentioned this pull request Sep 26, 2022
ref(nextjs): Use flush code from withSentry in all backend wrappers #5814
Merged

	// simulate the time it takes time to flush all events
	// simulate the time it takes to flush all events

Uh oh!

Uh oh!

Conversation

lobsterkatie commented Oct 5, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actionsBot commented Oct 5, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report

Uh oh!

iker-barriocanal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joshsny commented Nov 4, 2021

Uh oh!

iker-barriocanal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iker-barriocanalNov 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lobsterkatie commented Nov 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lobsterkatie commented Oct 5, 2021•
edited
Loading

github-actionsBot commented Oct 5, 2021•
edited
Loading

iker-barriocanal left a comment •
edited
Loading