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

Update synchronous crash reporting logic #1188

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Mar 11, 2021

Goal

Updates the existing synchronous reporting of launch crashes so that they fit the behaviour of the new spec. Specifically, synchronous delivery should only be attempted for the most recent startup crash.

Changeset

  • "_startupcrash" is appended to the filename based on whether app.isLaunching is set to true, rather than the previous app.duration exceeding a threshold
  • Refactored flushOnLaunch() so that it creates a Future and blocks for 2 seconds calling get(). This avoids a busy wait that the previous implementation used
  • Altered EventStore so that it only looks for the most recent startupcrash, as determined by the timestamp and "startupcrash suffix encoded in the filename
  • Fixed a bug where .json was encoded in the filename twice (this does not affect any functionality)
  • flushOnLaunch() no longer calls flushAsync() to improve testability

Testing

  • Updated existing test coverage where necessary
  • Added new unit tests for logic around finding a launch crash
  • Created tests that validate what thread event delivery occurs on, and confirms that flushOnLaunch() blocks the main thread for a short duration

Base automatically changed from remove-async to integration/road-1175-identify-crashes-on-launch March 11, 2021 21:53
@fractalwrench fractalwrench force-pushed the PLAT-5975/sync-crash-reporting branch 2 times, most recently from 0123264 to 468af60 Compare March 12, 2021 10:16
@fractalwrench fractalwrench marked this pull request as ready for review March 12, 2021 10:27
@fractalwrench fractalwrench force-pushed the PLAT-5975/sync-crash-reporting branch from 468af60 to 692ee14 Compare March 12, 2021 13:46
@fractalwrench fractalwrench marked this pull request as draft March 12, 2021 15:27
@fractalwrench fractalwrench force-pushed the PLAT-5975/sync-crash-reporting branch from 692ee14 to 86410b8 Compare March 12, 2021 17:27
@fractalwrench fractalwrench marked this pull request as ready for review March 12, 2021 17:28
@fractalwrench fractalwrench force-pushed the PLAT-5975/sync-crash-reporting branch from 86410b8 to c20f08c Compare March 12, 2021 18:16
@twometresteve twometresteve requested a review from kattrali March 16, 2021 10:31
Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

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

Looks good, added a few questions/small improvements.

@fractalwrench fractalwrench force-pushed the PLAT-5975/sync-crash-reporting branch from c20f08c to 03ed228 Compare March 16, 2021 11:55
@fractalwrench fractalwrench requested a review from kattrali March 16, 2021 11:55
@fractalwrench fractalwrench merged commit 2c182d0 into integration/road-1175-identify-crashes-on-launch Mar 16, 2021
@fractalwrench fractalwrench deleted the PLAT-5975/sync-crash-reporting branch March 16, 2021 13:11
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.

2 participants