Skip to content
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.

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

Send minimal error report if cached file is corrupted/empty #500

Merged
merged 6 commits into from Jun 20, 2019

Conversation

fractalwrench
Copy link
Contributor

Goal

If a report cannot be delivered due to delivery failure, it is cached on disk. If this cached report cannot be deserialised, or is only partially written, e.g. due to lack of disk space, then currently no bugsnag report will be sent.

This changeset alters bugsnag-android to send a minimal error report for cached JVM exceptions only, by encoding basic error information in the filename.

Changeset

  • Updated ErrorStore to pass a Delegate that is invoked whenever a cached file cannot be deserialised
  • Implemented Delegate in Client by sending a report with DeliveryStyle.NO_CACHE (this prevents minimal errors from generating other minimal errors if delivery fails and there's a serialisation bug in the notifier)
  • Updated Error to include incomplete field that is set when a report is incomplete
  • In the previous implementation cached files were only deserialised from JSON if a beforeSend callback was set; files are now always deserialised, which negates the need for Report#errorFile
  • Added functions to the ErrorStore which encode and decode basic information about the error from the filename

Tests

  • Added a mazerunner scenario for a cached handled and unhandled JVM exception, where the file is either corrupted or entirely empty
  • Adapted and added to existing unit test coverage for encoding/decoding of basic error info in the report's filename
  • Manually verified that when a cached report is edited on disk and delivered to the dashboard:

Screenshot 2019-06-13 at 14 07 47

Sends a report to bugsnag whenever it's not possible to deliver an error, ensuring that the user
receives at least some information. This new report will not be cached if delivery fails, to prevent
any serialisation bugs from looping and creating multiple reports. To support this change, we now
always attempt serialisation of the cached error. This removes the need for the Report class to have
a file, which has an added benefit of allowing us to specify Report#error as non-null.
Copy link
Contributor

@Cawllec Cawllec left a comment

Choose a reason for hiding this comment

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

Maze tests LGTM 👍

Copy link
Contributor

@bengourley bengourley left a comment

Choose a reason for hiding this comment

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

The implementation of this seems a little different than I imagined from our discussions.

I'm confused about the choice to send an "empty" report, rather than sending a report that communicates exactly what happened. I think it will be really confusing for users, with no clue as to why it was there and why it doesn't have any info.

I was imagining something like this:

image

Attaching any data we have to "Stored Report" (or some other aptly named tab).

Are there any reasons not to pursue this kind of thing?

@fractalwrench
Copy link
Contributor Author

Note to self: we should probably truncate the error class beyond a certain size to prevent the filename from being too large

Copy link
Contributor

@bengourley bengourley left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me, from reading the code. A few comments that I think will require answers rather than changes.

I'd like to understand how to edit the data on disk so I test out the functionality, unless this is going to UAT?

@fractalwrench
Copy link
Contributor Author

Thanks for the review! The best way to edit files on an emulator/device would be to set the device into airplane mode so that the crash is definitely stored, then use the Device File Explorer to edit the file before relaunching the app. The file will be stored under `data/data/com.bugsnag.android.example/cache/bugsnag-errors, and you should be able to edit it like a regular file.

I'll have a read through the other comments and address them inline.

@fractalwrench
Copy link
Contributor Author

I've addressed the remaining comments and also truncated the error class to a max of 40 chars - this is now ready for re-review.

@fractalwrench fractalwrench merged commit 4e13444 into next Jun 20, 2019
@fractalwrench fractalwrench deleted the send-report-delivery-failure branch June 20, 2019 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants