Closed lobsterkatie closed 1 year ago
We agreed that as a first step, we'd send better data in a different spot, so a) we can compare it to the bad data to see how often we're wrong, and b) we can give whoever might end up working on this eventually correct data to play with.
Tasks for this first step:
other
or meta
)captureException
comes from @sentry/core
(https://github.com/getsentry/sentry-javascript/pull/6488)heads-up @mydea just assigning you b/c you've taken a look at the PR. Doesn't mean we have to continue with this.
Hey all!
I recently had to take a look at Sentry source code for an unrelated issue and to my surprise I discovered that handled: true
tags do not mean that an error is handled as pointed out in this issue:
Errors which are caught by our auto-instrumentation (1, 2, 3, 4, 5) are marked as handled, even though they have not, in fact, been handled by the user.
When triaging errors, we focus our efforts first on unhandled errors before considering other issues that are handled (since, nominally, these have been correctly handled from the POV of the user).
I noticed there were a couple PRs that started to address this but have since been closed. Am I misunderstanding the expected semantics for the handled
tag? Does handled: true
mean that the error has been logged by some mechanism in Sentry prior to global event listeners and not that is has been handled from application code?
Hi @taj-p thanks for reminding us about this. We had to abandon changing behaviour here due to prioritization of other projects but I this again in our team and we talked about going with a pragmatic solution here - basically a subset of the proposed changes from the RC:
handled: true
by defaulthandled: false
This way, when users manually call captureException
their errors are considered handled. When we capture an error, it's considered unhandled.
We're not gonna get to this this week but I'll bring it up in planning for next week(s). Can't promise any ETA though.
As can be seen by the linked PRs above, we had to adjust quite a few of our internal handlers/wrappers to always mark errors caught by us (i.e. automatically from the SDK) as unhandled. Here's a list for future reference:
@sentry/browser
@sentry/react
@sentry/vue
@sentry/nextjs
@sentry/remix
@sentry/serverless
@sentry/integrations
I'm going to close this issue as we're releasing the changes listed above as I'm typing this. If you have concrete issues around specific mechanism adjustments please open a new issue. If you have general comments, feel free to comment here.
Problem Statement
TL;DR: We send wrong data in the
handled
field, we conflate unhandled errors and crashes, and even if we fixed the former we'd still need to find effective ways to differentiate the latter from one another.Stolen from my comment on https://github.com/getsentry/rfcs/pull/10:
Other relevant bits of that thread: https://github.com/getsentry/rfcs/pull/10#issuecomment-1267718900 https://github.com/getsentry/rfcs/pull/10#issuecomment-1289888297
Solution Brainstorm
There are a few separate but intertwined threads of what has to happen to really fix this:
We need the sentry server and UI to be able to handle three options rather than two.
We need to correct our blatantly incorrect booleans
We need to find effective ways to differentiate between unhandled errors which don't really block users and ones which actually break stuff (the closest we're going to get to actual "crashes" in browser-land, at least until a real minidump-like API is introduced for browser tabs - for obvious reasons this doesn't count).
Note: As per https://github.com/getsentry/sentry-dart/issues/1116#issuecomment-1308340171, we should coordinate with the mobile folks when we actually start working on this. EDIT: Turns out they can already do the data gathering for the dart SDK. Might nice to try sending correct data from RN, though.