sentry-javascript: Node suggestion: log errors in `unhandledRejection`

I still find it’s weird that uncaughtException and unhandledRejection are handled differently. Sentry restores logging for uncaughtException—why doesn’t it do the same for unhandledRejection? Users shouldn’t have to remember to use that Node flag. 🤔

As I see it there a few options:

1. Replicate Node.js’s way of doing it
2. Just write to console.error when there’s an unhandled rejection
3. Suppress the error so the developer will never see it

I think option 1 or 2 both seem fine. Your customer sees the error and can fix it. What you absolutely shouldn’t do is option 3, where your customer doesn’t see errors and Sentry causes errors to go in to production (oh, but the irony for a error reporting tool). This is the current behavior and this should really stop! Sentry should help me catch errors, not make it worse.

Even if you choose option 2, at least developers will see the rejection and will notice that they wanted a different behavior (like a crash), and can implement that. But without knowing there even was a rejection, they can’t do much about it.

Just to clarify: when enabling Sentry, the behaviour of unhandledException (exit + log) is preserved, but the behaviour of unhandledRejection (log) is not:

handler	logs	exits
unhandledException default	yes	yes
unhandledException Sentry	yes	yes
unhandledRejection default	yes	no
unhandledRejection Sentry	no	no

Right now everything works as expected according to official node documentation.

“As expected” here is assuming the user understands that Sentry is registering a listener for unhandledRejection. That’s an implementation detail that users shouldn’t have to worry about.

I do see your point though. Sentry should also respect --unhandled-rejections, which it wouldn’t be doing if the flag was set to none and Sentry was continuing to log.

So, a bit of debugging reveals the reason console logging is suppressed:

Sentry assigns a function to window.onunhandledrejection, and as we see here, that function returns false, thereby explicitly suppressing console logging. So yeah, Sentry does change the default behavior - that’s not cool.

Luckily, it stores a reference to any existing function, and calls that if it exists. So, the hacky workaround to re-enable console logging, is to add this line, before initializing Sentry:

window.onunhandledrejection = () => true;

Now, please fix this, so we can have the default behavior without such pointless hacks 🙂

@OliverJAsh, @kamilogorek Can we please have this reopened and fixed?

Sentry absolutely shouldn’t mess with how errors are logged in the console - and this is not just a problem with Node, as console logging is suppressed in the browser too.

It’s super annoying when trying to debug things, and since you didn’t bother mentioning this in the docs for JavaScript, I actually thought I had no errors when I looked in the console while testing a staging build, and therefore didn’t realize it had errors, until after it was deployed to production. That’s a really shitty user experience for an error reporting service, and one you need to fix if you want happy customers.

And as said before, promise rejections are not second-class errors - they can be just as fatal as any other unhandled error, and shouldn’t be suppressed in any way.

But that’s how Node.js is designed ¯_(ツ)_/¯ If you add handler, warning is gone. Our SDK adds handler, because it’s the only way to catch unhandled errors, which is the main purpose of the SDK.

This should do the work. https://github.com/getsentry/sentry-javascript/pull/2312 I didn’t add a way to add your own callback, as writing code below would have the exact same effect:

Sentry.init({
  integrations: [
    new Sentry.Integrations.OnUnhandledRejection({
      mode: 'none'
    })
  ]
});

process.on('unhandledRejection', (reason) => {
  // your callback
})

or uncaught exceptions it’s node itself what’s triggering a log call.

On second thoughts, I’m not sure that’s true.

Sentry registers an uncaughtException handler which disables the default Node behaviour (log + exit).

The handler (defaultOnFatalError) triggers its own log call: https://github.com/getsentry/sentry-javascript/blob/4.6.3/packages/node/src/handlers.ts#L282.

If we’re “restoring” logging for uncaught exceptions, I believe we should do the same thing for unhandled promise rejections.

@schumannd FYI, this morning we released @sentry/react-native 1.10.0 [EDIT: whoops, should be 1.1.0], which updates its dependency to use the latest version of @sentry/browser (which includes the return-true-instead-of-false fix mentioned above).

I do see your point - having to replicate the node behavior would be a potential maintenance issue, and it does help that this can, for node, be solved with a simple command line flag.

But if you don’t want to replicate the node behavior, then at least log a warning to the console when that flag is not specified, so users are aware that errors are being suppressed, and how to avoid that.

This becomes even more important in the next version of node, where unhandled rejections will, by default, crash the process - which, as I understand it, won’t happen when Sentry adds its handler. Users who rely on that new default behavior in node, could potentially be in for a nasty surprise, if they later install Sentry and their process then suddenly continues, despite a fatal error having occurred. That’s the kind of thing that could lead to data loss or other disasters.

@freeall comment sums this up pretty well:

I think users of Sentry would expect Sentry to not alter the behavior of their program.

Uncaught promise rejections are no “second class errors”. They can and do cause apps to break just like normal errors do.

It seems like several users have run into this problem (including me), and more will run into it in the future.

So in summary: Sentry silences errors from the console. And I think it is obvious how this is confusing, and probably not intended by most sentry users. They don’t install sentry to have some subset of errors silenced from console.

So the reasoning (by @kamilogorek )

that’s how Node.js is designed

Is a bit cryptic to me. How does this dictate the way sentry should behave?

Should we expect users to know about:

a) sentrys inner workings (registers event handlers)

b) node inner workings (adding handlers causes warnings to disappear)

And if they don’t, well they should?

Of course you are right in saying “well, the user should know, what the code does under the hood”, but the reality looks different. And you have the opportunity here to increase ease of use of sentry, or not.

It’s ok to say “we don’t care about this particular issue”, but painting it in a “it’s not a bug, it’s a feature” kind of way seems insincere.

TLDR: IMOH If you want to increase ease of use, and reduce problems for newbie users, then this should be fixed.

@OliverJAsh , @freeall What solutions did you use for this problem?

I’m not sure either.

Enabling Sentry doesn’t have the effect of disabling logging for uncaught exceptions, so maybe the same should be true for unhandled promise rejections. (Although this difference in behaviour is more of an inconsistency in Node than Sentry.)

When I enable Sentry I understand it’s reporting my errors, but I tend to think of console logging as a separate thing, especially in dev. 🤔

Docs PR is already in progress https://github.com/getsentry/sentry-docs/pull/1351/

@OliverJAsh this change has nothing to do with the cli flag. It’s behavior is untouched. The thing that changed is that OnUnhandledRejection integration got a new option that allows you to make it behave like the cli flag.

Sentry.init({
  integrations: [
    new Sentry.Integrations.OnUnhandledRejection({
      mode: 'none'
    })
  ]
});

is (conceptually) same as --unhandled-rejection=none and the same goes for warn and strict. When you use warn (which is the default now), it’ll log the warning and error itself, but the process will be kept alive. When you use strict, it’ll log, capture the event, flush it (wait until its delivered) and kill the process with exit code 1.

@OliverJAsh ping 😃

@schumannd please reopen issue