Bug 1649926 - Always enqueue an async task to change upload and deprecate getUploadEnabled

CHANGELOG.md

@@ -4,6 +4,11 @@
 
 * General
   * Implement JWE metric type ([#1073](https://github.com/mozilla/glean/pull/1073), [#1062](https://github.com/mozilla/glean/pull/1062)).
+  * DEPRECATION: `getUploadEnabled` is deprecated (respectively `get_upload_enabled` in Python) ([#1046](https://github.com/mozilla/glean/pull/1046))
+    * Due to Glean's asynchronous initialization the return value can be incorrect.
+      Applications should not rely on Glean's internal state.
+      Upload enabled status should be tracked by the application and communicated to Glean if it changes.
+      Note: The method was removed from the C# and Python implementation.
 
 # v31.5.0 (2020-07-22)
 

docs/user/general-api.md

@@ -18,7 +18,6 @@ The Glean SDK provides a general API that supports the following operations. See
 | --------- | ----------- | ----- |
 | `initialize` | Configure and initialize the Glean SDK. | [Initializing the Glean SDK](#initializing-the-glean-sdk) |
 | `setUploadEnabled` | Enable or disable Glean collection and upload. | [Enabling and disabling Metrics](#enabling-and-disabling-metrics) |
-| `getUploadEnabled` | Get whether or not Glean is allowed to record and upload data. | |
 | `registerPings` | Register custom pings generated from `pings.yaml`. | [Custom pings][custom-pings] |
 | `setExperimentActive` | Indicate that an experiment is running. | [Using the Experiments API][experiments-api] |
 | `setExperimentInactive` | Indicate that an experiment is no longer running.. | [Using the Experiments API][experiments-api] |

glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt

@@ -57,6 +57,10 @@ open class GleanInternalAPI internal constructor () {
     }
 
     private var initialized: Boolean = false
+    // Set when `initialize()` returns.
+    // This allows to detect calls that happen before `Glean.initialize()` was called.
+    // Note: The initialization might still be in progress, as it runs in a separate thread.
+    private var initFinished: Boolean = false
 
     internal lateinit var configuration: Configuration
 
@@ -72,9 +76,6 @@ open class GleanInternalAPI internal constructor () {
 
     private lateinit var gleanDataDir: File
 
-    // Keep track of this flag before Glean is initialized
-    private var uploadEnabled: Boolean = true
-
     // Keep track of this value before Glean is initialized
     private var debugViewTag: String? = null
 
@@ -152,10 +153,6 @@ open class GleanInternalAPI internal constructor () {
         this.httpClient = BaseUploader(configuration.httpClient)
         this.gleanDataDir = File(applicationContext.applicationInfo.dataDir, GLEAN_DATA_DIR)
 
-        // We know we're not initialized, so we can skip the check inside `setUploadEnabled`
-        // by setting the variable directly.
-        this.uploadEnabled = uploadEnabled
-
         // Execute startup off the main thread.
         @Suppress("EXPERIMENTAL_API_USAGE")
         Dispatchers.API.executeTask {
@@ -246,14 +243,6 @@ open class GleanInternalAPI internal constructor () {
                 initializeCoreMetrics(applicationContext)
             }
 
-            // Upload might have been changed in between the call to `initialize`
-            // and this task actually running.
-            // This actually enqueues a task, which will execute after other user-submitted tasks
-            // as part of the queue flush below.
-            if (this@GleanInternalAPI.uploadEnabled != uploadEnabled) {
-                setUploadEnabled(this@GleanInternalAPI.uploadEnabled)
-            }
-
             // Signal Dispatcher that init is complete
             Dispatchers.API.flushQueuedInitialTasks()
 
@@ -264,6 +253,7 @@ open class GleanInternalAPI internal constructor () {
                 ProcessLifecycleOwner.get().lifecycle.addObserver(gleanLifecycleObserver)
             }
         }
+        this.initFinished = true
     }
 
     /**
@@ -300,46 +290,75 @@ open class GleanInternalAPI internal constructor () {
      * @param enabled When true, enable metric collection.
      */
     fun setUploadEnabled(enabled: Boolean) {
-        if (isInitialized()) {
-            val originalEnabled = getUploadEnabled()
-
-            @Suppress("EXPERIMENTAL_API_USAGE")
-            Dispatchers.API.launch {
-                LibGleanFFI.INSTANCE.glean_set_upload_enabled(enabled.toByte())
-
-                if (!enabled) {
-                    // Cancel any pending workers here so that we don't accidentally upload or
-                    // collect data after the upload has been disabled.
-                    metricsPingScheduler.cancel()
-                    // Cancel any pending workers here so that we don't accidentally upload
-                    // data after the upload has been disabled.
-                    PingUploadWorker.cancel(applicationContext)
-                }
+        if (!this.initFinished) {
+            val msg = """
+            Changing upload enabled before Glean is initialized is not supported.
+            Pass the correct state into `Glean.initialize()`.
+            See documentation at https://mozilla.github.io/glean/book/user/general-api.html#initializing-the-glean-sdk
+            """.trimIndent()
+            Log.e(LOG_TAG, msg)
+            return
+        }
+        // Changing upload enabled always happens asynchronous.
+        // That way it follows what a user expect when calling it inbetween other calls:
+        // It executes in the right order.
+        //
+        // Because the dispatch queue is halted until Glean is fully initialized
+        // we can safely enqueue here and it will execute after initialization.
+        @Suppress("EXPERIMENTAL_API_USAGE")
+        Dispatchers.API.launch {
+            val originalEnabled = internalGetUploadEnabled()
+            LibGleanFFI.INSTANCE.glean_set_upload_enabled(enabled.toByte())
+
+            if (!enabled) {
+                // Cancel any pending workers here so that we don't accidentally upload or
+                // collect data after the upload has been disabled.
+                metricsPingScheduler.cancel()
+                // Cancel any pending workers here so that we don't accidentally upload
+                // data after the upload has been disabled.
+                PingUploadWorker.cancel(applicationContext)
+            }
 
-                if (!originalEnabled && enabled) {
-                    // If uploading is being re-enabled, we have to restore the
-                    // application-lifetime metrics.
-                    initializeCoreMetrics((this@GleanInternalAPI).applicationContext)
-                }
+            if (!originalEnabled && enabled) {
+                // If uploading is being re-enabled, we have to restore the
+                // application-lifetime metrics.
+                initializeCoreMetrics((this@GleanInternalAPI).applicationContext)
+            }
 
-                if (originalEnabled && !enabled) {
-                    // If uploading is disabled, we need to send the deletion-request ping
-                    PingUploadWorker.enqueueWorker(applicationContext)
-                }
+            if (originalEnabled && !enabled) {
+                // If uploading is disabled, we need to send the deletion-request ping
+                PingUploadWorker.enqueueWorker(applicationContext)
             }
-        } else {
-            uploadEnabled = enabled
         }
     }
 
     /**
      * Get whether or not Glean is allowed to record and upload data.
+     *
+     * Caution: the result is only correct if Glean is already initialized.
+     *
+     * **THIS METHOD IS DEPRECATED.**
+     * Applications should not rely on Glean's internal state.
+     * Upload enabled status should be tracked by the application and communicated to Glean if it changes.
      */
+    @Deprecated("Upload enabled should be tracked by the application and communicated to Glean if it changes")
     fun getUploadEnabled(): Boolean {
+        return internalGetUploadEnabled()
+    }
+
+    /**
+     * Get whether or not Glean is allowed to record and upload data.
+     *
+     * Caution: the result is only correct if Glean is already initialized.
+     *
+     * Note: due to the deprecation notice and because warnings break the build,
+     * we pull out the implementation into an internal method.
+     */
+    internal fun internalGetUploadEnabled(): Boolean {
         if (isInitialized()) {
             return LibGleanFFI.INSTANCE.glean_is_upload_enabled().toBoolean()
         } else {
-            return uploadEnabled
+            return false
         }
     }
 
@@ -605,7 +624,7 @@ open class GleanInternalAPI internal constructor () {
             return
         }
 
-        if (!getUploadEnabled()) {
+        if (!internalGetUploadEnabled()) {
             Log.e(LOG_TAG, "Glean disabled: not submitting any pings.")
             return
         }
@@ -757,6 +776,11 @@ open class GleanInternalAPI internal constructor () {
         }
 
         LibGleanFFI.INSTANCE.glean_destroy_glean()
+
+        // Reset all state.
+        @Suppress("EXPERIMENTAL_API_USAGE")
+        Dispatchers.API.setTaskQueueing(true)
+        initFinished = false
         initialized = false
     }
 

glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt

@@ -48,6 +48,7 @@ import org.junit.runner.RunWith
 import org.mockito.Mockito.mock
 import org.mockito.Mockito.spy
 import org.robolectric.shadows.ShadowProcess
+import org.robolectric.shadows.ShadowLog
 import java.io.File
 import java.util.Calendar
 import java.util.Locale
@@ -67,20 +68,6 @@ class GleanTest {
         Glean.setUploadEnabled(true)
     }
 
-    @Test
-    fun `getting uploadEnabled before initialization should not crash`() {
-        // Can't use resetGlean directly
-        Glean.testDestroyGleanHandle()
-
-        val config = Configuration()
-
-        Glean.setUploadEnabled(true)
-        assertTrue(Glean.getUploadEnabled())
-
-        Glean.initialize(context, true, config)
-        assertTrue(Glean.getUploadEnabled())
-    }
-
     // New from glean-core.
     @Test
     fun `send a ping`() {
@@ -131,7 +118,7 @@ class GleanTest {
                 sendInPings = listOf("store1")
         )
         Glean.setUploadEnabled(false)
-        assertEquals(false, Glean.getUploadEnabled())
+
         stringMetric.set("foo")
         assertFalse(stringMetric.testHasValue())
     }
@@ -831,4 +818,73 @@ class GleanTest {
         val request = server.takeRequest(20L, TimeUnit.SECONDS)
         assertEquals(request.getHeader("X-Debug-ID"), "valid-tag")
     }
+
+    @Test
+    fun `flipping upload enabled respects order of events`() {
+        // NOTES(janerik):
+        // I'm reasonably sure this test is excercising the right code paths
+        // and from the log output it does the right thing:
+        //
+        // * It fully initializes with the assumption uploadEnabled=true
+        // * It then disables upload
+        // * Then it submits the custom ping, which rightfully is ignored because uploadEnabled=false.
+        //
+        // The test passes.
+        // But it also does that for the old code and I think it's because of some weird WorkManager behaviour,
+        // where it doesn't actually start the work (= the upload).
+
+        // Redirecting log output, usually done by resetGlean, which we don't use here.
+        ShadowLog.stream = System.out
+        // This test relies on Glean not being initialized, we do that ourselves.
+        Glean.testDestroyGleanHandle()
+
+        // This test relies on testing mode to be disabled, since we need to prove the
+        // real-world async behaviour of this.
+        // We don't need to care about clearing it,
+        // the test-unit hooks will call `resetGlean` anyway.
+        Dispatchers.API.setTaskQueueing(true)
+        Dispatchers.API.setTestingMode(false)
+
+        // We create a ping and a metric before we initialize Glean
+        val pingName = "sample_ping_1"
+        val ping = PingType<NoReasonCodes>(
+            name = pingName,
+            includeClientId = true,
+            sendIfEmpty = false,
+            reasonCodes = listOf()
+        )
+        val stringMetric = StringMetricType(
+            disabled = false,
+            category = "telemetry",
+            lifetime = Lifetime.Ping,
+            name = "string_metric",
+            sendInPings = listOf(pingName)
+        )
+
+        val server = getMockWebServer()
+        val context = getContextWithMockedInfo()
+        val config = Glean.configuration.copy(
+            serverEndpoint = "http://" + server.hostName + ":" + server.port
+        )
+        Glean.initialize(context, true, config)
+
+        // Glean might still be initializing. Disable upload.
+        Glean.setUploadEnabled(false)
+
+        // Set data and try to submit a custom ping.
+        val testValue = "SomeTestValue"
+        stringMetric.set(testValue)
+        ping.submit()
+
+        // Trigger worker task to upload any submitted ping.
+        // We need to wait for the work to be enqueued first,
+        // since this test runs asynchronously.
+        waitForEnqueuedWorker(context, PingUploadWorker.PING_WORKER_TAG)
+        triggerWorkManager(context)
+
+        // Validate the received data.
+        val request = server.takeRequest(20L, TimeUnit.SECONDS)
+        val docType = request.path.split("/")[3]
+        assertEquals("deletion-request", docType)
+    }
 }

glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt

@@ -73,7 +73,7 @@ internal fun checkPingSchema(content: JSONObject) {
     }
 
     val exitCode = process.waitFor()
-    assert(exitCode == 0)
+    Assert.assertEquals("glean_parser check failed with exit code $exitCode", 0, exitCode)
 }
 
 /**

glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt

@@ -209,13 +209,11 @@ class EventMetricTypeTest {
             sendInPings = listOf("store1"),
             allowedExtraKeys = listOf("test_name")
         )
-        assertEquals(true, Glean.getUploadEnabled())
         Glean.setUploadEnabled(true)
         eventMetric.record(mapOf(testNameKeys.testName to "event1"))
         val snapshot1 = eventMetric.testGetValue()
         assertEquals(1, snapshot1.size)
         Glean.setUploadEnabled(false)
-        assertEquals(false, Glean.getUploadEnabled())
         eventMetric.record(mapOf(testNameKeys.testName to "event2"))
         @Suppress("EmptyCatchBlock")
         try {