From aa64a4aac9f75f18a424ed3aa1360bedd1ca3e55 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 9 Jan 2023 10:51:23 -0500 Subject: [PATCH] For #28428: update growth data with new events (#28449) (cherry picked from commit 24674cbec4a55cb89da5d447c4babbc5053011c7) Co-authored-by: MatthewTighe --- .../mozilla/fenix/components/metrics/Event.kt | 9 +-- .../components/metrics/MetricController.kt | 1 + .../components/metrics/MetricsMiddleware.kt | 5 +- .../components/metrics/MetricsStorage.kt | 50 +++++------- .../fenix/telemetry/TelemetryMiddleware.kt | 4 - .../java/org/mozilla/fenix/utils/Settings.kt | 15 ++-- app/src/main/res/values/preference_keys.xml | 3 +- .../metrics/DefaultMetricsStorageTest.kt | 80 +++++-------------- .../telemetry/TelemetryMiddlewareTest.kt | 9 --- 9 files changed, 50 insertions(+), 126 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 77fedea9a..9fec84140 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 @@ -23,14 +23,9 @@ sealed class Event { object SetAsDefault : GrowthData("xgpcgt") /** - * Event recording the first time Firefox has been resumed in a 24 hour period. + * Event recording that an ad was clicked in a search engine results page. */ - object FirstAppOpenForDay : GrowthData("41hl22") - - /** - * Event recording the first time a URI is loaded in Firefox in a 24 hour period. - */ - object FirstUriLoadForDay : GrowthData("ja86ek") + object SerpAdClicked : GrowthData("e2x17e") /** * Event recording the first time Firefox is used 3 days in a row in the first week of install. 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 d5590efe7..6b1f94994 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 @@ -262,6 +262,7 @@ internal class ReleaseMetricController( Component.FEATURE_SEARCH to AdsTelemetry.SERP_ADD_CLICKED -> { BrowserSearch.adClicks[value!!].add() + track(Event.GrowthData.SerpAdClicked) } Component.FEATURE_SEARCH to AdsTelemetry.SERP_SHOWN_WITH_ADDS -> { BrowserSearch.withAds[value!!].add() diff --git a/app/src/main/java/org/mozilla/fenix/components/metrics/MetricsMiddleware.kt b/app/src/main/java/org/mozilla/fenix/components/metrics/MetricsMiddleware.kt index e7d7cf2a1..83a1320f0 100644 --- a/app/src/main/java/org/mozilla/fenix/components/metrics/MetricsMiddleware.kt +++ b/app/src/main/java/org/mozilla/fenix/components/metrics/MetricsMiddleware.kt @@ -1,3 +1,7 @@ +/* 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.components.metrics import mozilla.components.lib.state.Middleware @@ -23,7 +27,6 @@ class MetricsMiddleware( private fun handleAction(action: AppAction) = when (action) { is AppAction.ResumedMetricsAction -> { metrics.track(Event.GrowthData.SetAsDefault) - metrics.track(Event.GrowthData.FirstAppOpenForDay) metrics.track(Event.GrowthData.FirstWeekSeriesActivity) } else -> Unit diff --git a/app/src/main/java/org/mozilla/fenix/components/metrics/MetricsStorage.kt b/app/src/main/java/org/mozilla/fenix/components/metrics/MetricsStorage.kt index f19557f7b..7cb678a8a 100644 --- a/app/src/main/java/org/mozilla/fenix/components/metrics/MetricsStorage.kt +++ b/app/src/main/java/org/mozilla/fenix/components/metrics/MetricsStorage.kt @@ -47,21 +47,20 @@ internal class DefaultMetricsStorage( */ override suspend fun shouldTrack(event: Event): Boolean = withContext(dispatcher) { - // The side-effect of storing days of use needs to happen during the first two days after - // install, which would normally be skipped by shouldSendGenerally. + // The side-effect of storing days of use always needs to happen. updateDaysOfUse() + val currentTime = System.currentTimeMillis() shouldSendGenerally() && when (event) { Event.GrowthData.SetAsDefault -> { - !settings.setAsDefaultGrowthSent && checkDefaultBrowser() - } - Event.GrowthData.FirstAppOpenForDay -> { - settings.resumeGrowthLastSent.hasBeenMoreThanDaySince() - } - Event.GrowthData.FirstUriLoadForDay -> { - settings.uriLoadGrowthLastSent.hasBeenMoreThanDaySince() + currentTime.duringFirstMonth() && + !settings.setAsDefaultGrowthSent && + checkDefaultBrowser() } Event.GrowthData.FirstWeekSeriesActivity -> { - shouldTrackFirstWeekActivity() + currentTime.duringFirstMonth() && shouldTrackFirstWeekActivity() + } + Event.GrowthData.SerpAdClicked -> { + currentTime.duringFirstMonth() && !settings.adClickGrowthSent } } } @@ -71,15 +70,12 @@ internal class DefaultMetricsStorage( Event.GrowthData.SetAsDefault -> { settings.setAsDefaultGrowthSent = true } - Event.GrowthData.FirstAppOpenForDay -> { - settings.resumeGrowthLastSent = System.currentTimeMillis() - } - Event.GrowthData.FirstUriLoadForDay -> { - settings.uriLoadGrowthLastSent = System.currentTimeMillis() - } Event.GrowthData.FirstWeekSeriesActivity -> { settings.firstWeekSeriesGrowthSent = true } + Event.GrowthData.SerpAdClicked -> { + settings.adClickGrowthSent = true + } } } @@ -87,13 +83,13 @@ internal class DefaultMetricsStorage( val daysOfUse = settings.firstWeekDaysOfUseGrowthData val currentDate = Calendar.getInstance(Locale.US) val currentDateString = dateFormatter.format(currentDate.time) - if (currentDate.timeInMillis.withinFirstWeek() && daysOfUse.none { it == currentDateString }) { + if (currentDate.timeInMillis.duringFirstWeek() && daysOfUse.none { it == currentDateString }) { settings.firstWeekDaysOfUseGrowthData = daysOfUse + currentDateString } } private fun shouldTrackFirstWeekActivity(): Boolean = Result.runCatching { - if (!System.currentTimeMillis().withinFirstWeek() || settings.firstWeekSeriesGrowthSent) { + if (!System.currentTimeMillis().duringFirstWeek() || settings.firstWeekSeriesGrowthSent) { return false } @@ -121,14 +117,13 @@ internal class DefaultMetricsStorage( return false }.getOrDefault(false) - private fun Long.hasBeenMoreThanDaySince(): Boolean = - System.currentTimeMillis() - this > dayMillis - private fun Long.toCalendar(): Calendar = Calendar.getInstance(Locale.US).also { calendar -> calendar.timeInMillis = this } - private fun Long.withinFirstWeek() = this < getInstalledTime() + fullWeekMillis + private fun Long.duringFirstWeek() = this < getInstalledTime() + fullWeekMillis + + private fun Long.duringFirstMonth() = this < getInstalledTime() + shortestMonthMillis private fun Calendar.createNextDay() = (this.clone() as Calendar).also { calendar -> calendar.add(Calendar.DAY_OF_MONTH, 1) @@ -136,8 +131,7 @@ internal class DefaultMetricsStorage( companion object { private const val dayMillis: Long = 1000 * 60 * 60 * 24 - private const val windowStartMillis: Long = dayMillis * 2 - private const val windowEndMillis: Long = dayMillis * 28 + private const val shortestMonthMillis: Long = dayMillis * 28 // Note this is 8 so that recording of FirstWeekSeriesActivity happens throughout the length // of the 7th day after install @@ -146,17 +140,11 @@ internal class DefaultMetricsStorage( /** * Determines whether events should be tracked based on some general criteria: * - user has installed as a result of a campaign - * - user is within 2-28 days of install * - tracking is still enabled through Nimbus */ fun shouldSendGenerally(context: Context): Boolean { - val installedTime = getInstalledTime(context) - val timeDifference = System.currentTimeMillis() - installedTime - val withinWindow = timeDifference in windowStartMillis..windowEndMillis - return context.settings().adjustCampaignId.isNotEmpty() && - FxNimbus.features.growthData.value().enabled && - withinWindow + FxNimbus.features.growthData.value().enabled } fun getInstalledTime(context: Context): Long = context.packageManager diff --git a/app/src/main/java/org/mozilla/fenix/telemetry/TelemetryMiddleware.kt b/app/src/main/java/org/mozilla/fenix/telemetry/TelemetryMiddleware.kt index c53ca405f..3c138f6d4 100644 --- a/app/src/main/java/org/mozilla/fenix/telemetry/TelemetryMiddleware.kt +++ b/app/src/main/java/org/mozilla/fenix/telemetry/TelemetryMiddleware.kt @@ -21,7 +21,6 @@ import mozilla.components.support.base.android.Clock import mozilla.components.support.base.log.logger.Logger import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.Metrics -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.utils.Settings import org.mozilla.fenix.GleanMetrics.EngineTab as EngineMetrics @@ -60,9 +59,6 @@ class TelemetryMiddleware( val tab = context.state.findTabOrCustomTab(action.tabId) onEngineSessionKilled(context.state, tab) } - is EngineAction.LoadUrlAction -> { - metrics.track(Event.GrowthData.FirstUriLoadForDay) - } else -> { // no-op } diff --git a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt index 944c4f008..379c87af4 100644 --- a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt +++ b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt @@ -1467,16 +1467,6 @@ class Settings(private val appContext: Context) : PreferencesHolder { default = false, ) - var resumeGrowthLastSent by longPreference( - key = appContext.getPreferenceKey(R.string.pref_key_growth_resume_last_sent), - default = 0, - ) - - var uriLoadGrowthLastSent by longPreference( - key = appContext.getPreferenceKey(R.string.pref_key_growth_uri_load_last_sent), - default = 0, - ) - var firstWeekSeriesGrowthSent by booleanPreference( key = appContext.getPreferenceKey(R.string.pref_key_growth_first_week_series_sent), default = false, @@ -1486,4 +1476,9 @@ class Settings(private val appContext: Context) : PreferencesHolder { key = appContext.getPreferenceKey(R.string.pref_key_growth_first_week_days_of_use), default = setOf(), ) + + var adClickGrowthSent by booleanPreference( + key = appContext.getPreferenceKey(R.string.pref_key_growth_ad_click_sent), + default = false, + ) } diff --git a/app/src/main/res/values/preference_keys.xml b/app/src/main/res/values/preference_keys.xml index 8c02486dd..fe2d3fa75 100644 --- a/app/src/main/res/values/preference_keys.xml +++ b/app/src/main/res/values/preference_keys.xml @@ -316,8 +316,7 @@ pref_key_growth_set_as_default - pref_key_growth_last_resumed - pref_key_growth_uri_load_last_sent pref_key_growth_first_week_series_sent pref_key_growth_first_week_days_of_use + pref_key_growth_ad_click_sent diff --git a/app/src/test/java/org/mozilla/fenix/components/metrics/DefaultMetricsStorageTest.kt b/app/src/test/java/org/mozilla/fenix/components/metrics/DefaultMetricsStorageTest.kt index 4266a4dee..d3f5d3481 100644 --- a/app/src/test/java/org/mozilla/fenix/components/metrics/DefaultMetricsStorageTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/metrics/DefaultMetricsStorageTest.kt @@ -100,68 +100,6 @@ class DefaultMetricsStorageTest { assertTrue(updateSlot.captured) } - @Test - fun `GIVEN that it has been less than 24 hours since last resumed sent WHEN checked for sending THEN will not be sent`() = runTest(dispatcher) { - val currentTime = System.currentTimeMillis() - every { settings.resumeGrowthLastSent } returns currentTime - - val result = storage.shouldTrack(Event.GrowthData.FirstAppOpenForDay) - - assertFalse(result) - } - - @Test - fun `GIVEN that it has been more than 24 hours since last resumed sent WHEN checked for sending THEN will be sent`() = runTest(dispatcher) { - val currentTime = System.currentTimeMillis() - every { settings.resumeGrowthLastSent } returns currentTime - 1000 * 60 * 60 * 24 * 2 - - val result = storage.shouldTrack(Event.GrowthData.FirstAppOpenForDay) - - assertTrue(result) - } - - @Test - fun `WHEN last resumed state updated THEN settings updated accordingly`() = runTest(dispatcher) { - val updateSlot = slot() - every { settings.resumeGrowthLastSent } returns 0 - every { settings.resumeGrowthLastSent = capture(updateSlot) } returns Unit - - storage.updateSentState(Event.GrowthData.FirstAppOpenForDay) - - assertTrue(updateSlot.captured > 0) - } - - @Test - fun `GIVEN that it has been less than 24 hours since uri load sent WHEN checked for sending THEN will not be sent`() = runTest(dispatcher) { - val currentTime = System.currentTimeMillis() - every { settings.uriLoadGrowthLastSent } returns currentTime - - val result = storage.shouldTrack(Event.GrowthData.FirstUriLoadForDay) - - assertFalse(result) - } - - @Test - fun `GIVEN that it has been more than 24 hours since uri load sent WHEN checked for sending THEN will be sent`() = runTest(dispatcher) { - val currentTime = System.currentTimeMillis() - every { settings.uriLoadGrowthLastSent } returns currentTime - 1000 * 60 * 60 * 24 * 2 - - val result = storage.shouldTrack(Event.GrowthData.FirstUriLoadForDay) - - assertTrue(result) - } - - @Test - fun `WHEN uri load updated THEN settings updated accordingly`() = runTest(dispatcher) { - val updateSlot = slot() - every { settings.uriLoadGrowthLastSent } returns 0 - every { settings.uriLoadGrowthLastSent = capture(updateSlot) } returns Unit - - storage.updateSentState(Event.GrowthData.FirstUriLoadForDay) - - assertTrue(updateSlot.captured > 0) - } - @Test fun `GIVEN that app has been used for less than 3 days in a row WHEN checked for first week activity THEN event will not be sent`() = runTest(dispatcher) { val tomorrow = calendarStart.createNextDay() @@ -273,6 +211,24 @@ class DefaultMetricsStorageTest { assertFalse(captureSlot.isCaptured) } + @Test + fun `GIVEN serp ad clicked event already sent WHEN checking to track serp ad clicked THEN event will not be sent`() = runTest(dispatcher) { + every { settings.adClickGrowthSent } returns true + + val result = storage.shouldTrack(Event.GrowthData.SerpAdClicked) + + assertFalse(result) + } + + @Test + fun `GIVEN serp ad clicked event not sent WHEN checking to track serp ad clicked THEN event will be sent`() = runTest(dispatcher) { + every { settings.adClickGrowthSent } returns false + + val result = storage.shouldTrack(Event.GrowthData.SerpAdClicked) + + assertTrue(result) + } + private fun Calendar.copy() = clone() as Calendar private fun Calendar.createNextDay() = copy().apply { add(Calendar.DAY_OF_MONTH, 1) diff --git a/app/src/test/java/org/mozilla/fenix/telemetry/TelemetryMiddlewareTest.kt b/app/src/test/java/org/mozilla/fenix/telemetry/TelemetryMiddlewareTest.kt index 77cf0b383..b8bd91bf9 100644 --- a/app/src/test/java/org/mozilla/fenix/telemetry/TelemetryMiddlewareTest.kt +++ b/app/src/test/java/org/mozilla/fenix/telemetry/TelemetryMiddlewareTest.kt @@ -6,7 +6,6 @@ package org.mozilla.fenix.telemetry import androidx.test.core.app.ApplicationProvider import io.mockk.mockk -import io.mockk.verify import mozilla.components.browser.state.action.ContentAction import mozilla.components.browser.state.action.EngineAction import mozilla.components.browser.state.action.TabListAction @@ -33,7 +32,6 @@ import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.Metrics -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.utils.Settings @@ -352,13 +350,6 @@ class TelemetryMiddlewareTest { assertNull(EngineMetrics.killForegroundAge.testGetValue()) assertEquals(600_000_000, EngineMetrics.killBackgroundAge.testGetValue()!!.sum) } - - @Test - fun `WHEN uri loaded to engine THEN matching event is sent to metrics`() { - store.dispatch(EngineAction.LoadUrlAction("", "")).joinBlocking() - - verify { metrics.track(Event.GrowthData.FirstUriLoadForDay) } - } } internal class FakeClock : Clock.Delegate {