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

[PLAT-5204] Fix recording of foregroundDuration #839

Merged
merged 1 commit into from
Oct 16, 2020

Conversation

nickdowell
Copy link
Contributor

@nickdowell nickdowell commented Oct 12, 2020

Goal

This changes the tracking of foregroundDuration to include both UIApplicationStateActive and UIApplicationStateInactive which are both described as "foreground" states in Apple documentation.

Previously only UIApplicationStateActive was being counted towards this total duration.

Design

This simplifies the logic behind duration tracking and makes our APIs more consistent and in line with Apple's documentation.

Changeset

BSG_KSCrash_State now only observes whether the app is in a foreground or background state. Code related to tracking active vs inactive has been removed.

activeDuration... struct fields have been renamed to foregroundDuration... for clarity

Test cases that only varied the active vs inactive state have been removed.

Testing

Manually verified that the duration and durationInForeground values increment as expected in payloads sent to the back-end.

Unit tests and e2e tests passing.

UIApplicationStateActive and UIApplicationStateInactive now both
count towards the foregroundDuration reported in an error report.
@nickdowell nickdowell marked this pull request as ready for review October 13, 2020 08:37
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.

More of a general question but would it be useful to know whether the app was active and for how long in addition to whether it was in the foreground? I could potentially see it as separate metadata in addition to correcting the definition of "in foreground."

@kstenerud
Copy link
Contributor

kstenerud commented Oct 14, 2020

This is probably going to get confusing since we'll now have the concept of "foreground duration" and "background duration", which leads to the assumption that they're opposite sides of the same state switch when they're not. The temptation will still be there to just add the two durations together in order to get "total app time", which will still be incorrect.

The way it was before, it had duration information for time backgrounded, time active (in fg, not interrupted), but was missing time interrupted and total time. We'll lose information about this kind of ios-specific information with this change, and recovering it in a later version will require changing the meaning of the fields again, and dealing with the headache of legacy versions of bugsnag in the field.

I think we should be very sure we're okay with this before committing to such a change...

@nickdowell
Copy link
Contributor Author

At the public API level and in the crash reports we only expose duration (total) and durationInForeground. Note that there is some ambiguity because we don't define what it means for the app to be "running" - currently we include time spent suspended (when the app stops getting CPU time) in the total.

Prior to this change;

  • duration = time spent in the active and background UIApplicationStates
  • durationInForeground = time spent in the active UIApplicationState

After this change;

  • duration = time spent in the active, inactive and background UIApplicationStates
  • durationInForeground = time spent in the active and inactive UIApplicationStates

@nickdowell nickdowell merged commit 0a0bfa2 into next Oct 16, 2020
@nickdowell nickdowell deleted the feature/fix-foreground-duration branch October 16, 2020 08:48
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.

3 participants