-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Introduce additional types of exceptions next to mechanism.handled
exceptions
#10
base: main
Are you sure you want to change the base?
Conversation
unhandled
exceptionsmechanism.handled
exceptions
Co-authored-by: Manoel Aranda Neto <[email protected]>
Co-authored-by: Manoel Aranda Neto <[email protected]>
Co-authored-by: Manoel Aranda Neto <[email protected]>
Co-authored-by: Manoel Aranda Neto <[email protected]>
Co-authored-by: Manoel Aranda Neto <[email protected]>
yes |
Not really. Or, rather, technically yes, but not in all browsers, and not with really any useful data. The problem is, it's not a hook like (You can test it out by using this demo site.) |
Co-authored-by: Manoel Aranda Neto <[email protected]>
transfering questions from Eran Arkin A few questions:
Points 1 & 3 I think developers here can answer from experience working with and on Sentry point 2, % of customer based would be for all platforms where the boolean doesn't really apply. In theory they would all be impacted |
This is answered in the RFC already, but I can elaborate more:
The changes on the SDKs should be very quick, but I can't estimate filters, search, alerts etc, it's not part of my scope. |
Yep, currently the problem is that our app basically never crashes (as in the app quits, that's how the platform works), so there's no easy way to differentiate between exception which we caught ourselves vs those that were unhandled but didn't crash (as in quitting the app) the application. And those are the one we're interested in fixing.
No clue, since I'm not working for Sentry but for a company which is a paying enterprise customer. Workarounds are filtering based on the mechanism with which the exception was reported, but that requires that the SDK does add a mechanism in those cases and it also requires a probably unrealistic good understanding of the SDK and UI. I just happen to have contributed a lot of code to Sentry SDKs, so I'm not a good example. Since Sentry is just starting to push into areas where this makes a difference (the Flutter, Unity and React.Native SDKs are relatively new compared to the pure Android and iOS SDKs) this is also a growth opportunity and not just an improvement opportunity. |
I'd echo everything @marandaneto and @ueman's said, and add that for JS, what the customer gains is data which isn't totally bogus, which feels like a good thing in and of itself. I/we would have fixed the data long ago if there weren't UI/product consequences, but even then it would have been only semi-accurate, because the reality is that there really is a distinction between crashes and unhandled errors. If we were to fix the booleans to be the correct booleans tomorrow, people's crash rates on their releases would skyrocket, because all sorts of things we're incorrectly marking as handled would get marked as unhandled and therefore count as crashing the app, even though that's not reflective of reality. (There's the added complication I detailed above of what exactly does count as a crash in JS, especially on the browser side, but at least if there were three options, there'd be the possibility of reporting accurate data, and then it'd just be on us SDK devs to figure out what to send when.)
There's no real workaround here. As for impact, I don't know the total numbers, but the mere fact that it affects JS SDK users already means we're talking about a plurality of our customers, both paid and unpaid. I wouldn't be surprised if adding in all of the other affected platforms pushed us past 50%. |
FWIW, I wrote up the JS-specific parts of this issue here: getsentry/sentry-javascript#6073 The JS SDK team chatted and agreed that as a first step, we will start sending improved (even if not perfect) data somewhere else (TBD where) in the event, so that a) the scale of the problem can be quantified, and b) any backend or UI folks who do eventually pick up that end of the work have example data to work with. (This will likely be somewhere viewable in the JSON but not directly in the UI for the moment, since for now it's really just analytics.) I'll update here once exactly where and exactly what we'll send is hammered out. Once we do that, maybe other SDKs can do a similar thing. |
I agree in general with your idea of having three separate categories to split errors recorded manually by users, errors recorded by Sentry auto-instrumentation, and errors which crash the program. However, I think we should be taking a step back here to reconsider our terminology and how we indicate error severity to users. The current handled/unhandled terminology is somewhat misleading in my opinion, since at least in the Python SDK that I am most familiar with, it does not actually indicate whether the exception gets caught, but rather whether the exception was reported manually by the user or by the SDK itself via an instrumentation. Furthermore, the UI visually indicates "unhandled" exceptions as more severe than "handled" ones, and I question whether this is necessarily the case. For this reason, I propose changing the handled/unhandled terminology to something more meaningful, and I also propose creating a separate measure of error severity. Capturing mechanismFor the capturing mechanism, we should have two categories to separate manually and automatically captured errors. These would replace the current labels of "handled" and "unhandled," respectively.
Unlike the current “handled” and “unhandled” categories, automatically captured errors would not necessarily be marked as higher severity than manually captured ones. Severity levelsTo indicate error severity, we should have a separate concept of error severity. I propose three error severity levels as follows, but am open to other suggestions:
This separate severity level mechanism would be more flexible, since users would have the option to set a different severity level for certain errors or configure a different default level for certain classes of errors, instead of tying the severity level to how the error was captured. |
I generally like the idea of moving away from handled/unhandled. Two things:
For the mechanism, we already have the
I guess severity levels are hard to define well across different platforms. For example, in Browser JS we can’t detect a full crash and it basically never happens. Also besides Http error responses (which not all of our SDKs capture), it’s probably hard to distinguish between high/low reliably. Whatever we do here, if we want to solve this for the entire product, we need to coordinate this change with how we set the session status and, based on that, how we calculate release health. Maybe the key here is adapting the calculation to a certain platform, so that it makes (somewhat) sense for web, mobile and backend/server projects respectively. |
This RFC suggests a feature which introduces additional types of exceptions next to
mechanism.handled
.Currently, exception which cause the running software to exit are marked as
handled: false
. This isn't enough for SDKs where an exception can be unhandled but at the same time doesn't cause the software to exit. The aim of this RFC is to fix that.Rendered RFC
Disclaimer: I'm no Sentry employee. However, this is an issue I would really love to see fixed, and I was directed here, to write up my ideas. I hope that's okay.
This PR also looks a bit related to this issue: getsentry/relay#306