1520754 - Correctly pair events in GleanLifecycleObserver

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Alessio Placitelli [:Dexter]]

From Sebastian's comment:

@Dexterp37 You are not using matching/synchronized/paired events here: Either you want to use resume/pause or start/stop here. With the difference being:

* start/stop: App is visible/invisible

* resume/pause: App is in the foreground, the user can interact with it.

An example for an app can going through those methods/state

* User starts the app
  
  * onCreate()
  * onStart()
  * onResume()

* User pulls down the notification shade (-> The app is still visible, but the user is interacting with the notification drawn on top)
  
  * onPause()

* User closes the notifaction shade and is back in the app
  
  * onResume()

* User leaves the app to go to the home screen:
  
  * onPause()
  * onStop()

* User goes back to the app:
  
  * onStart()

* onResume()

* User presses back to close the app:
  
  * onPause()
  * onStop()
  * onDestroy()

See:

* https://developer.android.com/guide/components/activities/activity-lifecycle#onstart

* https://developer.android.com/guide/components/activities/state-changes

Also see: https://developer.android.com/reference/android/app/Activity#ActivityLifecycle

It appears that we're doing it wrong :( We should probably change this line to use ON_START, as far as I understand.

Otherwise, we might overcount metrics updated when entering foreground again.

Comment 1

[Alessio Placitelli [:Dexter]]

Mike, what's your take on this?

Comment 2

[Michael Droettboom [:mdroettboom]]

I think this is just from a failure to find the right docs. Obviously, these events should be properly paired. But IIUC, we should use pause/resume, not stop/start if we really want events to be for foreground/background. I also feel it's safest to use the "innermost" pair of events.

I found this image helpful.

Comment 3

[Michael Droettboom [:mdroettboom]]

Actually, maybe I'm reading this wrong and we do what stop/start...

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Michael Droettboom [:mdroettboom] from comment #3)

Actually, maybe I'm reading this wrong and we do what stop/start...

From comment 0, it looks to me this should be start/stop, unless we want to send a ping each time the notification shade is interacted with. Moreover, we should really add this to the ping docs: this is a stricter definition of what foreground/background means and it will be very useful for people dealing with the data.

Comment 5

[Travis Long [:travis_]]

I agree that Start/Stop are the correct lifecycle events to work with here. Especially since a popover or interacting with the notification shade could incorrectly affect recording of metrics.

Comment 6

[Travis Long [:travis_]]

Changed to ON_START and updated comment to reflect the issue with using ON_PAUSE/ON_RESUME. Resolved as fixed :)