Event delivery with an event with setUnhandled(true) should behave as an unhandled event delivery

[joshskeen]

Goal

In the case where client.notify is called and the event is modified to be treated as unhandled:

client.notify(throwable, new OnErrorCallback() {
      @Override public boolean onError(@NonNull Event event) {
        event.setUnhandled(true);
       ...
      }
    });

we should cache the event, similar to how it is done for an unhandled exception. With an unhandled exception, the app's availability will cease - and if we are marking an event as unhandled, we are indicating this will be the case.

We use our own default exception handler which allows us to customize crash handling and send crashes to distinct destinations. Using the bugsnag client with client.notify with setUnhandled = true as part of that handler and being able to rely upon similar behavior as the bugsnag default exception handler rather than forward to the bugsnag exception handler would simplify the structure of our code, and remove the hacky bugsnag workarounds we've had to implement.

Design

Change delivery logic to view events marked as unhandled to behave as a typical unhandled exception in bugsnag.

Changeset

Testing

[pyricau]

(note: by "we" here @mutexkid is referring to Square)

[yousif-bugsnag]

Hey @mutexkid, thanks for the PR. Have you considered making use of the delivery config option? That should allow you to easily override this behaviour in your own Delivery class: https://docs.bugsnag.com/platforms/android/configuration-options/#delivery

[joshskeen]

Thanks for pointing this possibility out. With a custom Delivery option, it seems like we'd need 2 clients: 1 using the DefaultDelivery class instance (which we would typically want to use), and another initialized with a custom Delivery option for this case. I don't see a way of extending the DefaultDelivery class to handle only this case. Does that match what you would recommend?

If that is the case, while having 2 clients might be an option , it still seems like it would be more ideal if we were simply treating an event marked as unhandled as how unhandled events are typically treated by DeliveryDelegate, to prevent adding an additional client or a custom Delivery. Thoughts?

[pyricau]

@yousif-bugsnag this points to a core design flaw in the Bugsnag Android SDK: it should be fairly straightforward to adopt it without the default automatic crash reporting, and still report crashes at crash times with a pipeline identical to the default. That's not possible today. As @mutexkid pointed out, Delivery is set at the client level. We're not looking to change the default delivery, we just want to make sure unhandled crashes are uploaded the way they are with the automatic bugsnag upload.

The best option here would be to actually force the bugsnag uncaught exception handler to only use public APIs of a Bugsnag client instead of relying on internal details like it's doing today. Then all the needed APIs would actually surface.

[luke-belton]

Hi @mutexkid, @pyricau - thanks for the additional feedback. We're discussing this internally to see what improvements we could make here to help with your use case and will get back to you with an update as soon as possible 👍

[luke-belton]

Hi - just a quick update on this. We've added an item to our roadmap to look at some options here, including whether we could allow you to 'opt-in' via your Bugsnag config to persisting events you mark as unhandled. I don't have an ETA for this work, but will make sure we keep you updated on this thread 👍