Bug 1844533 - Fix Glean ping uploading from background process

When Glean attempted to schedule an upload task on the WorkManager from a background process
it would get the main-process Glean when the task was executed. Since the main-process Glean
didn't have the data directory from the background process, it would not find the ping to be
able to upload it. This changes the uploader to run on the internal Glean Dispatcher when it
is being run from a background service or process and continues to rely on WorkManager for
the main process execution.

.dictionary

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 267 utf-8
+personal_ws-1.1 en 268 utf-8
 AAR
 AARs
 ABI
@@ -98,6 +98,7 @@ VPN
 Wasm
 WebRender
 Webpack
+WorkManager
 XCFramework
 Xcode
 YAML

CHANGELOG.md

@@ -10,6 +10,8 @@
   * `locale` now exposed through the RLB so it can be set by consumers ([2527](https://github.com/mozilla/glean/pull/2527))
 * Python
   * Added the shutdown API for Python to ensure orderly shutdown and waiting for uploader processes ([#2538](https://github.com/mozilla/glean/pull/2538))
+* Kotlin
+  * Move running of upload task when Glean is running in a background service to use the internal Glean Dispatchers rather than WorkManager. [Bug 1844533](https://bugzilla.mozilla.org/show_bug.cgi?id=1844533)
 
 # v53.1.0 (2023-06-28)
 

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

@@ -23,6 +23,7 @@ import mozilla.telemetry.glean.net.BaseUploader
 import mozilla.telemetry.glean.scheduler.GleanLifecycleObserver
 import mozilla.telemetry.glean.scheduler.MetricsPingScheduler
 import mozilla.telemetry.glean.scheduler.PingUploadWorker
+import mozilla.telemetry.glean.scheduler.PingUploadWorker.Companion.performUpload
 import mozilla.telemetry.glean.utils.ThreadUtils
 import mozilla.telemetry.glean.utils.calendarToDatetime
 import mozilla.telemetry.glean.utils.canWriteToDatabasePath
@@ -59,7 +60,16 @@ internal class OnGleanEventsImpl(val glean: GleanInternalAPI) : OnGleanEvents {
     }
 
     override fun triggerUpload() {
-        PingUploadWorker.enqueueWorker(glean.applicationContext)
+        if (!glean.isCustomDataPath) {
+            PingUploadWorker.enqueueWorker(glean.applicationContext)
+        } else {
+            // WorkManager wants to run on the main thread/process typically, so when Glean is
+            // running in a background process we will instead just use the internal Glean
+            // coroutine dispatcher to run the upload task.
+            Dispatchers.API.executeTask {
+                performUpload()
+            }
+        }
     }
 
     override fun startMetricsPingScheduler(): Boolean {

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

@@ -73,6 +73,47 @@ class PingUploadWorker(context: Context, params: WorkerParameters) : Worker(cont
             }
         }
 
+        /**
+         * Perform the upload task synchronously.
+         *
+         * This will be called from the [doWork] function of the [PingUploadWorker] when Glean is
+         * being run from the main process of an application, but for background services it will
+         * be called from the Glean.Dispatchers couroutine scope to avoid WorkManager complexity
+         * for multi-process applications. See Bug1844533 for more information.
+         *
+         * @param context the application [Context]
+         */
+        @OptIn(ExperimentalUnsignedTypes::class)
+        internal fun performUpload() {
+            do {
+                when (val action = gleanGetUploadTask()) {
+                    is PingUploadTask.Upload -> {
+                        // Upload the ping request.
+                        // If the status is `null` there was some kind of unrecoverable error
+                        // so we return a known unrecoverable error status code
+                        // which will ensure this gets treated as such.
+                        val body = action.request.body.toUByteArray().asByteArray()
+                        val result = Glean.httpClient.doUpload(
+                            action.request.path,
+                            body,
+                            action.request.headers,
+                            Glean.configuration,
+                        )
+
+                        // Process the upload response
+                        when (gleanProcessPingUploadResponse(action.request.documentId, result)) {
+                            UploadTaskAction.NEXT -> continue
+                            UploadTaskAction.END -> break
+                        }
+                    }
+                    is PingUploadTask.Wait -> SystemClock.sleep(action.time.toLong())
+                    is PingUploadTask.Done -> break
+                }
+            } while (true)
+            // Limits are enforced by glean-core to avoid an inifinite loop here.
+            // Whenever a limit is reached, this binding will receive `PingUploadTask.Done` and step out.
+        }
+
         /**
          * Function to cancel any pending ping upload workers
          *
@@ -95,38 +136,8 @@ class PingUploadWorker(context: Context, params: WorkerParameters) : Worker(cont
      *
      * @return The [androidx.work.ListenableWorker.Result] of the computation
      */
-    @OptIn(ExperimentalUnsignedTypes::class)
-    @Suppress("ReturnCount")
     override fun doWork(): Result {
-        do {
-            when (val action = gleanGetUploadTask()) {
-                is PingUploadTask.Upload -> {
-                    // Upload the ping request.
-                    // If the status is `null` there was some kind of unrecoverable error
-                    // so we return a known unrecoverable error status code
-                    // which will ensure this gets treated as such.
-                    val body = action.request.body.toUByteArray().asByteArray()
-                    val result = Glean.httpClient.doUpload(
-                        action.request.path,
-                        body,
-                        action.request.headers,
-                        Glean.configuration,
-                    )
-
-                    // Process the upload response
-                    val nextAction = gleanProcessPingUploadResponse(action.request.documentId, result)
-                    when (nextAction) {
-                        UploadTaskAction.NEXT -> continue
-                        UploadTaskAction.END -> break
-                    }
-                }
-                is PingUploadTask.Wait -> SystemClock.sleep(action.time.toLong())
-                is PingUploadTask.Done -> break
-            }
-        } while (true)
-        // Limits are enforced by glean-core to avoid an inifinite loop here.
-        // Whenever a limit is reached, this binding will receive `PingUploadTask.Done` and step out.
-
+        performUpload()
         return Result.success()
     }
 }

samples/android/app/build.gradle

@@ -41,7 +41,6 @@ dependencies {
 
     implementation "androidx.appcompat:appcompat:$rootProject.versions.androidx_appcompat"
     implementation "androidx.browser:browser:$rootProject.versions.androidx_browser"
-    implementation "androidx.work:work-runtime-ktx:$rootProject.versions.androidx_browser"
 
 
     androidTestImplementation "androidx.test:core-ktx:$rootProject.versions.androidx_test"

samples/android/app/src/androidTest/java/org/mozilla/samples/gleancore/pings/BackgroundPingTest.kt

@@ -0,0 +1,64 @@
+/* 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.samples.gleancore.pings
+
+import android.content.Context
+import android.content.Intent
+import androidx.test.core.app.ApplicationProvider
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.test.rule.ServiceTestRule
+import mozilla.telemetry.glean.testing.GleanTestLocalServer
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertTrue
+import org.junit.Ignore
+import org.junit.Rule
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.mozilla.samples.gleancore.SampleBackgroundProcess
+
+// Note that this is ignored because for some reason the mock server
+// doesn't seem to be able to catch the background ping when it is
+// sent, despite the logs clearly stating that the ping is being sent
+// and from testing with the Glean Debug View also clearly showing
+// that the ping is being sent. See Bug 1846518 filed as a follow-up
+// to this to try and fix this.
+@RunWith(AndroidJUnit4::class)
+@Ignore
+class BackgroundPingTest {
+    private val server = createMockWebServer()
+
+    @get:Rule
+    val serviceRule: ServiceTestRule = ServiceTestRule()
+
+    @get:Rule
+    val gleanRule = GleanTestLocalServer(context, server.port)
+
+    private val context: Context
+        get() = ApplicationProvider.getApplicationContext()
+
+    @Test
+    fun validateBackgroundPing() {
+        val serviceIntent = Intent(
+            context,
+            SampleBackgroundProcess::class.java,
+        )
+        serviceRule.startService(serviceIntent)
+
+        val backgroundPing = waitForPingContent("background", "started", server)
+        assertNotNull(backgroundPing)
+
+        val metrics = backgroundPing?.getJSONObject("metrics")
+
+        val counters = metrics?.getJSONObject("counter")
+        assertTrue(counters?.getJSONObject("custom.bg_counter")?.getLong("value") == 1L)
+
+        // Make sure there's no errors.
+        val errors = metrics?.optJSONObject("labeled_counter")?.keys()
+        errors?.forEach {
+            assertFalse(it.startsWith("glean.error."))
+        }
+    }
+}

samples/android/app/src/main/AndroidManifest.xml

@@ -53,22 +53,5 @@
         -->
         <service android:name=".SampleBackgroundProcess"
             android:process=":sampleBackgroundProcess" />
-
-        <!--
-          Disable default WorkManager initialization so we can use a custom
-          provider in order to test background service operation
-        -->
-        <provider
-            android:name="androidx.startup.InitializationProvider"
-            android:authorities="${applicationId}.androidx-startup"
-            android:exported="false"
-            tools:node="merge">
-            <!-- If you are using androidx.startup to initialize other components -->
-            <meta-data
-                android:name="androidx.work.WorkManagerInitializer"
-                android:value="androidx.startup"
-                tools:node="remove" />
-        </provider>
-
     </application>
 </manifest>

samples/android/app/src/main/java/org/mozilla/samples/gleancore/GleanApplication.kt

@@ -6,7 +6,6 @@ package org.mozilla.samples.gleancore
 
 import android.app.Application
 import android.util.Log
-import androidx.work.Configuration
 import mozilla.telemetry.glean.Glean
 import org.mozilla.samples.gleancore.GleanMetrics.Basic
 import org.mozilla.samples.gleancore.GleanMetrics.Custom
@@ -18,21 +17,7 @@ import java.util.UUID
 
 private const val TAG = "Glean"
 
-class GleanApplication : Application(), Configuration.Provider {
-
-    /**
-     * Override for providing a default WorkManager configuration for the background service to
-     * use. This sets the default process to the main process, otherwise the background service
-     * WorkManager doesn't function.
-     */
-    override fun getWorkManagerConfiguration(): Configuration {
-        return Configuration.Builder()
-            // This is required for Glean to be able to enqueue the PingUploadWorker
-            // from both the daemon and the main app.
-            .setDefaultProcessName(packageName)
-            .setMinimumLoggingLevel(Log.INFO)
-            .build()
-    }
+class GleanApplication : Application() {
 
     override fun onCreate() {
         super.onCreate()

samples/android/app/src/main/java/org/mozilla/samples/gleancore/MainActivity.kt

@@ -16,16 +16,16 @@ import org.mozilla.samples.gleancore.databinding.ActivityMainBinding
 
 open class MainActivity : AppCompatActivity() {
     private lateinit var binding: ActivityMainBinding
+    private lateinit var serviceIntent: Intent
 
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
         binding = ActivityMainBinding.inflate(layoutInflater)
+        serviceIntent = Intent(this, SampleBackgroundProcess::class.java)
 
         // Start the background service to test recording metrics and sending pings in from off
         // the main process
-        startService(
-            Intent(this, SampleBackgroundProcess::class.java),
-        )
+        startService(serviceIntent)
 
         setContentView(binding.root)
 
@@ -80,4 +80,9 @@ open class MainActivity : AppCompatActivity() {
 
         Test.timespan.stop()
     }
+
+    override fun onDestroy() {
+        stopService(serviceIntent)
+        super.onDestroy()
+    }
 }

samples/android/app/src/main/java/org/mozilla/samples/gleancore/SampleBackgroundProcess.kt

@@ -18,14 +18,12 @@ import java.util.Calendar
  * process. This service records a counter and then submits a ping as it starts.
  */
 class SampleBackgroundProcess : Service() {
-    private var mBinder: Binder = Binder()
-
     /**
      * Required override, don't need to do anything here so we
      * just return a default Binder
      */
-    override fun onBind(p0: Intent?): IBinder? {
-        return mBinder
+    override fun onBind(intent: Intent?): IBinder {
+        return Binder()
     }
 
     /**