Set up Sentry for Android-native code #4996
Conversation
gnprice
force-pushed
the
pr-sentry-android
branch
from
e5d3e75 to
eb20490
Compare
|
Thanks for the review! Yeah, a good improvement to this code would be to unify things so that each error handler only has to make one call instead of two. Our @chrisbobbe also pointed out (in the office yesterday) that we already have been getting some Sentry reports that look like they come from the Android-native side. For example here's one with a Java stack trace: I haven't actually done a negative test of this: I didn't try reporting a warning, or inducing an exception, before this "Enable Sentry" commit. I suspect what's happening is:
I'll do some investigation to confirm. |
|
OK, some findings:
|
Looks like this is probably getsentry/sentry-react-native#1102 . The person reporting that issue says it was limited to a debug build, and worked fine in a release build. (Easy to imagine how that could come about, with the red-screen mechanism interfering somehow.) If that's the case then this is… still an annoying Sentry bug that they should really fix, because it causes confusion for developers, but it doesn't have a direct effect on error reporting in the field, which is nice. |
|
Great, thanks for that investigation! I'm satisfied with it. |
gnprice
force-pushed
the
pr-sentry-android
branch
from
eb20490 to
31dd498
Compare
|
Pushed a revision with |
| @@ -87,5 +87,12 @@ | |||
| <action android:name="com.google.firebase.MESSAGING_EVENT" /> | |||
| </intent-filter> | |||
| </service> | |||
|
|
|||
There was a problem hiding this comment.
(IIUC)
I think it makes sense that getsentry/sentry-react-native#1102 would be the issue with TextCompressionModule#decompress, and that that report would be accurate about the problem being debug-mode-only. But I don't think we're 100% sure, right?
This should immediately mean that we start reporting to Sentry when
an exception propagates uncaught and crashes the app.
So I wonder if this deserves an asterisk on the word "should", just in case we find an uncaught exception not being reported to Sentry in the future? Maybe just a link to #4996 (comment).
There was a problem hiding this comment.
Sure; added a note about that.
| public fun warnException(e: Throwable) { | ||
| Sentry.withScope { scope -> | ||
| scope.level = SentryLevel.WARNING | ||
| Sentry.captureException(e) |
There was a problem hiding this comment.
Interesting, I think I'd expect that you'd have to pass the scope param somewhere in the Sentry.captureException call. But I guess you don't: https://docs.sentry.io/platforms/java/enriching-events/scopes/#local-scopes
There was a problem hiding this comment.
Yeah. I think this API actually makes more sense than it would if you did have to pass it: basically the point is that there's always a current scope (there has to be, to handle uncaught exceptions where you don't explicitly invoke Sentry at all), and so Sentry.captureException uses that.
The withScope function is sort of doing two things at once: it pushes a new scope onto the scope stack (and undertakes to pop it at the end), and then it passes you the scope much like Sentry.configureScope does. I think that's a bit confusing, and if I were designing this API, I'd probably separate those, so that you'd write:
Sentry.pushScope {
Sentry.configureScope { scope -> scope.level = SentryLevel.WARNING }
Sentry.captureException(e)
}(Well, and I'd probably make configureScope a bit less cumbersome too, like:
Sentry.pushScope {
Sentry.currentScope.level = SentryLevel.WARNING
Sentry.captureException(e)
})
This should immediately mean that we start reporting to Sentry when an exception propagates uncaught and crashes the app. It also means we can start explicitly logging things to Sentry when something goes wrong that we do anticipate and catch. It's actually already possible to invoke Sentry, but it only works when our JS code has already been started up, for example in a method exposed to JS with `@ReactMethod`. We want to use it from places like the notifications code, which can run without starting JS. Tested manually by following this comment's instructions with a DSN pointing to a test "project" in Sentry (as well as the existing instructions in `src/sentryConfig.js`), and inserting (into `MainActivity#onCreate`) both a Sentry warning and then just an uncaught exception. Both were successfully reported to Sentry, appearing in the test project's dashboard. There is an asterisk: when I tried inducing an uncaught exception inside a `@ReactMethod` method, I got (in a debug build) a red-screen modal as expected, but no report to Sentry. That seems like a Sentry bug. But it's a low-priority issue for us, in that AFAIK we've never had an issue that would cause one of those. Moreover, there looks to be a report of it in the Sentry tracker, which says it's specific to debug -- that it works fine in a release build. Discussion here: zulip#4996 (comment)
Like `Sentry.captureException`, but at warning level.
The one for malformed FCM messages may be especially informative as we start treating FCM messages from very old servers (that lack important pieces of information) as malformed. More generally this covers all of our call sites to `Log.w` and above. We should use `Zlog.w` and `Zlog.e` in place of those for any future call sites, too -- the `Log` call gets it to the device log, and that could be quite handy for development, but for issues encountered in a published build the Sentry call will get it to our Sentry dashboard, which is much more likely to be helpful. This change drops the message strings that were at each of these specific call sites. * In Sentry, there isn't a good place to put those -- the exception type and message are the most salient pieces of information -- and because that's our most valuable channel for debugging, it didn't seem like a good idea to have the `ZLog` API accept a message that we weren't going to be able to make good use of in Sentry. That's why we drop them. * In the device log, passing a throwable to `Log.*` causes the throwable's class name and message to get printed, as well as the stack trace. In each of these cases, that should cover all the information that was in these messages anyway. In particular, the most informative of these messages are really just restating the name of the class that we already know the exception is an instance of. So we're not losing anything material by dropping them, even when reading the device log rather than Sentry.
gnprice
force-pushed
the
pr-sentry-android
branch
from
31dd498 to
ab3ab62
Compare
This means we'll start seeing reports in Sentry of both uncaught exceptions, and error or warning cases we explicitly catch, in our Kotlin code just as we do already in our RN-based JS code.