From 86fdad8d490dbf168f0cdd60efecd1d955758e8b Mon Sep 17 00:00:00 2001 From: Travis Long Date: Fri, 17 Jul 2020 12:13:59 -0500 Subject: [PATCH] Bug 1653244 - Fix experiment info getting cleared before getting into metrics ping This is the fix for the Android/Kotlin bindings and a modified test to cover the case. --- .../glean/scheduler/MetricsPingScheduler.kt | 35 +++++-------------- .../scheduler/MetricsPingSchedulerTest.kt | 28 ++++++++++++++- 2 files changed, 36 insertions(+), 27 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/MetricsPingScheduler.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/MetricsPingScheduler.kt index 4ad3ef5538..353a446c8d 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/MetricsPingScheduler.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/MetricsPingScheduler.kt @@ -201,10 +201,10 @@ internal class MetricsPingScheduler( if (isDifferentVersion()) { Log.i(LOG_TAG, "The application just updated. Send metrics ping now.") - @Suppress("EXPERIMENTAL_API_USAGE") - Dispatchers.API.executeTask { - collectPingAndReschedule(now, startupPing = true, reason = Pings.metricsReasonCodes.upgrade) - } + // Since `schedule` is only ever called from Glean.initialize, we need to ensure + // that this gets executed now before the Application lifetime metrics get cleared. + collectPingAndReschedule(now, startupPing = true, reason = Pings.metricsReasonCodes.upgrade) + return } @@ -231,31 +231,14 @@ internal class MetricsPingScheduler( Log.i(LOG_TAG, "The 'metrics' ping was already sent today, ${now.time}.") schedulePingCollection(now, sendTheNextCalendarDay = true, reason = Pings.metricsReasonCodes.tomorrow) } - // The ping wasn't already sent today. Are we overdue or just waiting for - // the right time? This covers (2) isAfterDueTime(now) -> { + // The ping wasn't already sent today. Are we overdue or just waiting for + // the right time? This covers (2) Log.i(LOG_TAG, "The 'metrics' ping is scheduled for immediate collection, ${now.time}") - // **IMPORTANT** - // - // The reason why we're collecting the "metrics" ping in the `Dispatchers.API` - // context is that we want to make sure no other metric API adds data before - // the ping is collected. All the exposed metrics API dispatch calls to the - // engines through the `Dispatchers.API` context, so this ensures we are enqueued - // before any other recording API call. - // - // * Do not change `Dispatchers.API.executeTask` to `Dispatchers.API.launch` as - // this would break startup overdue ping collection. * - // `executeTask` schedules the task for immediate execution on the - // `Dispatchers.API` thread pool, before any other enqueued task. For more - // context, see bug 1604861 and the implementation of - // `collectPingAndReschedule`. - - @Suppress("EXPERIMENTAL_API_USAGE") - Dispatchers.API.executeTask { - // This addresses (2). - collectPingAndReschedule(now, startupPing = true, reason = Pings.metricsReasonCodes.overdue) - } + // Since `schedule` is only ever called from Glean.initialize, we need to ensure + // that this gets executed now before the Application lifetime metrics get cleared. + collectPingAndReschedule(now, startupPing = true, reason = Pings.metricsReasonCodes.overdue) } else -> { // This covers (3). diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt index 1965f33e44..162cf65d6b 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt @@ -476,7 +476,8 @@ class MetricsPingSchedulerTest { } @Test - fun `startup ping sends old version when upgraded`() { + @Suppress("LongMethod") + fun `startup ping sends correct data when upgraded`() { // Start the web-server that will receive the metrics ping. val server = getMockWebServer() val configuration = Configuration( @@ -510,10 +511,15 @@ class MetricsPingSchedulerTest { val expectedValue = "canary" expectedStringMetric.set(expectedValue) + // Set an experiment active to verify it gets sent in the pings + Glean.setExperimentActive("test-experiment", "a") + // Reset Glean. Glean.testDestroyGleanHandle() @Suppress("EXPERIMENTAL_API_USAGE") Dispatchers.API.setTaskQueueing(true) + @Suppress("EXPERIMENTAL_API_USAGE") + Dispatchers.API.setTestingMode(false) // Initialize Glean again with the new version. // This should trigger a metrics ping after an upgrade (differing version). @@ -523,6 +529,10 @@ class MetricsPingSchedulerTest { configuration ) + // Unfortunately, we need to delay a bit here to give init time to run because we are + // running async at this point in the test. + Thread.sleep(500) + // Trigger worker task to upload the pings in the background. triggerWorkManager(context) @@ -534,8 +544,22 @@ class MetricsPingSchedulerTest { val metricsJsonData = request.getPlainBody() val pingJson = JSONObject(metricsJsonData) + assertEquals("The metrics ping reason must be 'upgrade'", + "upgrade", pingJson.getJSONObject("ping_info")["reason"]) + assertEquals("The received ping must contain the old version", oldVersion, pingJson.getJSONObject("client_info")["app_display_version"]) + + val stringMetrics = pingJson.getJSONObject("metrics")["string"] as JSONObject + assertEquals("The received ping must contain the expected metric", + expectedValue, stringMetrics.getString("telemetry.expected_metric")) + + val experiments = pingJson.getJSONObject("ping_info")["experiments"] as JSONObject + val testExperiment = experiments["test-experiment"] as JSONObject + assertNotNull("The received ping must contain the experiment", + testExperiment) + assertEquals("Experiment branch should exist and match", + "a", testExperiment.getString("branch")) } finally { server.shutdown() @@ -543,6 +567,8 @@ class MetricsPingSchedulerTest { Glean.testDestroyGleanHandle() @Suppress("EXPERIMENTAL_API_USAGE") Dispatchers.API.setTaskQueueing(true) + @Suppress("EXPERIMENTAL_API_USAGE") + Dispatchers.API.setTestingMode(true) } }