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

Ensure foreground stats are recorded for handled errors #755

Merged
merged 6 commits into from
Jul 17, 2020

Conversation

fractalwrench
Copy link
Contributor

Goal

Ensures that foreground stats are recorded correctly for handled errors. This fixes a bug where app.inForeground, app.durationInForeground, and app.duration were captured for unhandled errors but not for handled errors. This is due to the two errors using a different mechanism for capture.

Changeset

  • Added [BSG_KSCrash captureApplicationStatsInfo] to BSG_KSSystemInfo to ensure that application stats are always captured for handled errors
  • Returned the value of bsg_kscrw_i_writeAppStats as a JSON string when capturing application stats
  • Renamed BSG_ThreadDataBuffer to BSG_JSONDataBuffer as the struct can be used as a buffer for both threads + app stats

Tests

Verified locally in an example app that the fields are now populated.

Copy link
Contributor

@tomlongridge tomlongridge left a comment

Choose a reason for hiding this comment

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

A few suggestions for consistency in naming, in case we use the data for other things in future.

It seems a little convoluted to encode and decode the JSON when all we're doing is reaching inside the crash context. It also calls this for any report, not just handled.

Did you consider using it directly with bsg_kscrashstate_currentState from BugsnagClient? Would it be better to override the AppWithState fields with this once it's been created for new handled errors only?

Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.h Outdated Show resolved Hide resolved
Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.h Outdated Show resolved Hide resolved
@fractalwrench
Copy link
Contributor Author

Updated approach to avoid serialization by getting the information from the KSCrash state directly. This necessitates a small refactor of the logic for updating the duration/durationInForeground fields, which need to be recalculated whenever an error is captured.

@tomlongridge tomlongridge changed the base branch from master to next July 17, 2020 14:23
Copy link
Contributor

@tomlongridge tomlongridge 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 and works as expected now.

Small quibble over one of the method names and an extraneous declaration.

@fractalwrench fractalwrench merged commit 1b45518 into next Jul 17, 2020
@fractalwrench fractalwrench deleted the fix-foreground-stats branch July 17, 2020 15:46
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