Bug 1653244 - Fix experiment info getting cleared before getting into metrics ping

CHANGELOG.md

@@ -6,6 +6,8 @@
   * BUGFIX: fix `int32` to `ErrorType` mapping. The `InvalidOverflow` had a value mismatch between glean-core and the bindings. This would only be a problem in unit tests. ([#1063](https://github.com/mozilla/glean/pull/1063))
   * Implement a JWE metric type ([#1062](https://github.com/mozilla/glean/pull/1062)).
   * Enable propagating options to the main product Activity when using the `GleanDebugActivity`.
+* Android
+  * BUGFIX: Fix the metrics ping collection for startup pings such as `reason=upgrade` to occur in the same thread/task as Glean initialize. Otherwise, it gets collected after the application lifetime metrics are cleared such as experiments that should be in the ping. ([#1069](https://github.com/mozilla/glean/pull/1069))
 
 # v31.4.0 (2020-07-16)
 

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).

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,15 +544,31 @@ 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()
 
             // Reset Glean.
             Glean.testDestroyGleanHandle()
             @Suppress("EXPERIMENTAL_API_USAGE")
             Dispatchers.API.setTaskQueueing(true)
+            @Suppress("EXPERIMENTAL_API_USAGE")
+            Dispatchers.API.setTestingMode(true)
         }
     }