Errors that don't crash the app shouldn't be logged as fatal

[feinstein]

Problem Statement

Fatal usually means your app crashed (fatal == death), but FlutterErrorIntegration on line 64 logs all FlutterError as fatal instead of error, even though they are not crashing the app.

I have wrapped my app with runZonedGuarded and all errors are directed to Sentry so AFAIK the only way I can have a crash is from the native side.

Solution Brainstorm

The FlutterError level should be an option we can pass, so we can decide the severity it has.

Are you willing to submit a PR?

None

[buenaflor]

Thank you for the issue!

I don't see any reason why not. What's your take here @denrase

[ueman]

This conflicts with #456 and #1535

On the flutter side, it can also be changed via beforeSend or event processors.

In the end this change would also affect the release health to make it look better than it actually is.

Also relates to getsentry/rfcs#10

[buenaflor]

I see, thx for the context!

Then I'd also suggest using beforeSend or event processors to change if necessary.

[feinstein]

I thought the Sentry plugin would mark an exception as fatal, if it was detected on the native side that the last time the app was executed it crashed.

I don't think this makes the release looks better than it is, since having a crash is way worse than having an error.

If you crashed (thus fatal), than your user can't do anything else. If you had an exception that wasn't caugh, but didn't crash your app, there's a big chance your app is still able to run, and your user can still use the app and enjoy it (hence not fatal, but an error).

If I use beforeSend to change every event from fatal to error, than I would never have a fatal alert from the dart side, side I run the app inside a runZonedGuarded. But what happens if I have custom native code and it crashes the app? Does Sentry detect it on the native side and sends a fatal alarm on the native integration? Or does Sentry delegate to dart's next run and beforeSend would wrongly mark a crash that should have been fatal as an error?

[ueman]

I basically agree with your points. However, the nuance here is that the SDK doesn't actually know about fatal crashes. It just knows about handled or unhandled exceptions right now. (This is explained in more detail in the linked RFC)

On Sentry every unhandled exception is marked as fatal.

Because Flutter typically doesn't crash at all, the next worst exception the unhandled one, which is now marked as fatal, since Sentry can't yet differentiate between unhandled + crashing, unhandled + not crashing and handled.

But what happens if I have custom native code and it crashes the app? Does Sentry detect it on the native side and sends a fatal alarm on the native integration?

Yes, native crashes will still be marked as fatal if they crash the app, since they don't go through the beforeSend on the Dart side.

[denrase]

@feinstein Pretty much what @ueman described. There was a larger effort to bring the SDKs in line and we will be marking errors that occurred in runZonedGuarded, SentryIsolate and FlutterErrorIntegration as handled: false, as they don't crash the app, but are also not handled. This will be part of the upcoming 8.0.0 release.

However, this is separate from the level, which in all cases is set to fatal, I'll check what other SDKs are doing and get back to you.

[denrase]

Ok, I checked out the native SDKs, which are responsible for release health session handling.

Android

When an uncaught exception occurs, the event is marked as fatal. This is in the case that the host application (thread) is crashed.

Also, an event is interpreted as crahsed if the mechanism is unhandled in android. The level does not apply.

https://github.com/getsentry/sentry-java/blob/3cda41bf9cbd76abd99f957e3e8ed921cc779b87/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java#L96

https://github.com/getsentry/sentry-java/blob/3cda41bf9cbd76abd99f957e3e8ed921cc779b87/sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java#L163

iOS

For the session to be recreated state we check if the the event was unhandled and if the level was higher than error. As far as i can tell changing the level from fatal to error would not mess with session/health data.

https://github.com/getsentry/sentry-cocoa/blob/881a95561aedd5bac232089866de9a476d18c85a/Sources/Sentry/SentryHub.m#L606C15-L606C53

Conclusion

I don't think changing the level property from fatal to error will have an effect on release health & sessions. I'm not sure yet if we can do this change without other consequences though.

I checked out Crashlytics, as @ueman also linked to the documentation in one of the above issues. In their example setup of an application, they let users decide if those captured errors should be captured as fatal or not.

https://github.com/firebase/flutterfire/blob/4e9bb694ad13cd06ecc6c1936cab0a6b9c77273c/packages/firebase_crashlytics/firebase_crashlytics/example/lib/main.dart#L33

[feinstein]

So if we have a native crash it will appear as fatal, and if we have an uncaught exception that didn't break the app, it will be marked as handled: false. This meets what I would expect.... are you still investigating if the level could be changed to error or if you are going to provide an easy way to configure it like crashlytics does?

[denrase]

are you still investigating if the level could be changed to error or if you are going to provide an easy way to configure it like crashlytics does?

Yeah pretty much need to look into why we have fatal here. Will discuss with @buenaflor and @kahest

[knaeckeKami]

There is also an inconsistency on what "crash" means on the "Releases" section.

Releases overview page show a number of "Crashes": https://imgur.com/a/PRXmJyL

But when you click on it, the filter is set to "error.unhandled:true": https://imgur.com/a/kbqc7tg
Which in my case is empty. So it's not immediately clear where the number of crashes comes from.

[krystofwoldrich]

• Related RFC Introduce additional types of exceptions next to mechanism.handled exceptions rfcs#10

[buenaflor]

@knaeckeKami this might be related to getsentry/sentry-cocoa#3353

[denrase]

@buenaflor I assigned you if it's ok, as it seems there's still an internal discussion needed to resolve this. Think the only open question is if the level can be changed: #1624 (comment)

[buenaflor]

I'll bring this up internally.

[buenaflor]

@feinstein

We decided to provide the abiility to optionally decide whether to report it as fatal or error. By default it will stay the way it is now.

[denrase]

@feinstein We have merged the PR #1738, it will be part of one of the next releases. Thank you for bringing this up!