From a2f380aa3df3234f628ae5788da8201142268007 Mon Sep 17 00:00:00 2001 From: Travis Long Date: Mon, 31 Jul 2023 10:10:19 +0200 Subject: [PATCH] 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 | 3 +- CHANGELOG.md | 2 + .../java/mozilla/telemetry/glean/Glean.kt | 12 ++- .../glean/scheduler/PingUploadWorker.kt | 73 +++++++++++-------- samples/android/app/build.gradle | 1 - .../gleancore/pings/BackgroundPingTest.kt | 64 ++++++++++++++++ .../android/app/src/main/AndroidManifest.xml | 17 ----- .../samples/gleancore/GleanApplication.kt | 17 +---- .../mozilla/samples/gleancore/MainActivity.kt | 11 ++- .../gleancore/SampleBackgroundProcess.kt | 6 +- 10 files changed, 132 insertions(+), 74 deletions(-) create mode 100644 samples/android/app/src/androidTest/java/org/mozilla/samples/gleancore/pings/BackgroundPingTest.kt diff --git a/.dictionary b/.dictionary index f01ed1619..316918ea3 100644 --- a/.dictionary +++ b/.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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ba537867..0786090c3 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt index 94c72bf0e..1637f50e4 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt +++ b/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 { diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/PingUploadWorker.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/PingUploadWorker.kt index e02b87897..eb55fdcd7 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/PingUploadWorker.kt +++ b/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() } } diff --git a/samples/android/app/build.gradle b/samples/android/app/build.gradle index 235d312fd..7099fc7af 100644 --- a/samples/android/app/build.gradle +++ b/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" diff --git a/samples/android/app/src/androidTest/java/org/mozilla/samples/gleancore/pings/BackgroundPingTest.kt b/samples/android/app/src/androidTest/java/org/mozilla/samples/gleancore/pings/BackgroundPingTest.kt new file mode 100644 index 000000000..f80e40690 --- /dev/null +++ b/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.")) + } + } +} diff --git a/samples/android/app/src/main/AndroidManifest.xml b/samples/android/app/src/main/AndroidManifest.xml index 9307f215e..4552a4451 100644 --- a/samples/android/app/src/main/AndroidManifest.xml +++ b/samples/android/app/src/main/AndroidManifest.xml @@ -53,22 +53,5 @@ --> - - - - - - - diff --git a/samples/android/app/src/main/java/org/mozilla/samples/gleancore/GleanApplication.kt b/samples/android/app/src/main/java/org/mozilla/samples/gleancore/GleanApplication.kt index 2b8cfc74b..828cfe59b 100644 --- a/samples/android/app/src/main/java/org/mozilla/samples/gleancore/GleanApplication.kt +++ b/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() diff --git a/samples/android/app/src/main/java/org/mozilla/samples/gleancore/MainActivity.kt b/samples/android/app/src/main/java/org/mozilla/samples/gleancore/MainActivity.kt index b3f32ec7a..99d6883ed 100644 --- a/samples/android/app/src/main/java/org/mozilla/samples/gleancore/MainActivity.kt +++ b/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() + } } diff --git a/samples/android/app/src/main/java/org/mozilla/samples/gleancore/SampleBackgroundProcess.kt b/samples/android/app/src/main/java/org/mozilla/samples/gleancore/SampleBackgroundProcess.kt index 49db6ca9d..b10c69a46 100644 --- a/samples/android/app/src/main/java/org/mozilla/samples/gleancore/SampleBackgroundProcess.kt +++ b/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() } /**