From 3afee842eebabcab3eed0309c2e7393b775e66cc Mon Sep 17 00:00:00 2001 From: mcarare Date: Thu, 14 Apr 2022 15:45:48 +0300 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/24758: Remove wrapper from browser search metrics. --- .../mozilla/fenix/components/metrics/Event.kt | 15 ----- .../components/metrics/GleanMetricsService.kt | 20 +----- .../components/metrics/MetricController.kt | 21 +++--- .../metrics/MetricControllerTest.kt | 67 ++++++++++++++++++- 4 files changed, 79 insertions(+), 44 deletions(-) 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 357a0a613..b625cbd09 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 @@ -8,21 +8,6 @@ sealed class Event { // Interaction events with extras - data class SearchWithAds(val providerName: String) : Event() { - val label: String - get() = providerName - } - - data class SearchAdClicked(val keyName: String) : Event() { - val label: String - get() = keyName - } - - data class SearchInContent(val keyName: String) : Event() { - val label: String - get() = keyName - } - sealed class Search internal open val extras: Map<*, String>? 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 f8050aa9c..9c3e9ac68 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 @@ -6,9 +6,7 @@ package org.mozilla.fenix.components.metrics import android.content.Context import mozilla.components.service.glean.Glean -import mozilla.components.service.glean.private.NoExtraKeys import mozilla.components.support.base.log.logger.Logger -import org.mozilla.fenix.GleanMetrics.BrowserSearch import org.mozilla.fenix.GleanMetrics.Pings import org.mozilla.fenix.ext.components @@ -54,23 +52,7 @@ private class EventWrapper>( @Suppress("DEPRECATION") // FIXME(#19967): Migrate to non-deprecated API. private val Event.wrapper: EventWrapper<*>? - get() = when (this) { - is Event.SearchWithAds -> EventWrapper( - { - BrowserSearch.withAds[label].add(1) - } - ) - is Event.SearchAdClicked -> EventWrapper( - { - BrowserSearch.adClicks[label].add(1) - } - ) - is Event.SearchInContent -> EventWrapper( - { - BrowserSearch.inContent[label].add(1) - } - ) - } + get() = null /** * Service responsible for sending the activation and installation pings. diff --git a/app/src/main/java/org/mozilla/fenix/components/metrics/MetricController.kt b/app/src/main/java/org/mozilla/fenix/components/metrics/MetricController.kt index 5e800ce14..8565725e9 100644 --- a/app/src/main/java/org/mozilla/fenix/components/metrics/MetricController.kt +++ b/app/src/main/java/org/mozilla/fenix/components/metrics/MetricController.kt @@ -39,6 +39,7 @@ import org.mozilla.fenix.GleanMetrics.Addons import org.mozilla.fenix.GleanMetrics.ContextMenu import org.mozilla.fenix.GleanMetrics.AndroidAutofill import org.mozilla.fenix.GleanMetrics.Awesomebar +import org.mozilla.fenix.GleanMetrics.BrowserSearch import org.mozilla.fenix.GleanMetrics.ContextualMenu import org.mozilla.fenix.GleanMetrics.CreditCards import org.mozilla.fenix.GleanMetrics.CustomTab @@ -247,6 +248,16 @@ internal class ReleaseMetricController( ProgressiveWebApp.installTap.record(NoExtras()) } + Component.FEATURE_SEARCH to AdsTelemetry.SERP_ADD_CLICKED -> { + BrowserSearch.adClicks[value!!].add() + } + Component.FEATURE_SEARCH to AdsTelemetry.SERP_SHOWN_WITH_ADDS -> { + BrowserSearch.withAds[value!!].add() + } + Component.FEATURE_SEARCH to InContentTelemetry.IN_CONTENT_SEARCH -> { + BrowserSearch.inContent[value!!].add() + } + else -> { this.toEvent()?.also { track(it) @@ -363,15 +374,7 @@ internal class ReleaseMetricController( } null } - Component.FEATURE_SEARCH == component && AdsTelemetry.SERP_ADD_CLICKED == item -> { - Event.SearchAdClicked(value!!) - } - Component.FEATURE_SEARCH == component && AdsTelemetry.SERP_SHOWN_WITH_ADDS == item -> { - Event.SearchWithAds(value!!) - } - Component.FEATURE_SEARCH == component && InContentTelemetry.IN_CONTENT_SEARCH == item -> { - Event.SearchInContent(value!!) - } + else -> null } 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 cdad4b3ad..7c556a6ef 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 @@ -19,6 +19,8 @@ import mozilla.components.feature.media.facts.MediaFacts import mozilla.components.feature.prompts.dialog.LoginDialogFacts import mozilla.components.feature.prompts.facts.CreditCardAutofillDialogFacts import mozilla.components.feature.pwa.ProgressiveWebAppFacts +import mozilla.components.feature.search.telemetry.ads.AdsTelemetry +import mozilla.components.feature.search.telemetry.incontent.InContentTelemetry import mozilla.components.feature.syncedtabs.facts.SyncedTabsFacts import mozilla.components.feature.top.sites.facts.TopSitesFacts import mozilla.components.support.base.Component @@ -37,9 +39,10 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.GleanMetrics.AndroidAutofill +import org.mozilla.fenix.GleanMetrics.Awesomebar +import org.mozilla.fenix.GleanMetrics.BrowserSearch import org.mozilla.fenix.GleanMetrics.ContextualMenu import org.mozilla.fenix.GleanMetrics.CreditCards -import org.mozilla.fenix.GleanMetrics.Awesomebar import org.mozilla.fenix.GleanMetrics.CustomTab import org.mozilla.fenix.GleanMetrics.LoginDialog import org.mozilla.fenix.GleanMetrics.MediaNotification @@ -609,4 +612,66 @@ class MetricControllerTest { assertTrue(Awesomebar.openedTabSuggestionClicked.testHasValue()) } + + @Test + fun `GIVEN advertising search facts WHEN the list is processed THEN the right metric is recorded`() { + val controller = ReleaseMetricController(emptyList(), { true }, { false }, mockk()) + val action = mockk() + + // an ad was clicked in a Search Engine Result Page + val addClickedInSearchFact = Fact( + Component.FEATURE_SEARCH, + action, + AdsTelemetry.SERP_ADD_CLICKED, + "provider" + ) + + assertFalse(BrowserSearch.adClicks["provider"].testHasValue()) + controller.run { + addClickedInSearchFact.process() + } + assertTrue(BrowserSearch.adClicks["provider"].testHasValue()) + assertEquals(1, BrowserSearch.adClicks["provider"].testGetValue()) + + // the user opened a Search Engine Result Page of one of our search providers which contains ads + val searchWithAdsOpenedFact = Fact( + Component.FEATURE_SEARCH, + action, + AdsTelemetry.SERP_SHOWN_WITH_ADDS, + "provider" + ) + + assertFalse(BrowserSearch.withAds["provider"].testHasValue()) + + controller.run { + searchWithAdsOpenedFact.process() + } + + assertTrue(BrowserSearch.withAds["provider"].testHasValue()) + assertEquals(1, BrowserSearch.withAds["provider"].testGetValue()) + + // the user performed a search + val inContentSearchFact = Fact( + Component.FEATURE_SEARCH, + action, + InContentTelemetry.IN_CONTENT_SEARCH, + "provider" + ) + + assertFalse(BrowserSearch.inContent["provider"].testHasValue()) + + controller.run { + inContentSearchFact.process() + } + + assertTrue(BrowserSearch.inContent["provider"].testHasValue()) + assertEquals(1, BrowserSearch.inContent["provider"].testGetValue()) + + // the user performed another search + controller.run { + inContentSearchFact.process() + } + + assertEquals(2, BrowserSearch.inContent["provider"].testGetValue()) + } }