For mozilla-mobile#18836: replace StartupActivityStateProvider with S…

…tartupStateProvider. The StartupActivityStateProvider uses an imperative implementation, driven by callbacks, to set the state of the application. This is hard to follow as you need to understand which callbacks will be called in which order. For example, to make sense of an implementation like this, COLD, WARM, AND HOT would likely need to be implemented in separate ActivityLifecycleCallbacks. I feel the StartupStateProvider is an improvement because it leverages the StartupActivityLog to query a linear state for a more understandable implementation. Furthermore, it seems accessible to write COLD, WARM, and HOT in the same class because they can all be approached the same way.
mcomella · Apr 8, 2021 · 9b71ff2 · 9b71ff2
1 parent a8b1906
commit 9b71ff2
Show file tree

Hide file tree

Showing 7 changed files with 13 additions and 153 deletions.
diff --git a/app/metrics.yaml b/app/metrics.yaml
@@ -1227,23 +1227,6 @@ metrics:
       - perf-android-fe@mozilla.com
       - mcomella@mozilla.com
     expires: "2021-08-11"
-  activity_state_provider_error:
-    type: boolean
-    description: |
-      The `StartupActivityStateProvider...onActivityStarted` was unexpectedly
-      called twice. We can use this metric to validate our assumptions about
-      how these APIs are called. This probe can be removed once we validate
-      these assumptions.
-    bugs:
-      - https://github.com/mozilla-mobile/fenix/issues/18426
-    data_reviews:
-      - https://github.com/mozilla-mobile/fenix/pull/18632#issue-600193452
-    data_sensitivity:
-      - technical
-    notification_emails:
-      - perf-android-fe@mozilla.com
-      - mcomella@mozilla.com
-    expires: "2021-08-11"
 
 preferences:
   show_search_suggestions:

