Add CoroutineExceptionHandler

[lcian]

📜 Description

Adds a CoroutineExceptionHandler, similar to UncaughtExceptionHandler for exceptions thrown in coroutines.
It can be used together with SentryContext when launching coroutines.

💡 Motivation and Context

Closes #1524
I have tried https://github.com/Anamorphosee/stacktrace-decoroutinator as well but it doesn't really improve the stack trace and also only supports being enabled using a plugin in Android.

💚 How did you test it?

Unit tests and manual test in the UI triggered by button in Android app

📝 Checklist

• I added tests to verify the changes.
• No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
• I updated the docs if needed.
• I updated the wizard if needed.
• Review from the native team if needed.
• No breaking change or entry added to the changelog.
• No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Update the docs

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	413.08 ms	427.98 ms	14.90 ms
Size	1.58 MiB	2.08 MiB	505.39 KiB

Previous results on branch: lcian/feat/coroutine-exception-handler

Startup times

Revision	Plain	With Sentry	Diff
d6b98a4	388.40 ms	398.72 ms	10.32 ms

App size

Revision	Plain	With Sentry	Diff
d6b98a4	1.58 MiB	2.21 MiB	646.60 KiB

[lcian]

This is how the issue and stack trace look like when running it from the sample app with an intermediate function, like so:

    fun throwInCoroutine() {
        GlobalScope.launch(SentryContext() + SentryCoroutineExceptionHandler()) {
            someFunction()
        }
    }

    fun someFunction() {
        throw RuntimeException("Exception in coroutine")
    }

[adinauer]

LGTM, though my familiarity with coroutines is limited. I'd say we get this into the hands of our customers to get some feedback. We can always iterate on it, especially since it's marked experimental for now.

[lcian]

So this is how exceptions show up with the handler, note that they do not crash the app as they get handler by the handler

This is what happens without the CoroutineExceptionHandler. In this case the app crashes and so the exception is caught by our normal UncaughtExceptionHandler:

I guess this handler improves things as it makes it clearer that the exception was thrown in the coroutine and we don't have the confusing DiagnosticCoroutineContextException attached.

Still, I'm not sure if the handler should prevent the app from crashing if that's what happens when we're not using.
And also this doesn't seem very useful overall as we don't get a nice stack trace, just the name of the function that threw the exception (f in this case), even though we do have some intermediate functions that are not appearing here.

What do you think? @adinauer @romtsn

[adinauer]

Still, I'm not sure if the handler should prevent the app from crashing if that's what happens when we're not using.

I missed that part on my first review. We usually only report the error to Sentry but still pretend we're not there when it comes to exceptions. Not sure if the error handler design even allows us to do this here.

And also this doesn't seem very useful overall as we don't get a nice stack trace, just the name of the function that threw the exception (f in this case), even though we do have some intermediate functions that are not appearing here.

I believe that's just how Kotlin generates stack traces for coroutines and you need to reproduce with debug mode to have stacktrace recovery (https://github.com/Kotlin/kotlinx.coroutines/blob/master/docs/topics/debugging.md#stacktrace-recovery).

@romtsn do you see potential in this where it makes sense to release it?

It might be confusing our customers if this is the only Sentry exception handler that catches the exception for them.
While confusing, i don't see DiagnosticCoroutineContextException as a huge problem if that's the only thing this handler is ever going to improve.

[lcian]

About the fact that it's being swallowed by the handler, we could also just rethrow it, but then it would show up twice in Sentry obviously.

[adinauer]

Yeah let's wait for @romtsn maybe we shouldn't merge this PR in its current state / feature set.

[romtsn]

If it's low maintenance for us (imo it is, the code is not gonna change much, and we don't have to introduce a new package for this), I think it's fine to ship it as-is and add docs. I guess it's no different from all the logging integrations we have, so people don't have to call Sentry.captureEvent themselves.

And I also think it's fine that it prevents the app from crashing as this is how CoroutineExceptionHandler is designed, and folks who use it are aware of that behavior most likely (but ofc worth to mention in bold when adding docs).

Since we also made the class open we could add docs showing how to inherit from it if you already have an existing CoroutineExceptionHandler but additionally want to report to Sentry.

[romtsn]

we could also set handled = true here I guess, because this is what the handler does, right?

[lcian]

If it's low maintenance for us (imo it is, the code is not gonna change much, and we don't have to introduce a new package for this), I think it's fine to ship it as-is and add docs. I guess it's no different from all the logging integrations we have, so people don't have to call Sentry.captureEvent themselves.

And I also think it's fine that it prevents the app from crashing as this is how CoroutineExceptionHandler is designed, and folks who use it are aware of that behavior most likely (but ofc worth to mention in bold when adding docs).

Since we also made the class open we could add docs showing how to inherit from it if you already have an existing CoroutineExceptionHandler but additionally want to report to Sentry.

Good points, thanks @romtsn !