-
-
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
Node suggestion: log errors in unhandledRejection
#1909
Node suggestion: log errors in unhandledRejection
#1909
Comments
Sounds good, however, I'm not sure if this should be behind our |
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. 🤔 |
We don't modify default logging behavior, and for uncaught exceptions it's node itself what's triggering a log call. I think the best solution here would be to just make a note about it in the docs and leave process.on('unhandledRejection', (reason, p) => {
console.log('Unhandled Rejection at:', p, 'reason:', reason);
}); snippet in case someone wants to re-enable the warning. |
Docs would suffice. At least then Sentry isn't doing anything special. It's really Node that's the problem here. |
On second thoughts, I'm not sure that's true. Sentry registers an The handler ( If we're "restoring" logging for uncaught exceptions, I believe we should do the same thing for unhandled promise rejections. |
Can we re-open this? As per my last comment, I now believe Sentry should restore logging for unhandled promise rejections, like it does for uncaught exceptions. |
I found this issue because I was starting to use Sentry and wondered if I had made a mistake in my code since only So I would say either log both or log neither. |
I found a better way to handle this, than copying code from Node's core. λ: node --unhandled-rejections=warn app.js Also made it obvious by including on the main docs page getsentry/sentry-docs#1099 |
I still find it's weird that |
Because one is critical ( If we reverse the order and emit the warning by default, we'll break CLI behavior, by emitting them despite Once Node decides to make |
@kamilogorek, but when running with |
@freeall only if you don't attach process.on('unhandledRejection', () => `¯\_(ツ)_/¯`); And we state very clear here that we do that: https://docs.sentry.io/platforms/node/default-integrations/
I just don't want to change node's behavior, that's it. And behavior is: "if there's a listener, don't emit. if you still want the warning, use flag." - and this is exactly what we do. |
@kamilogorek I understand where you're coming from. But I think users of Sentry would expect Sentry to not alter the behavior of their program. If I have an I personally don't like that Sentry alters the behavior. |
But that's how Node.js is designed ¯_(ツ)_/¯ |
You are of course right about how node is designed. When you attach a handler the warning is gone. Anyway, it seems you're set on this behavior so no point in keep the discussion going. But thanks for taking the time to answer :) |
@freeall thanks as well, it's always good to see the both sides :) |
Just to clarify: when enabling Sentry, the behaviour of
"As expected" here is assuming the user understands that Sentry is registering a listener for I do see your point though. Sentry should also respect |
@freeall comment sums this up pretty well:
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 )
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 |
@schumannd I've added a snippet from how I load it. And I agree with you that the "it's not a bug, it's a feature" answer doesn't seem fulfilling. I fear that many programmers won't catch some bugs in their program because Sentry eats them. To me, the first priority of a tool like Sentry should be to catch bugs, and not to create them. ...
if (isUsingSentry) {
// Log to console because Sentry overwrites standard behavior. https://github.com/getsentry/sentry-javascript/issues/1909.
// Note that it doesn't overwrite for uncaughtException.
process.on('unhandledRejection', console.error)
}
... |
As I am using sentry in react native, it seems like I would need to do some hacky things to fix this issue. Is anyone using React Native and solving this problem in a different way? |
@OliverJAsh, @kamilogorek 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. |
So, a bit of debugging reveals the reason console logging is suppressed: Sentry assigns a function to Luckily, it stores a reference to any existing function, and calls that if it exists.
Now, please fix this, so we can have the default behavior without such pointless hacks 🙂 |
@schumannd please reopen issue |
@thomas-darling I agree with the browser change, it should return However, for node, I'm still not convinced because of one reason. It ties the code to current Node implementation. If we'll copy the internals instead of relying on flags, and promise rejection behavior will change in v14, we'll have to detect what version of node we are in and act accordingly. |
Sounds good, regarding the browser change 👍 As for Node, if you don't fix logging in Sentry, you basically just force all your users to do it themselves - with the added risk that some will do it wrong, and some will not even realize they need it, until after they get bitten by a production bug, like I did. That's not a good developer experience... |
@thomas-darling how would you like to get it fixed? Reproduce the same code that's inside node's code? At the very top of our docs there's a very visible note what to do in order to get default console logging - https://docs.sentry.io/platforms/node/ |
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. |
As I see it there a few options:
I think option 1 or 2 both seem fine. Your customer sees the error and can fix it. 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. |
This should do the work. #2312 Sentry.init({
integrations: [
new Sentry.Integrations.OnUnhandledRejection({
mode: 'none'
})
]
});
process.on('unhandledRejection', (reason) => {
// your callback
}) |
for me that results in |
@schumannd |
@kamilogorek looks good to me 👍 |
Thank you for doing this! Would you be able to ping here once this is released? |
@OliverJAsh ping :) |
Just to check I understand correctly: if I use Node's I ask because when I tried enabling It would be great if we could add some docs around this! |
Docs PR is already in progress getsentry/sentry-docs#1351 @OliverJAsh this change has nothing to do with the cli flag. It's behavior is untouched. The thing that changed is that Sentry.init({
integrations: [
new Sentry.Integrations.OnUnhandledRejection({
mode: 'none'
})
]
}); is (conceptually) same as |
That makes sense. I understand the integration doesn't change the behaviour of the Node flag. However, can I just check I correctly understand how Sentry behaves (outside of this integration) with regards to the Node flag?
|
@schumannd FYI, this morning we released |
@lobsterkatie the latest release of @sentry/react-native seems to be 1.3.7.. So trying to install 1.10.0 does not work. How do I get the fix? |
@schumannd @lobsterkatie meant |
Package + Version
@sentry/browser
@sentry/node
raven-js
raven-node
(raven for node)Version:
Description
Out of the box, Node will log unhandled promise rejections. However, after initialising Sentry, these logs will disappear. This happens because Node won't log rejections if an
unhandledRejection
handler exists, and Sentry registers such a handler.I would like to suggest that Sentry adds logging to its handler, to provide parity with the experience provided out of the box. At the very least, the docs should mention this behaviour and the need to add manually an additional handler to restore logging.
The text was updated successfully, but these errors were encountered: