Skip to content

Improve EventBus crash logging by removing top level exception #12102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jun 5, 2020
Merged

Improve EventBus crash logging by removing top level exception #12102

merged 2 commits into from
Jun 5, 2020

Conversation

oguzkocer
Copy link
Contributor

We have a lot of EventBusException org.greenrobot.eventbus.EventBus in handleSubscriberException issues in Sentry most of which is pointing a different error. From the issue list, it's impossible to tell what the issue is about which makes monitoring these errors a very inefficient workflow. It's so bad that most of the time I feel like I have to ignore them unless the numbers are big enough.

With the recent Sentry SDK upgrade we are able to modify the events before they are sent to their servers. In this PR, I am removing this particular exception if it meets a very specific set of conditions - which should help us avoid accidentally modifying the exception list.

To test:

  1. Apply the diff at the bottom.
  2. Tap on the switch site button in the my site screen
  3. Pull to refresh
  4. This will create the following issue in Sentry: https://sentry.io/share/issue/59c9fae8616d473180cfcaff3a43de4b/ - it probably will just add to it since we already have an issue, but you can modify the error to create a different issue, just be sure to resolve it once you're done.
  5. If you'd like to check what this PR changes, comment out the event modification changes from this PR and follow the same testing instructions. In this case this issue will be created: https://sentry.io/share/issue/72f4ba0af36f4c6893e6761fba806ea8/ - Notice the top level information after the change is much more descriptive and useful.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.
diff --git a/WordPress/src/main/java/org/wordpress/android/ui/main/SitePickerActivity.java b/WordPress/src/main/java/org/wordpress/android/ui/main/SitePickerActivity.java
index 0e3e093154..c875f08cdf 100644
--- a/WordPress/src/main/java/org/wordpress/android/ui/main/SitePickerActivity.java
+++ b/WordPress/src/main/java/org/wordpress/android/ui/main/SitePickerActivity.java
@@ -304,10 +304,11 @@ public class SitePickerActivity extends LocaleAwareActivity
     @SuppressWarnings("unused")
     @Subscribe(threadMode = ThreadMode.MAIN)
     public void onSiteChanged(OnSiteChanged event) {
-        if (mSwipeToRefreshHelper.isRefreshing()) {
-            mSwipeToRefreshHelper.setRefreshing(false);
-        }
-        debounceLoadSites();
+        throw new IllegalStateException("Testing event bus crash logging changes");
+//        if (mSwipeToRefreshHelper.isRefreshing()) {
+//            mSwipeToRefreshHelper.setRefreshing(false);
+//        }
+//        debounceLoadSites();
     }
 
     private void debounceLoadSites() {
diff --git a/WordPress/src/main/java/org/wordpress/android/util/CrashLoggingUtils.kt b/WordPress/src/main/java/org/wordpress/android/util/CrashLoggingUtils.kt
index aa1db8394c..602dc292c8 100644
--- a/WordPress/src/main/java/org/wordpress/android/util/CrashLoggingUtils.kt
+++ b/WordPress/src/main/java/org/wordpress/android/util/CrashLoggingUtils.kt
@@ -14,6 +14,7 @@ private const val EVENT_BUS_INVOKING_SUBSCRIBER_FAILED_ERROR = "Invoking subscri
 class CrashLoggingUtils {
     companion object {
         @JvmStatic fun shouldEnableCrashLogging(context: Context): Boolean {
+            return true
             if (PackageUtils.isDebugBuild()) {
                 return false
             }

@oguzkocer oguzkocer added this to the 15.1 milestone Jun 4, 2020
@oguzkocer oguzkocer requested review from jkmassel and loremattei June 4, 2020 20:15
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 4, 2020 •
edited
Loading

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 4, 2020 •
edited
Loading

You can test the changes on this Pull Request by downloading the APK here.

@oguzkocer oguzkocer mentioned this pull request Jun 4, 2020
Closed
loremattei
loremattei approved these changes Jun 5, 2020
View reviewed changes
Copy link
Contributor

@loremattei loremattei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very interesting!
The changes look good to me and if it works well in production we may use something like this for other cases as well.

@oguzkocer oguzkocer merged commit 930f845 into develop Jun 5, 2020
@oguzkocer oguzkocer deleted the improve-event-bus-crash-logging branch June 5, 2020 14:39
@oguzkocer oguzkocer mentioned this pull request Jul 9, 2020
Closed
@oguzkocer oguzkocer mentioned this pull request Aug 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkmassel jkmassel Awaiting requested review from jkmassel

1 more reviewer

@loremattei loremattei loremattei approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tooling

Projects

None yet

Milestone

15.1

Development

Successfully merging this pull request may close these issues.

2 participants

@oguzkocer @loremattei