For #18836: replace StartupActivityStateProvider with StartupStateProvider.

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.
upstream-sync
Michael Comella 3 years ago committed by Michael Comella
parent 0cbedaadb1
commit 000bef020a

@ -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:

@ -193,7 +193,6 @@ open class FenixApplication : LocaleAwareApplication(), Provider {
// }
components.appStartReasonProvider.registerInAppOnCreate(this)
components.startupActivityStateProvider.registerInAppOnCreate(this)
components.startupActivityLog.registerInAppOnCreate(this)
initVisualCompletenessQueueAndQueueTasks()

@ -275,8 +275,7 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity {
components.performance.coldStartupDurationTelemetry.onHomeActivityOnCreate(
components.performance.visualCompletenessQueue,
components.appStartReasonProvider,
components.startupActivityStateProvider,
components.startupStateProvider,
safeIntent,
rootContainer
)

@ -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
@ -177,7 +176,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) }
}

@ -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

@ -1,77 +0,0 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package org.mozilla.fenix.perf
import android.app.Activity
import android.app.Application
import mozilla.components.support.base.log.logger.Logger
import org.mozilla.fenix.GleanMetrics.Metrics
import org.mozilla.fenix.HomeActivity
import org.mozilla.fenix.android.DefaultActivityLifecycleCallbacks
private val logger = Logger("StartupActivityState")
/**
* Provides meta information about the Activities that occur during the initial parts of start up
* and their state.
*
* [registerInAppOnCreate] must be called for this class to work correctly.
*/
class StartupActivityStateProvider {
enum class FirstForegroundActivity {
TO_BE_DETERMINED,
HOME_ACTIVITY,
UNKNOWN
}
enum class FirstForegroundActivityState {
BEFORE_FOREGROUND,
CURRENTLY_FOREGROUNDED,
AFTER_FOREGROUND,
}
/** The first [Activity] that has been foreground in this process lifetime. */
var firstForegroundActivityOfProcess = FirstForegroundActivity.TO_BE_DETERMINED
private set
/** The current foreground state of the [firstForegroundActivityOfProcess]. */
var firstForegroundActivityState = FirstForegroundActivityState.BEFORE_FOREGROUND
private set
/**
* Registers the handlers needed by this class: this is expected to be called from
* [Application.onCreate].
*/
fun registerInAppOnCreate(application: Application) {
application.registerActivityLifecycleCallbacks(StateActivityLifecycleCallbacks())
}
private inner class StateActivityLifecycleCallbacks : DefaultActivityLifecycleCallbacks {
override fun onActivityStarted(activity: Activity) {
if (firstForegroundActivityOfProcess != FirstForegroundActivity.TO_BE_DETERMINED) {
// This should never happen because we remove the listener in onStop and old activities
// should be stopped before new ones are started but the call order may change slightly
// between devices.
Metrics.activityStateProviderError.set(true)
logger.error("StartupActivityStateProvider...onActivityStarted unexpectedly called twice.")
return
}
firstForegroundActivityOfProcess = when (activity) {
is HomeActivity -> FirstForegroundActivity.HOME_ACTIVITY
else -> FirstForegroundActivity.UNKNOWN
}
firstForegroundActivityState = FirstForegroundActivityState.CURRENTLY_FOREGROUNDED
}
override fun onActivityStopped(activity: Activity) {
firstForegroundActivityState = FirstForegroundActivityState.AFTER_FOREGROUND
activity.application.unregisterActivityLifecycleCallbacks(this) // no more state updates needed.
}
}
}

@ -313,7 +313,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 |

Loading…
Cancel
Save