Re-enable Sentry on Android, with `enableNdk: false` #5765

chrisbobbe · 2023-09-21T00:31:13Z

~~Our hope here is to fix the Android crashes in #5757. Details in the commit message.~~

~~Edit, following #5765 (comment): This did not fix it. Subsequent testing showed that the crash still happens at this later version.~~

Edit, following #5765 (comment): This PR was originally just an upgrade of @sentry/react-native. It still does that, but goes to a later version, where they added an enableNdk option, and it uses that option for the sake of #5766.

Fixes: #5766

chrisbobbe · 2023-09-21T01:00:52Z

Closing, as our initial testing was found to be invalid. The DSN was accidentally not included, so the app wasn't told where to send Sentry events. In a subsequent test in which the DSN was included, the crash reproduced: https://chat.zulip.org/#narrow/stream/48-mobile/topic/Android.20crashes/near/1644202

chrisbobbe · 2023-09-21T01:03:06Z

Ah, as Greg points out in chat:

We'll still want the upgrade #M5765 — if Sentry comes out with a fix, we'll be best placed to take it having already upgraded to the current latest. But we don't need to rush to merge it today.

So I'll reopen, but I'll update the branch and PR description to not say it fixes the crash. (edit: Done.)

gnprice · 2023-10-04T00:41:21Z

OK, I spent some time today investigating this. I've just pushed back to this PR a branch with several further changes:

Upgrade to the newer 5.10.0, which adds an option enableNdk
Set that option to false on Android
Revert e8c57e7 / Disable Sentry for now on Android (cherry-pick from v27.213) #5768, to re-enable Sentry for our normal release builds on Android, fixing Re-enable Sentry on Android (without crashes) #5766

That's based on some testing which confirmed that enableNdk: false appears to resolve the crashes, and that we do get Sentry events after these changes. Details in chat thread: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/re-enabling.20Sentry/near/1654107

Then for the types, I spent a bit of time trying to get working type definitions through TsFlower. My conclusion is that given that we're planning to mothball this codebase soon, it's not worth it. Instead, we just keep the existing hand-edited type definitions we have and add the one new piece of API we use (namely that enableSdk option).

In particular, there's a fairly small API surface area that we actually use from Sentry; and it's pretty infrequent (was pretty infrequent, even when zulip-mobile was the codebase we were developing full time) that we make changes in the parts of our codebase that interact with Sentry directly. So the value from having completely accurate type definitions for Sentry is fairly low.

And conversely, the internals of the Sentry type definitions are fairly complex, type-system-wise. It's possible that if we buckled down to it, then adding to TsFlower whatever it needs in order to handle them wouldn't be too hard… but I'm pretty sure it'd be a few hours, at minimum. And given the above, that's more work than it's worth to us.

chrisbobbe · 2023-10-04T22:03:32Z

wip/ref Revert "sentry: Disable Sentry for now on Android"

What's left to do here, indicated by "wip/ref"? 🙂

chrisbobbe · 2023-10-05T01:35:20Z

Thanks for that work! Pushed back some changes including:

Removed some "wip"s in commit messages
Added back the commit to have us upload Android debug symbols

gnprice

Thanks! Two comments below.

I'll go ahead and merge with these changes, unless you think the "Turn on debug-symbol uploads" commit is essential and there's something else that should be adjusted if we leave it out.

gnprice · 2023-10-05T20:44:22Z

android/app/build.gradle

