From b5cb951ea4dff29a975be9ce837167b415deb697 Mon Sep 17 00:00:00 2001 From: Alexandru2909 Date: Thu, 14 Apr 2022 16:27:48 +0300 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/24786 - Remove Event.wrapper for SearchTerms telemetry --- app/metrics.yaml | 2 ++ .../mozilla/fenix/components/metrics/Event.kt | 16 ----------- .../components/metrics/GleanMetricsService.kt | 15 ----------- .../org/mozilla/fenix/home/HomeFragment.kt | 1 - .../controller/RecentTabController.kt | 6 ++--- .../fenix/tabstray/TabsTrayMiddleware.kt | 16 ++++++++--- .../metrics/MetricControllerTest.kt | 23 ---------------- .../controller/RecentTabControllerTest.kt | 21 ++++++++++++--- .../fenix/tabstray/TabsTrayMiddlewareTest.kt | 27 +++++++++++++++---- 9 files changed, 56 insertions(+), 71 deletions(-) diff --git a/app/metrics.yaml b/app/metrics.yaml index e5529ef499..c5028b60d1 100644 --- a/app/metrics.yaml +++ b/app/metrics.yaml @@ -7297,6 +7297,7 @@ search_terms: Number of search term group when tabs tray is opened. extra_keys: count: + type: string description: | The number of tabs per search group bugs: @@ -7314,6 +7315,7 @@ search_terms: Number of search term tabs per group when tabs tray is opened. extra_keys: count: + type: string description: | The average number of tabs per search group bugs: diff --git a/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt b/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt index b08cdeaf81..2902af9222 100644 --- a/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt +++ b/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt @@ -4,8 +4,6 @@ package org.mozilla.fenix.components.metrics -import org.mozilla.fenix.GleanMetrics.SearchTerms - sealed class Event { // Interaction Events @@ -61,20 +59,6 @@ sealed class Event { get() = keyName } - data class SearchTermGroupCount(val count: Int) : Event() { - override val extras: Map - get() = hashMapOf(SearchTerms.numberOfSearchTermGroupKeys.count to count.toString()) - } - - data class AverageTabsPerSearchTermGroup(val averageSize: Double) : Event() { - override val extras: Map - get() = hashMapOf(SearchTerms.averageTabsPerGroupKeys.count to averageSize.toString()) - } - - data class SearchTermGroupSizeDistribution(val groupSizes: List) : Event() - - object JumpBackInGroupTapped : Event() - sealed class Search sealed class Messaging(open val messageId: String) : Event() { diff --git a/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt b/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt index 074f81ea30..25d0e66b63 100644 --- a/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt +++ b/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt @@ -16,7 +16,6 @@ import org.mozilla.fenix.GleanMetrics.Pings import org.mozilla.fenix.GleanMetrics.ProgressiveWebApp import org.mozilla.fenix.GleanMetrics.RecentSearches import org.mozilla.fenix.GleanMetrics.RecentlyVisitedHomepage -import org.mozilla.fenix.GleanMetrics.SearchTerms import org.mozilla.fenix.GleanMetrics.StartOnHome import org.mozilla.fenix.GleanMetrics.SyncedTabs import org.mozilla.fenix.GleanMetrics.Tabs @@ -140,20 +139,6 @@ private val Event.wrapper: EventWrapper<*>? { RecentSearches.groupDeleted.record(it) } ) - is Event.SearchTermGroupCount -> EventWrapper( - { SearchTerms.numberOfSearchTermGroup.record(it) }, - { SearchTerms.numberOfSearchTermGroupKeys.valueOf(it) } - ) - is Event.AverageTabsPerSearchTermGroup -> EventWrapper( - { SearchTerms.averageTabsPerGroup.record(it) }, - { SearchTerms.averageTabsPerGroupKeys.valueOf(it) } - ) - is Event.SearchTermGroupSizeDistribution -> EventWrapper( - { SearchTerms.groupSizeDistribution.accumulateSamples(this.groupSizes.toLongArray()) }, - ) - is Event.JumpBackInGroupTapped -> EventWrapper( - { SearchTerms.jumpBackInGroupTapped.record(it) } - ) is Event.Messaging.MessageShown -> EventWrapper( { Messaging.messageShown.record( diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt index 3fd2a10106..0dc9d4fd92 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -359,7 +359,6 @@ class HomeFragment : Fragment() { recentTabController = DefaultRecentTabsController( selectTabUseCase = components.useCases.tabsUseCases.selectTab, navController = findNavController(), - metrics = requireComponents.analytics.metrics, store = components.core.store, appStore = components.appStore, ), diff --git a/app/src/main/java/org/mozilla/fenix/home/recenttabs/controller/RecentTabController.kt b/app/src/main/java/org/mozilla/fenix/home/recenttabs/controller/RecentTabController.kt index b541c13312..31cab87e20 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recenttabs/controller/RecentTabController.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recenttabs/controller/RecentTabController.kt @@ -10,11 +10,10 @@ import androidx.navigation.NavController import mozilla.components.browser.state.store.BrowserStore import mozilla.components.feature.tabs.TabsUseCases.SelectTabUseCase import mozilla.components.service.glean.private.NoExtras +import org.mozilla.fenix.GleanMetrics.SearchTerms import org.mozilla.fenix.R import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.components.appstate.AppAction -import org.mozilla.fenix.components.metrics.Event -import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.ext.inProgressMediaTab import org.mozilla.fenix.home.HomeFragmentDirections import org.mozilla.fenix.home.recenttabs.RecentTab @@ -56,7 +55,6 @@ interface RecentTabController { class DefaultRecentTabsController( private val selectTabUseCase: SelectTabUseCase, private val navController: NavController, - private val metrics: MetricController, private val store: BrowserStore, private val appStore: AppStore, ) : RecentTabController { @@ -79,7 +77,7 @@ class DefaultRecentTabsController( } override fun handleRecentSearchGroupClicked(tabId: String) { - metrics.track(Event.JumpBackInGroupTapped) + SearchTerms.jumpBackInGroupTapped.record(NoExtras()) navController.navigate( HomeFragmentDirections.actionGlobalTabsTrayFragment( focusGroupTabId = tabId diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayMiddleware.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayMiddleware.kt index 2e918a88a0..ceb0946a41 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayMiddleware.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayMiddleware.kt @@ -8,8 +8,8 @@ import androidx.annotation.VisibleForTesting import mozilla.components.lib.state.Middleware import mozilla.components.lib.state.MiddlewareContext import org.mozilla.fenix.GleanMetrics.Metrics +import org.mozilla.fenix.GleanMetrics.SearchTerms import org.mozilla.fenix.GleanMetrics.TabsTray -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController /** @@ -45,15 +45,23 @@ class TabsTrayMiddleware( shouldReportSearchGroupMetrics = false val tabGroups = action.tabPartition?.tabGroups ?: emptyList() - metrics.track(Event.SearchTermGroupCount(tabGroups.size)) + SearchTerms.numberOfSearchTermGroup.record( + SearchTerms.NumberOfSearchTermGroupExtra( + tabGroups.size.toString() + ) + ) if (tabGroups.isNotEmpty()) { val tabsPerGroup = tabGroups.map { it.tabIds.size } val averageTabsPerGroup = tabsPerGroup.average() - metrics.track(Event.AverageTabsPerSearchTermGroup(averageTabsPerGroup)) + SearchTerms.averageTabsPerGroup.record( + SearchTerms.AverageTabsPerGroupExtra( + averageTabsPerGroup.toString() + ) + ) val tabGroupSizeMapping = tabsPerGroup.map { generateTabGroupSizeMappedValue(it) } - metrics.track(Event.SearchTermGroupSizeDistribution(tabGroupSizeMapping)) + SearchTerms.groupSizeDistribution.accumulateSamples(tabGroupSizeMapping.toLongArray()) } } } diff --git a/app/src/test/java/org/mozilla/fenix/components/metrics/MetricControllerTest.kt b/app/src/test/java/org/mozilla/fenix/components/metrics/MetricControllerTest.kt index d2d3b70f6f..baaad94ba4 100644 --- a/app/src/test/java/org/mozilla/fenix/components/metrics/MetricControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/metrics/MetricControllerTest.kt @@ -381,29 +381,6 @@ class MetricControllerTest { } } - @Test - fun `search term group events should be sent to enabled service`() { - val controller = ReleaseMetricController( - listOf(dataService1), - isDataTelemetryEnabled = { true }, - isMarketingDataTelemetryEnabled = { true }, - mockk() - ) - every { dataService1.shouldTrack(Event.SearchTermGroupCount(5)) } returns true - every { dataService1.shouldTrack(Event.AverageTabsPerSearchTermGroup(2.5)) } returns true - every { dataService1.shouldTrack(Event.JumpBackInGroupTapped) } returns true - - controller.start(MetricServiceType.Data) - - controller.track(Event.SearchTermGroupCount(5)) - controller.track(Event.AverageTabsPerSearchTermGroup(2.5)) - controller.track(Event.JumpBackInGroupTapped) - - verify { dataService1.track(Event.SearchTermGroupCount(5)) } - verify { dataService1.track(Event.AverageTabsPerSearchTermGroup(2.5)) } - verify { dataService1.track(Event.JumpBackInGroupTapped) } - } - @Test fun `WHEN processing a FEATURE_AUTOFILL fact THEN the right metric is recorded`() { val controller = ReleaseMetricController(emptyList(), { true }, { true }, mockk()) diff --git a/app/src/test/java/org/mozilla/fenix/home/recenttabs/controller/RecentTabControllerTest.kt b/app/src/test/java/org/mozilla/fenix/home/recenttabs/controller/RecentTabControllerTest.kt index 9008d4c5be..ced628147d 100644 --- a/app/src/test/java/org/mozilla/fenix/home/recenttabs/controller/RecentTabControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/recenttabs/controller/RecentTabControllerTest.kt @@ -28,9 +28,9 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.GleanMetrics.RecentTabs +import org.mozilla.fenix.GleanMetrics.SearchTerms import org.mozilla.fenix.R import org.mozilla.fenix.components.AppStore -import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.helpers.FenixRobolectricTestRunner @OptIn(ExperimentalCoroutinesApi::class) @@ -45,7 +45,6 @@ class RecentTabControllerTest { private val navController: NavController = mockk(relaxed = true) private val selectTabUseCase: TabsUseCases = mockk(relaxed = true) - private val metrics: MetricController = mockk(relaxed = true) private val appStore: AppStore = mockk() private lateinit var store: BrowserStore @@ -61,7 +60,6 @@ class RecentTabControllerTest { DefaultRecentTabsController( selectTabUseCase = selectTabUseCase.selectTab, navController = navController, - metrics = metrics, store = store, appStore = appStore, ) @@ -165,4 +163,21 @@ class RecentTabControllerTest { assertTrue(RecentTabs.showAllClicked.testHasValue()) } + + @Test + fun `WHEN handleRecentSearchGroupClicked is called THEN navigate to the tabsTrayFragment and record the correct metric`() { + assertFalse(SearchTerms.jumpBackInGroupTapped.testHasValue()) + + controller.handleRecentSearchGroupClicked("1") + + verify { + navController.navigate( + match { + it.actionId == R.id.action_global_tabsTrayFragment && + it.arguments["focusGroupTabId"] == "1" + } + ) + } + assertTrue(SearchTerms.jumpBackInGroupTapped.testHasValue()) + } } diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/TabsTrayMiddlewareTest.kt b/app/src/test/java/org/mozilla/fenix/tabstray/TabsTrayMiddlewareTest.kt index 408ab9c170..e6e6dd677a 100644 --- a/app/src/test/java/org/mozilla/fenix/tabstray/TabsTrayMiddlewareTest.kt +++ b/app/src/test/java/org/mozilla/fenix/tabstray/TabsTrayMiddlewareTest.kt @@ -6,7 +6,6 @@ package org.mozilla.fenix.tabstray import io.mockk.every import io.mockk.mockk -import io.mockk.verify import mozilla.components.browser.state.state.TabGroup import mozilla.components.browser.state.state.TabPartition import mozilla.components.service.glean.testing.GleanTestRule @@ -20,8 +19,8 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.GleanMetrics.Metrics +import org.mozilla.fenix.GleanMetrics.SearchTerms import org.mozilla.fenix.GleanMetrics.TabsTray -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.helpers.FenixRobolectricTestRunner @@ -49,23 +48,41 @@ class TabsTrayMiddlewareTest { @Test fun `WHEN search term groups are updated AND there is at least one group THEN report the average tabs per group`() { + assertFalse(SearchTerms.averageTabsPerGroup.testHasValue()) + store.dispatch(TabsTrayAction.UpdateTabPartitions(generateSearchTermTabGroupsForAverage())) store.waitUntilIdle() - verify { metrics.track(Event.AverageTabsPerSearchTermGroup(5.0)) } + + assertTrue(SearchTerms.averageTabsPerGroup.testHasValue()) + val event = SearchTerms.averageTabsPerGroup.testGetValue() + assertEquals(1, event.size) + assertEquals("5.0", event.single().extra!!["count"]) } @Test fun `WHEN search term groups are updated AND there is at least one group THEN report the distribution of tab sizes`() { + assertFalse(SearchTerms.groupSizeDistribution.testHasValue()) + store.dispatch(TabsTrayAction.UpdateTabPartitions(generateSearchTermTabGroupsForDistribution())) store.waitUntilIdle() - verify { metrics.track(Event.SearchTermGroupSizeDistribution(listOf(3L, 2L, 1L, 4L))) } + + assertTrue(SearchTerms.groupSizeDistribution.testHasValue()) + val event = SearchTerms.groupSizeDistribution.testGetValue().values + // Verify the distribution correctly describes the tab group sizes + assertEquals(mapOf(0L to 0L, 1L to 1L, 2L to 1L, 3L to 1L, 4L to 1L), event) } @Test fun `WHEN search term groups are updated THEN report the count of search term tab groups`() { + assertFalse(SearchTerms.numberOfSearchTermGroup.testHasValue()) + store.dispatch(TabsTrayAction.UpdateTabPartitions(null)) store.waitUntilIdle() - verify { metrics.track(Event.SearchTermGroupCount(0)) } + + assertTrue(SearchTerms.numberOfSearchTermGroup.testHasValue()) + val event = SearchTerms.numberOfSearchTermGroup.testGetValue() + assertEquals(1, event.size) + assertEquals("0", event.single().extra!!["count"]) } @Test