Improve exception grouping on Android for RuntimeExceptions

[markushi]

📜 Description

Add exception_id and parent_id to describe the outer/inner exception relationships.

Also add is_exception_group=true to java.lang.RuntimeException - not sure if we should apply this globally or not? Maybe @adinauer can provide some backend feedback here?

💡 Motivation and Context

Fixes getsentry/sentry#64074
This affects exception grouping and determining issue titles, RFC details are here.

Funny enough the same outcome can be achieved by doing nothing from the above and simply sending the exception->values in reverse order (right now it's values = [inner, outer])

💚 How did you test it?

📝 Checklist

• I reviewed the submitted code.
• 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.
• 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

[github-actions]

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 8c4a254

[adinauer]

A RuntimeException by itself isn't an exception group, there's suppressed exceptions which we have an issue for (#542) but I'm guessing that has little to do with the issue we're trying to fix here.

[adinauer]

Funny enough the same outcome can be achieved by doing nothing from the above and simply sending the exception->values in reverse order (right now it's values = [inner, outer])

This sounds like something that wouldn't apply to all examples. Let's assume a NullPointerException which is caught and rethrown a bunch of times. Now multiple separate code problems would end up in the same NPE issue, just because we use the innermost exception. They should still end up as separate issues in Sentry though.

My suggestion would be to unwrap the RuntimeException in case of Android if we're sure this is causing problems.

I'm not a fan of hacking this in via the exception groups feature as I'm unsure whether this would cause problems when we actually get around to implementing suppressed exception support (#542).

[lobsterkatie]

Yeah, we definitely don't want to just reverse the order, for exactly the reasons @adinauer said, and because it would confuse the UI.

Also, IMHO it should definitely be scoped to this particular wrapper exception, lest we overcompensate and end up creating the exact problem that JS is having in getsentry/sentry#59679.

As for unwrapping vs using is_exception_group:

• Are the error message and stacktrace of the wrapper error always the same and/or do they provide any informational value? If no, then I'd vote for unwrapping.
• I agree that this is kind of abuse of is_exception_group, in order to trick our logic here and here into setting the correct main_exception_id. It would be a stopgap until the time if and when we make this logic smarter. See With chained errors, which one counts as the main one? sentry#64088.

[romtsn]

Are the error message and stacktrace of the wrapper error always the same and/or do they provide any informational value? If no, then I'd vote for unwrapping.

Most of the time should be the same, but might differ between OS versions depending on what they've changed. I agree we should scope this to Android only and probably unwrapping is a better way forward here. This is also only a case for checked exceptions, so if we could narrow down to only those on Android would be great.

[adinauer]

Could we do the unwrapping during Sentry processing instead of the SDK?

• Not sure if obfuscation is a problem here
• We could adapt this for future (Android) versions without requiring customers to upgrade their SDK and their users in turn to upgrade the App.

[markushi]

Thanks for chiming in @lobsterkatie!

Also, IMHO it should definitely be scoped to this particular wrapper exception, lest we overcompensate and end up creating the exact problem that JS is having in getsentry/sentry#59679.

Agreed! Unfortunately we could only do a best-effort here, as this java.lang.RuntimeException is not specific to Android, and could be also thrown by anyone anywhere.

Are the error message and stacktrace of the wrapper error always the same and/or do they provide any informational value?

Whilst the wrapped exception itself and it's stacktrace don't provide any extra information, it indicates one crucial semantic information: An app crash is about to happen.

Thinking more about future solutions: I'm wondering if in situations where the wrapping exception contains no in-app frames, we simply could decide to just use the inner exception for titleing.

[lobsterkatie]

I'm wondering if in situations where the wrapping exception contains no in-app frames, we simply could decide to just use the inner exception for titleing.

Interesting. I added this in getsentry/sentry#64088 as an idea to consider.

[adinauer]

I'm wondering if in situations where the wrapping exception contains no in-app frames, we simply could decide to just use the inner exception for titleing.

Hmm haven't thought this through but e.g. config issues could end up with no in-app frames but still be caused by the user. Same goes for library bugs that may e.g. affect request handling where the exception doesn't go through user code.

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	385.80 ms	470.94 ms	85.14 ms
Size	1.70 MiB	2.27 MiB	586.79 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
93a76ca	381.08 ms	459.22 ms	78.14 ms
b7fa26f	440.10 ms	533.98 ms	93.88 ms
1f3652d	367.21 ms	438.46 ms	71.25 ms
86f0096	368.63 ms	446.92 ms	78.29 ms
7eccfdb	389.94 ms	461.29 ms	71.35 ms
d6d2b2e	392.55 ms	467.50 ms	74.95 ms
d6d2b2e	463.14 ms	545.56 ms	82.42 ms
c7e2fbc	372.00 ms	461.71 ms	89.71 ms
8838e01	387.41 ms	467.00 ms	79.59 ms
eecfab6	399.27 ms	461.82 ms	62.55 ms

App size

Revision	Plain	With Sentry	Diff
93a76ca	1.72 MiB	2.29 MiB	576.75 KiB
b7fa26f	1.70 MiB	2.27 MiB	583.82 KiB
1f3652d	1.72 MiB	2.27 MiB	554.95 KiB
86f0096	1.72 MiB	2.29 MiB	576.50 KiB
7eccfdb	1.72 MiB	2.27 MiB	556.93 KiB
d6d2b2e	1.72 MiB	2.27 MiB	555.05 KiB
d6d2b2e	1.72 MiB	2.27 MiB	555.05 KiB
c7e2fbc	1.72 MiB	2.29 MiB	576.40 KiB
8838e01	1.72 MiB	2.29 MiB	578.15 KiB
eecfab6	1.72 MiB	2.27 MiB	558.56 KiB

[markushi]

In the meantime I stumbled upon the synthetic flag in our exception interface.

An optional flag indicating that this error is synthetic. Synthetic errors are errors that carry little meaning by themselves. This may be because they are created at a central place (like a crash handler), and are all called the same: Error, Segfault etc. When the flag is set, Sentry will then try to use other information (top in-app frame function) rather than exception type and value in the UI for the primary event display.

IMHO this sounds awfully similar to what we actually want to achieve here. @lobsterkatie would this make sense for you?

[markushi]

Closing this one for now in favor of the different issues linked above.