+    apply plugin: "io.sentry.android.gradle"
+    // https://docs.sentry.io/platforms/android/configuration/gradle/#configure
+    sentry {
+        uploadNativeSymbols = true
+        includeNativeSources = true


I think "native" here refers to actual native code, i.e. machine code, the kind whose source code might be C or C++ or Rust.

(In a React Native context on Android the term "native" is ambiguous, as it often means JVM code like one writes in Java or Kotlin. But here this is Sentry's Gradle plugin for Android in general, not specific to React Native; and in a general Android context, JVM code is just the normal kind of code, and "native" always means machine code.)

In that case, these symbols and sources are things that could be useful for interpreting events reported for exceptions in native code on Android — but reporting those events is exactly the functionality we turn off in our Sentry config later in this PR.

So I think this piece of configuration won't actually do anything for us, and we can leave out this commit.

(If we were including it, I'd want to spend a bit more time looking through what this Gradle plugin does and validating that it seems unlikely to break anything. For example, one of the listed features at https://docs.sentry.io/platforms/android/configuration/gradle/ is "Auto-instrumentation tracing through bytecode manipulation", which sounds like something that could cause hard-to-debug problems if buggy; and from the config docs lower down on that page, it looks like that's enabled by default.)

gnprice · 2023-10-05T20:46:30Z

src/sentry.js

+            // memory-corruption bugs that cause the app to crash.
+            //   https://github.com/zulip/zulip-mobile/issues/5766
+            //   https://github.com/getsentry/sentry-java/issues/2955#issuecomment-1739030872
+            enableNdk: false,


Greg found in testing that this, plus re-enabling Sentry on Android (coming up) seems adequate for #5766.

In this commit message let's include a link for that testing, since that's easy to do:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/re-enabling.20Sentry/near/1654140

Done by following the upgrade guide at https://docs.sentry.io/platforms/react-native/migration/ starting in the "From 3.x to 4.x" section and working up through "From 4.x to 5.x". Not many declared breaking changes relevant to our app: - The `Severity` enum is removed in favor of string literals - The Sentry CLI had a big upgrade with declared breaking changes to handle in our iOS build We leave our existing hand-edited type definitions in place, even though they're doubtless increasingly out of date. Given that we're planning to mothball this codebase fairly soon, it's not worth the effort to try to systematically update them.

This introduces the `enableNdk` option at Sentry initialization time: getsentry/sentry-react-native@2c394d1 We add that option to our hand-edited type definitions, because we intend to use it.

Using the new option added in @sentry/react-native 5.10.0: https://github.com/getsentry/sentry-react-native/releases/tag/5.10.0 Greg found in testing: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/re-enabling.20Sentry/near/1654140 that this, plus re-enabling Sentry on Android (coming up) seems adequate for zulip#5766.

This reverts commit e8c57e7, except for its updates to the changelog. Fixes: zulip#5766

chrisbobbe · 2023-10-05T20:50:41Z

I'll go ahead and merge with these changes, unless you think the "Turn on debug-symbol uploads" commit is essential and there's something else that should be adjusted if we leave it out.

Sounds good, please go ahead and merge with those changes! 🙂 Thanks.

chrisbobbe added a-Android severe: crash The app quits, or stops responding. P0 critical Highest priority labels Sep 21, 2023

chrisbobbe requested a review from gnprice September 21, 2023 00:31

chrisbobbe closed this Sep 21, 2023

chrisbobbe reopened this Sep 21, 2023

chrisbobbe force-pushed the pr-sentry-react-native-5-9-2 branch from bc47c24 to beba72d Compare September 21, 2023 01:11

chrisbobbe removed severe: crash The app quits, or stops responding. P0 critical Highest priority labels Sep 21, 2023

chrisbobbe mentioned this pull request Sep 21, 2023

The number of Crashes and ANRs has increased getsentry/sentry-java#2955

Closed

gnprice mentioned this pull request Sep 21, 2023

Android app crashing #5757

Closed

chrisbobbe added the dependencies Pull requests that update a dependency file label Sep 21, 2023

gnprice force-pushed the pr-sentry-react-native-5-9-2 branch from beba72d to 6ac949c Compare October 4, 2023 00:31

chrisbobbe force-pushed the pr-sentry-react-native-5-9-2 branch from 6ac949c to b731fc8 Compare October 5, 2023 01:32

chrisbobbe changed the title ~~deps: Upgrade @sentry/react-native to 5.9.2, the latest~~ Re-enable Sentry on Android, with enableNdk: false Oct 5, 2023

chrisbobbe added the P1 high-priority label Oct 5, 2023

gnprice reviewed Oct 5, 2023

View reviewed changes

chrisbobbe and others added 4 commits October 5, 2023 13:48

deps: Upgrade @sentry/react-native to 5.10.0

49b0913

This introduces the `enableNdk` option at Sentry initialization time: getsentry/sentry-react-native@2c394d1 We add that option to our hand-edited type definitions, because we intend to use it.

sentry: Re-enable Sentry again (on Android)

f792c39

This reverts commit e8c57e7, except for its updates to the changelog. Fixes: zulip#5766

gnprice force-pushed the pr-sentry-react-native-5-9-2 branch from b731fc8 to f792c39 Compare October 5, 2023 20:54

gnprice merged commit f792c39 into zulip:main Oct 5, 2023
1 check passed

chrisbobbe deleted the pr-sentry-react-native-5-9-2 branch October 5, 2023 20:55

This was referenced Oct 31, 2023

Re-enable Sentry on Android, with no libsentry.so at all #5782

Closed

Re-enable Sentry on Android (without crashes) #5766

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable Sentry on Android, with `enableNdk: false` #5765

Re-enable Sentry on Android, with `enableNdk: false` #5765

chrisbobbe commented Sep 21, 2023 •

edited

Loading

chrisbobbe commented Sep 21, 2023

chrisbobbe commented Sep 21, 2023 •

edited

Loading

gnprice commented Oct 4, 2023

chrisbobbe commented Oct 4, 2023

chrisbobbe commented Oct 5, 2023

gnprice left a comment

gnprice Oct 5, 2023

gnprice Oct 5, 2023

chrisbobbe commented Oct 5, 2023

Re-enable Sentry on Android, with enableNdk: false #5765

Re-enable Sentry on Android, with enableNdk: false #5765

Conversation

chrisbobbe commented Sep 21, 2023 • edited Loading

chrisbobbe commented Sep 21, 2023

chrisbobbe commented Sep 21, 2023 • edited Loading

gnprice commented Oct 4, 2023

chrisbobbe commented Oct 4, 2023

chrisbobbe commented Oct 5, 2023

gnprice left a comment

Choose a reason for hiding this comment

gnprice Oct 5, 2023

Choose a reason for hiding this comment

gnprice Oct 5, 2023

Choose a reason for hiding this comment

chrisbobbe commented Oct 5, 2023

Re-enable Sentry on Android, with `enableNdk: false` #5765

Re-enable Sentry on Android, with `enableNdk: false` #5765

chrisbobbe commented Sep 21, 2023 •

edited

Loading

chrisbobbe commented Sep 21, 2023 •

edited

Loading