diff --git a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt
@@ -191,7 +191,6 @@ open class FenixApplication : LocaleAwareApplication(), Provider {
             // }
 
             components.appStartReasonProvider.registerInAppOnCreate(this)
-            components.startupActivityStateProvider.registerInAppOnCreate(this)
             components.startupActivityLog.registerInAppOnCreate(this)
             initVisualCompletenessQueueAndQueueTasks()
 

diff --git a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt
@@ -275,8 +275,7 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity {
 
         components.performance.coldStartupDurationTelemetry.onHomeActivityOnCreate(
             components.performance.visualCompletenessQueue,
-            components.appStartReasonProvider,
-            components.startupActivityStateProvider,
+            components.startupStateProvider,
             safeIntent,
             rootContainer
         )

diff --git a/app/src/main/java/org/mozilla/fenix/components/Components.kt b/app/src/main/java/org/mozilla/fenix/components/Components.kt
@@ -30,7 +30,6 @@ import org.mozilla.fenix.ext.components
 import org.mozilla.fenix.ext.settings
 import org.mozilla.fenix.perf.AppStartReasonProvider
 import org.mozilla.fenix.perf.StartupActivityLog
-import org.mozilla.fenix.perf.StartupActivityStateProvider
 import org.mozilla.fenix.perf.StartupStateProvider
 import org.mozilla.fenix.perf.lazyMonitored
 import org.mozilla.fenix.utils.ClipboardHandler
@@ -176,7 +175,6 @@ class Components(private val context: Context) {
     }
 
     val appStartReasonProvider by lazyMonitored { AppStartReasonProvider() }
-    val startupActivityStateProvider by lazyMonitored { StartupActivityStateProvider() }
     val startupActivityLog by lazyMonitored { StartupActivityLog() }
     val startupStateProvider by lazyMonitored { StartupStateProvider(startupActivityLog, appStartReasonProvider) }
 }
diff --git a/app/src/main/java/org/mozilla/fenix/perf/ColdStartupDurationTelemetry.kt b/app/src/main/java/org/mozilla/fenix/perf/ColdStartupDurationTelemetry.kt
@@ -11,9 +11,7 @@ import androidx.core.view.doOnPreDraw
 import mozilla.components.support.base.log.logger.Logger
 import mozilla.components.support.utils.SafeIntent
 import org.mozilla.fenix.GleanMetrics.PerfStartup
-import org.mozilla.fenix.perf.StartupActivityStateProvider.FirstForegroundActivity
-import org.mozilla.fenix.perf.StartupActivityStateProvider.FirstForegroundActivityState
-import org.mozilla.fenix.perf.AppStartReasonProvider.StartReason
+import org.mozilla.fenix.HomeActivity
 import java.util.concurrent.TimeUnit
 
 private val logger = Logger("ColdStartupDuration")
@@ -23,74 +21,35 @@ private val logger = Logger("ColdStartupDuration")
  * [org.mozilla.fenix.components.metrics.AppStartupTelemetry] class by being simple-to-implement and
  * simple-to-analyze (i.e. works in GLAM) rather than being a "perfect" and comprehensive measurement.
  *
- * This class relies on external state providers like [AppStartReasonProvider] and
- * [StartupActivityStateProvider] that are tricky to implement correctly so take the results with a
- * grain of salt.
+ * This class relies on external state providers like [StartupStateProvider] that are tricky to
+ * implement correctly so take the results with a grain of salt.
  */
 class ColdStartupDurationTelemetry {
 
     fun onHomeActivityOnCreate(
         visualCompletenessQueue: VisualCompletenessQueue,
-        startReasonProvider: AppStartReasonProvider,
-        startupActivityStateProvider: StartupActivityStateProvider,
+        startupStateProvider: StartupStateProvider,
         safeIntent: SafeIntent,
         rootContainer: View
     ) {
-        // Optimization: it's expensive to post runnables so we can short-circuit with a subset of the later logic.
-        if (startupActivityStateProvider.firstForegroundActivityState ==
-                FirstForegroundActivityState.AFTER_FOREGROUND) {
-            logger.debug("Not measuring: first foreground activity already backgrounded")
+        // Optimization: it might be expensive to post runnables so we can short-circuit
+        // with a subset of the later logic.
+        if (startupStateProvider.shouldShortCircuitColdStart()) {
+            logger.debug("Not measuring: is not cold start (short-circuit)")
             return
         }
 
         rootContainer.doOnPreDraw {
-            // Optimization: we're running code before the first frame so we want to avoid doing anything
-            // expensive as part of the drawing loop. Recording telemetry took 3-7ms on the Moto G5 (a
-            // frame is ~16ms) so we defer the expensive work for later by posting a Runnable.
-            //
-            // We copy the values because their values may change when passed into the handler. It's
-            // cheaper to copy the values than copy the objects (= allocation + copy values) so we just
-            // copy the values even though this copy could happen incorrectly if these values become objects later.
-            val startReason = startReasonProvider.reason
-            val firstActivity = startupActivityStateProvider.firstForegroundActivityOfProcess
-            val firstActivityState = startupActivityStateProvider.firstForegroundActivityState
+            // This block takes 0ms on a Moto G5: it doesn't seem long enough to optimize.
             val firstFrameNanos = SystemClock.elapsedRealtimeNanos()
-
-            // On the visual completeness queue, this will report later than posting to the main thread (not
-            // ideal for pulling out of automated performance tests) but should delay visual completeness less.
-            visualCompletenessQueue.queue.runIfReadyOrQueue {
-                if (!isColdStartToThisHomeActivityInstance(startReason, firstActivity, firstActivityState)) {
-                    logger.debug("Not measuring: this activity isn't both the first foregrounded & HomeActivity")
-                    return@runIfReadyOrQueue
+            if (startupStateProvider.isColdStartForStartedActivity(HomeActivity::class.java)) {
+                visualCompletenessQueue.queue.runIfReadyOrQueue {
+                    recordColdStartupTelemetry(safeIntent, firstFrameNanos)
                 }
-
-                recordColdStartupTelemetry(safeIntent, firstFrameNanos)
             }
         }
     }
 
-    private fun isColdStartToThisHomeActivityInstance(
-        startReason: StartReason,
-        firstForegroundActivity: FirstForegroundActivity,
-        firstForegroundActivityState: FirstForegroundActivityState
-    ): Boolean {
-        // This logic is fragile: if an Activity that isn't currently foregrounded is refactored to get
-        // temporarily foregrounded (e.g. IntentReceiverActivity) or an interstitial Activity is added
-        // that is temporarily foregrounded, we'll no longer detect HomeActivity as the first foregrounded
-        // activity and we'll never record telemetry.
-        //
-        // Because of this, we may not record values in Beta and Release if MigrationDecisionActivity
-        // gets foregrounded (I never tested these channels: I think Nightly data is probably good enough for now).
-        //
-        // What we'd ideally determine is, "Is the final activity during this start up HomeActivity?"
-        // However, it's challenging to do so in a robust way so we stick with this simpler solution
-        // ("Is the first foregrounded activity during this start up HomeActivity?") despite its flaws.
-        val wasProcessStartedBecauseOfAnActivity = startReason == StartReason.ACTIVITY
-        val isThisTheFirstForegroundActivity = firstForegroundActivity == FirstForegroundActivity.HOME_ACTIVITY &&
-            firstForegroundActivityState == FirstForegroundActivityState.CURRENTLY_FOREGROUNDED
-        return wasProcessStartedBecauseOfAnActivity && isThisTheFirstForegroundActivity
-    }
-
     private fun recordColdStartupTelemetry(safeIntent: SafeIntent, firstFrameNanos: Long) {
         // This code duplicates the logic for determining how we should handle this intent which
         // could result in inconsistent results: e.g. the browser might get a VIEW intent but it's

diff --git a/app/src/main/java/org/mozilla/fenix/perf/StartupActivityStateProvider.kt b/app/src/main/java/org/mozilla/fenix/perf/StartupActivityStateProvider.kt
diff --git a/docs/metrics.md b/docs/metrics.md
@@ -312,7 +312,6 @@ In addition to those built-in metrics, the following metrics are added to the pi
 | engine_tab.kills |[labeled_counter](https://mozilla.github.io/glean/book/user/metrics/labeled_counters.html) |How often was the content process of a foreground (selected) or background tab killed.  |[mozilla-mobile/fenix#17864](https://github.com/mozilla-mobile/fenix/pull/17864)|<ul><li>foreground</li><li>background</li></ul>|2021-12-31 |1 |
 | events.normal_and_private_uri_count |[counter](https://mozilla.github.io/glean/book/user/metrics/counter.html) |A counter of URIs visited by the user in the current session, including page reloads. This includes private browsing.  This does not include background page requests and URIs from embedded pages but may be incremented without user interaction by website scripts that programmatically redirect to a new location.  |[mozilla-mobile/fenix#17935](https://github.com/mozilla-mobile/fenix/pull/17935)||2022-08-01 |2 |
 | events.total_uri_count |[counter](https://mozilla.github.io/glean/book/user/metrics/counter.html) |A counter of URIs visited by the user in the current session, including page reloads. This does not include background page requests and URIs from embedded pages or private browsing but may be incremented without user interaction by website scripts that programmatically redirect to a new location.  |[mozilla-mobile/fenix#1785](https://github.com/mozilla-mobile/fenix/pull/1785), [mozilla-mobile/fenix#8314](https://github.com/mozilla-mobile/fenix/pull/8314), [mozilla-mobile/fenix#15713](https://github.com/mozilla-mobile/fenix/pull/15713#issuecomment-703972068)||2021-08-01 |2 |
-| metrics.activity_state_provider_error |[boolean](https://mozilla.github.io/glean/book/user/metrics/boolean.html) |The `StartupActivityStateProvider...onActivityStarted` was unexpectedly called twice. We can use this metric to validate our assumptions about how these APIs are called. This probe can be removed once we validate these assumptions.  |[mozilla-mobile/fenix#18632](https://github.com/mozilla-mobile/fenix/pull/18632#issue-600193452)||2021-08-11 |1 |
 | metrics.adjust_ad_group |[string](https://mozilla.github.io/glean/book/user/metrics/string.html) |A string containing the Adjust ad group ID from which the user installed Fenix. This will not send on the first session the user runs. If the install is organic, this will be empty.  |[mozilla-mobile/fenix#9253](https://github.com/mozilla-mobile/fenix/pull/9253), [mozilla-mobile/fenix#15713](https://github.com/mozilla-mobile/fenix/pull/15713#issuecomment-703972068)||2021-08-01 |2 |
 | metrics.adjust_campaign |[string](https://mozilla.github.io/glean/book/user/metrics/string.html) |A string containing the Adjust campaign ID from which the user installed Fenix. This will not send on the first session the user runs. If the install is organic, this will be empty.  |[mozilla-mobile/fenix#5579](https://github.com/mozilla-mobile/fenix/pull/5579), [mozilla-mobile/fenix#15713](https://github.com/mozilla-mobile/fenix/pull/15713#issuecomment-703972068)||2021-08-01 |1 |
 | metrics.adjust_creative |[string](https://mozilla.github.io/glean/book/user/metrics/string.html) |A string containing the Adjust creative ID from which the user installed Fenix. This will not send on the first session the user runs. If the install is organic, this will be empty.  |[mozilla-mobile/fenix#9253](https://github.com/mozilla-mobile/fenix/pull/9253), [mozilla-mobile/fenix#15713](https://github.com/mozilla-mobile/fenix/pull/15713#issuecomment-703972068)||2021-08-01 |2 |