From 5179fdefa03c5d9af6f4da9c818aaeb84b38a500 Mon Sep 17 00:00:00 2001 From: Grisha Kruglov Date: Thu, 23 Apr 2020 00:10:02 -0700 Subject: [PATCH] [components] Closes https://github.com/mozilla-mobile/android-components/issues/4992: Emit provider duration facts from BrowserAwesomeBar Some things to consider and trade-offs: When recording durations of various providers into a glean metric, we have a bit of a hurdle. One approach would be to encapsulate all of the awesomebar-related perf telemetry entirely within the awesomebar a-c component. But, set of providers isn't entirely known to us at the a-c level. We know what providers we have defined, but we don't know what providers applications will provide themselves. Also, glean doesn't have a metric of type (string->timespan), which would allow us to work-around this. So, we can't define ping/metrics in the a-c component. This means that they need to be defined elsewhere, while the measurement happens within the component. An established pattern for that in the codebase is emitting "facts", which is what this patch does. We delegate to the consuming application to then actually do something with these facts - e.g. map providers to concrete glean metric definitions. Another consideration is if we should try to group timings related to a single query. That's hard to do reliably, and will introduce additional complexity into what's otherwise a super simple setup. Source of that complexity is that we actively try to cancel query jobs as user is typing; our providers could take an arbitrary time to resolve, and so grouping becomes difficult. However, we care about improving how long each individual provider takes, since that's a good proxy for "how responsive is this UI?". Simply recording each individual timing we see should be enough for that. --- .../components/browser/awesomebar/build.gradle | 1 + .../browser/awesomebar/BrowserAwesomeBar.kt | 22 ++++++++- .../awesomebar/facts/BrowserAwesomeBarFacts.kt | 55 ++++++++++++++++++++++ .../browser/awesomebar/BrowserAwesomeBarTest.kt | 47 ++++++++++++++++-- .../provider/HistoryStorageSuggestionProvider.kt | 3 +- 5 files changed, 120 insertions(+), 8 deletions(-) create mode 100644 mobile/android/android-components/components/browser/awesomebar/src/main/java/mozilla/components/browser/awesomebar/facts/BrowserAwesomeBarFacts.kt diff --git a/mobile/android/android-components/components/browser/awesomebar/build.gradle b/mobile/android/android-components/components/browser/awesomebar/build.gradle index ddff3762d0a8..0f7a76c588c7 100644 --- a/mobile/android/android-components/components/browser/awesomebar/build.gradle +++ b/mobile/android/android-components/components/browser/awesomebar/build.gradle @@ -30,6 +30,7 @@ dependencies { api project(':concept-awesomebar') implementation project(':support-ktx') + implementation project(':support-base') api Dependencies.androidx_recyclerview implementation Dependencies.androidx_constraintlayout diff --git a/mobile/android/android-components/components/browser/awesomebar/src/main/java/mozilla/components/browser/awesomebar/BrowserAwesomeBar.kt b/mobile/android/android-components/components/browser/awesomebar/src/main/java/mozilla/components/browser/awesomebar/BrowserAwesomeBar.kt index c50ac2548a7c..ef9fdc2b14c1 100644 --- a/mobile/android/android-components/components/browser/awesomebar/src/main/java/mozilla/components/browser/awesomebar/BrowserAwesomeBar.kt +++ b/mobile/android/android-components/components/browser/awesomebar/src/main/java/mozilla/components/browser/awesomebar/BrowserAwesomeBar.kt @@ -5,6 +5,7 @@ package mozilla.components.browser.awesomebar import android.content.Context +import android.os.SystemClock import android.util.AttributeSet import android.util.LruCache import androidx.annotation.VisibleForTesting @@ -17,6 +18,7 @@ import kotlinx.coroutines.Job import kotlinx.coroutines.asCoroutineDispatcher import kotlinx.coroutines.launch import kotlinx.coroutines.withContext +import mozilla.components.browser.awesomebar.facts.emitProviderQueryTimingFact import mozilla.components.browser.awesomebar.layout.SuggestionLayout import mozilla.components.browser.awesomebar.transform.SuggestionTransformer import mozilla.components.concept.awesomebar.AwesomeBar @@ -30,7 +32,7 @@ internal const val INITIAL_NUMBER_OF_PROVIDERS = 5 /** * A customizable [AwesomeBar] implementation. */ -@Suppress("TooManyFunctions") +@Suppress("TooManyFunctions", "LargeClass") class BrowserAwesomeBar @JvmOverloads constructor( context: Context, attrs: AttributeSet? = null, @@ -154,7 +156,25 @@ class BrowserAwesomeBar @JvmOverloads constructor( providers.forEach { provider -> launch { + // At this point, we have a timing value for a provider. + // We have a choice here - we can try grouping different timings together for a + // single user input value, or we can just treat them as entirely independent values. + // These timings are correlated with each other in a sense that they act on the same + // inputs, and are executed at the same time. However, our goal here is to track performance + // of the providers. Each provider acts independently from another; recording their performance + // at an individual level will allow us to track that performance over time. + // Tracked value will be reflected both in perceived user experience (how quickly results from + // a provider show up), and in a purely technical interpretation of how quickly providers + // fulfill requests. + // Grouping also poses timing challenges - as user is typing, we're trying to cancel these + // provider requests. Given that each request can take an arbitrary amount of time to execute, + // grouping correctly becomes tricky and we run a risk of omitting certain values - or, of + // adding a bunch of complexity just for the sake of "correct grouping". + val start = SystemClock.elapsedRealtimeNanos() val suggestions = withContext(jobDispatcher) { provider.block() } + val end = SystemClock.elapsedRealtimeNanos() + emitProviderQueryTimingFact(provider, timing = end - start) + val processedSuggestions = processProviderSuggestions(suggestions) suggestionsAdapter.addSuggestions( provider, diff --git a/mobile/android/android-components/components/browser/awesomebar/src/main/java/mozilla/components/browser/awesomebar/facts/BrowserAwesomeBarFacts.kt b/mobile/android/android-components/components/browser/awesomebar/src/main/java/mozilla/components/browser/awesomebar/facts/BrowserAwesomeBarFacts.kt new file mode 100644 index 000000000000..7c43555c8c6e --- /dev/null +++ b/mobile/android/android-components/components/browser/awesomebar/src/main/java/mozilla/components/browser/awesomebar/facts/BrowserAwesomeBarFacts.kt @@ -0,0 +1,55 @@ +/* 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 mozilla.components.browser.awesomebar.facts + +import mozilla.components.concept.awesomebar.AwesomeBar +import mozilla.components.support.base.Component +import mozilla.components.support.base.facts.Action +import mozilla.components.support.base.facts.Fact +import mozilla.components.support.base.facts.collect + +/** + * Facts emitted for telemetry related to the AwesomeBar feature. + */ +class BrowserAwesomeBarFacts { + /** + * Specific types of telemetry items. + */ + object Items { + const val PROVIDER_DURATION = "provider_duration" + } + + /** + * Keys used to record metadata about [Items]. + */ + object MetadataKeys { + const val DURATION_PAIR = "duration_pair" + } +} + +private fun emitAwesomebarFact( + action: Action, + item: String, + value: String? = null, + metadata: Map? = null +) { + Fact( + Component.BROWSER_AWESOMEBAR, + action, + item, + value, + metadata + ).collect() +} + +internal fun emitProviderQueryTimingFact(provider: AwesomeBar.SuggestionProvider, timing: Long) { + emitAwesomebarFact( + Action.INTERACTION, + BrowserAwesomeBarFacts.Items.PROVIDER_DURATION, + metadata = mapOf( + BrowserAwesomeBarFacts.MetadataKeys.DURATION_PAIR to (provider to timing) + ) + ) +} diff --git a/mobile/android/android-components/components/browser/awesomebar/src/test/java/mozilla/components/browser/awesomebar/BrowserAwesomeBarTest.kt b/mobile/android/android-components/components/browser/awesomebar/src/test/java/mozilla/components/browser/awesomebar/BrowserAwesomeBarTest.kt index 8a9efc302ecf..f55860a5a4a0 100644 --- a/mobile/android/android-components/components/browser/awesomebar/src/test/java/mozilla/components/browser/awesomebar/BrowserAwesomeBarTest.kt +++ b/mobile/android/android-components/components/browser/awesomebar/src/test/java/mozilla/components/browser/awesomebar/BrowserAwesomeBarTest.kt @@ -11,8 +11,14 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.asCoroutineDispatcher import kotlinx.coroutines.delay import kotlinx.coroutines.runBlocking +import mozilla.components.browser.awesomebar.facts.BrowserAwesomeBarFacts import mozilla.components.browser.awesomebar.transform.SuggestionTransformer import mozilla.components.concept.awesomebar.AwesomeBar +import mozilla.components.support.base.Component +import mozilla.components.support.base.facts.Action +import mozilla.components.support.base.facts.Fact +import mozilla.components.support.base.facts.FactProcessor +import mozilla.components.support.base.facts.Facts import mozilla.components.support.test.mock import mozilla.components.support.test.robolectric.testContext import mozilla.components.support.test.rule.MainCoroutineRule @@ -28,6 +34,7 @@ import org.mockito.Mockito.inOrder import org.mockito.Mockito.never import org.mockito.Mockito.spy import org.mockito.Mockito.verify +import org.mockito.Mockito.verifyZeroInteractions import org.robolectric.Shadows.shadowOf import java.util.UUID import java.util.concurrent.Executor @@ -43,22 +50,52 @@ class BrowserAwesomeBarTest { fun `BrowserAwesomeBar forwards input to providers`() { runBlocking { val provider1 = mockProvider() + val awesomeBar = BrowserAwesomeBar(testContext) + val provider2 = mockProvider() val provider3 = mockProvider() + awesomeBar.addProviders(provider1) - val awesomeBar = BrowserAwesomeBar(testContext) - awesomeBar.addProviders(provider1, provider2) - awesomeBar.addProviders(provider3) + val facts = mutableListOf() + Facts.registerProcessor(object : FactProcessor { + override fun process(fact: Fact) { + facts.add(fact) + } + }) awesomeBar.onInputChanged("Hello World!") awesomeBar.awaitForAllJobsToFinish() verify(provider1).onInputChanged("Hello World!") - verify(provider2).onInputChanged("Hello World!") - verify(provider3).onInputChanged("Hello World!") + verifyZeroInteractions(provider2) + verifyZeroInteractions(provider3) + + assertEquals(1, facts.size) + assertBrowserAwesomebarFact(facts[0], provider1) + + awesomeBar.addProviders(provider2, provider3) + + awesomeBar.onInputChanged("Hello Back!") + awesomeBar.awaitForAllJobsToFinish() + + verify(provider2).onInputChanged("Hello Back!") + verify(provider3).onInputChanged("Hello Back!") + + assertEquals(4, facts.size) + + facts.forEach { assertBrowserAwesomebarFact(it) } } } + private fun assertBrowserAwesomebarFact(f: Fact, provider: AwesomeBar.SuggestionProvider? = null) { + assertEquals(Component.BROWSER_AWESOMEBAR, f.component) + assertEquals(Action.INTERACTION, f.action) + assertEquals(BrowserAwesomeBarFacts.Items.PROVIDER_DURATION, f.item) + val pair = f.metadata!![BrowserAwesomeBarFacts.MetadataKeys.DURATION_PAIR] as Pair<*, *> + provider?.let { assertEquals(pair.first, provider) } + assertTrue(pair.second is Long) + } + @Test fun `BrowserAwesomeBar forwards onInputStarted to providers`() { val provider1: AwesomeBar.SuggestionProvider = mock() diff --git a/mobile/android/android-components/components/feature/awesomebar/src/main/java/mozilla/components/feature/awesomebar/provider/HistoryStorageSuggestionProvider.kt b/mobile/android/android-components/components/feature/awesomebar/src/main/java/mozilla/components/feature/awesomebar/provider/HistoryStorageSuggestionProvider.kt index 597d26b40d9e..012125bccd3f 100644 --- a/mobile/android/android-components/components/feature/awesomebar/src/main/java/mozilla/components/feature/awesomebar/provider/HistoryStorageSuggestionProvider.kt +++ b/mobile/android/android-components/components/feature/awesomebar/src/main/java/mozilla/components/feature/awesomebar/provider/HistoryStorageSuggestionProvider.kt @@ -8,7 +8,6 @@ import mozilla.components.browser.icons.BrowserIcons import mozilla.components.browser.icons.IconRequest import mozilla.components.concept.awesomebar.AwesomeBar import mozilla.components.concept.engine.Engine -import mozilla.components.concept.storage.BookmarksStorage import mozilla.components.concept.storage.HistoryStorage import mozilla.components.concept.storage.SearchResult import mozilla.components.feature.session.SessionUseCases @@ -20,7 +19,7 @@ internal const val HISTORY_SUGGESTION_LIMIT = 20 * A [AwesomeBar.SuggestionProvider] implementation that provides suggestions based on the browsing * history stored in the [HistoryStorage]. * - * @property historyStorage and instance of the [BookmarksStorage] used + * @property historyStorage and instance of the [HistoryStorage] used * to query matching bookmarks. * @property loadUrlUseCase the use case invoked to load the url when the * user clicks on the suggestion. -- 2.11.4.GIT