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

fix: handle Promise.reject case of Sentry.flush in with-sentry example #26767

Closed

Conversation

natterstefan
Copy link

@natterstefan natterstefan commented Jun 30, 2021

Note

It took me hours to figure out why I was getting an "Internal Server Error" when I added Sentry to our Next-Vercel setup based on "with-sentry".

I prepared examples in my demo repo here: natterstefan/next-with-sentry#2 with one deployment with and one without the try/catch.

The with-sentry example does not handle the case when Sentry.flush returns a rejected Promise. I was not aware of that as well until I read getsentry/sentry-javascript#3691 (comment).

In my opinion, one problem lies in the description of Sentry.flush:

If you provide a timeout and the queue takes longer to drain the promise returns false
(src)

I did not think of handling Promise.reject when I read the docs of Sentry.flush. 😅 That took some time to figure that out.

The maintainers of @sentry/nextjs use try/catch as well -> getsentry/sentry-javascript@6c1b4bd.


FTR: So this fixes the example, but it does not fix the issue why Sentry.flush throws an error/rejects. 🤔

Edit: There are comments mentioning the issue with Sentry.flush: getsentry/sentry-javascript#3643 (comment), and getsentry/sentry-javascript#3746. Let's see if they handle it (announced here) or not.

Documentation / Examples

  • Make sure the linting passes

@lobsterkatie
Copy link
Contributor

lobsterkatie commented Jul 29, 2021

So this fixes the example, but it does not fix the issue why Sentry.flush throws an error/rejects

Hi, @natterstefan. I pinged you also in one of the issues you linked, but this should be fixed in getsentry/sentry-javascript#3846. Also, getsentry/sentry-javascript#3811 fixed the await issue with flush.

In fact, everything shown in your example, including capturing the exception, is handled by withSentry and shouldn’t have to be done manually anymore (see the bottom code snippet here).

Cheers!

@natterstefan
Copy link
Author

natterstefan commented Sep 26, 2021

Hi @lobsterkatie,

I am sorry it took me some time to answer. Thanks for getting back to me. I tested it with "@sentry/nextjs": "6.13.2" and I can confirm that it works (both with the latest next@10 but also next@11 (as of today) on Vercel.

Thanks a lot for taking care of that!

@natterstefan
Copy link
Author

Closed, see #26767 (comment) for more details.

@natterstefan natterstefan deleted the fix/with-sentry-flush-catch branch September 26, 2021 13:31
@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
examples Issue/PR related to examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants