Bug 1638877: Fix schema validation errors. (#996)

* Bug 1638877: Fix schema validation

This fixes two separate issues that could lead to schema validation failures:

1. Don't send a dirty startup ping if the previous run of the application didn't
   successfully and completely initialize Glean.

2. Always set values for app_build and app_display_version even if there is
   an exception fetching them from the OS PackageManager, so that the pings
   aren't rejected by the schema.

* Move to Android section

* Add docs about "inaccessible"

* Add more docs

* Improve docs

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 * General
     * Add rate limiting capabilities to the upload manager. ([1543612](https://bugzilla.mozilla.org/show_bug.cgi?id=1543612))
+* Android
+    * BUGFIX: baseline pings with reason "dirty startup" are no longer sent if Glean did not full initialize in the previous run.
 
 [Full changelog](https://github.com/mozilla/glean/compare/v31.1.2...main)
 

docs/user/pings/index.md

@@ -42,9 +42,9 @@ Optional fields are marked accordingly.
 
 | Field name | Type | Description |
 |---|---|---|
-| `app_build` | String | The build identifier generated by the CI system (e.g. "1234/A") |
+| `app_build` | String | The build identifier generated by the CI system (e.g. "1234/A"). In the unlikely event this value can not be obtained from the OS, it is set to "inaccessible". |
 | `app_channel` | String | *Optional* The product-provided release channel (e.g. "beta") |
-| `app_display_version` | String | The user-visible version string (e.g. "1.0.3") |
+| `app_display_version` | String | The user-visible version string (e.g. "1.0.3"). In the unlikely event this value can not be obtained from the OS, it is set to "inaccessible". If it is accessible, but not set by the application, it is set to "Unknown". |
 | `architecture` | String | The architecture of the device (e.g. "arm", "x86") |
 | `client_id` | UUID |  *Optional* A UUID identifying a profile and allowing user-oriented correlation of data |
 | `device_manufacturer` | String | *Optional* The manufacturer of the device |

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

@@ -7,6 +7,7 @@ package mozilla.telemetry.glean
 import android.app.ActivityManager
 import android.util.Log
 import android.content.Context
+import android.content.pm.PackageInfo
 import android.content.pm.PackageManager
 import android.os.Build
 import android.os.Process
@@ -164,6 +165,14 @@ open class GleanInternalAPI internal constructor () {
                 return@executeTask
             }
 
+            // Get the current value of the dirty flag so we know whether to
+            // send a dirty startup baseline ping below.  Immediately set it to
+            // `false` so that dirty startup pings won't be sent if Glean
+            // initialization does not complete successfully. It is set to `true`
+            // again in the ON_START lifecycle event.
+            val isDirtyFlagSet = LibGleanFFI.INSTANCE.glean_is_dirty_flag_set().toBoolean()
+            LibGleanFFI.INSTANCE.glean_set_dirty_flag(false.toByte())
+
             // If any pings were registered before initializing, do so now.
             // We're not clearing this queue in case Glean is reset by tests.
             synchronized(this@GleanInternalAPI) {
@@ -197,11 +206,8 @@ open class GleanInternalAPI internal constructor () {
             // Check if the "dirty flag" is set. That means the product was probably
             // force-closed. If that's the case, submit a 'baseline' ping with the
             // reason "dirty_startup". We only do that from the second run.
-            if (!isFirstRun && LibGleanFFI.INSTANCE.glean_is_dirty_flag_set().toBoolean()) {
+            if (!isFirstRun && isDirtyFlagSet) {
                 submitPingByNameSync("baseline", "dirty_startup")
-                // Note: while in theory we should set the "dirty flag" to true
-                // here, in practice it's not needed: if it hits this branch, it
-                // means the value was `true` and nothing needs to be done.
             }
 
             // From the second time we run, after all startup pings are generated,
@@ -444,23 +450,30 @@ open class GleanInternalAPI internal constructor () {
             GleanInternalMetrics.appChannel.setSync(it)
         }
 
-        try {
-            val packageInfo = applicationContext.packageManager.getPackageInfo(
-                    applicationContext.packageName, 0
-            )
-            @Suppress("DEPRECATION")
-            GleanInternalMetrics.appBuild.setSync(packageInfo.versionCode.toString())
+        val packageInfo: PackageInfo
 
-            GleanInternalMetrics.appDisplayVersion.setSync(
-                    packageInfo.versionName?.let { it } ?: "Unknown"
+        try {
+            packageInfo = applicationContext.packageManager.getPackageInfo(
+                    applicationContext.packageName, 0
             )
         } catch (e: PackageManager.NameNotFoundException) {
             Log.e(
                 LOG_TAG,
                 "Could not get own package info, unable to report build id and display version"
             )
-            throw AssertionError("Could not get own package info, aborting init")
+
+            GleanInternalMetrics.appBuild.setSync("inaccessible")
+            GleanInternalMetrics.appDisplayVersion.setSync("inaccessible")
+
+            return
         }
+
+        @Suppress("DEPRECATION")
+        GleanInternalMetrics.appBuild.setSync(packageInfo.versionCode.toString())
+
+        GleanInternalMetrics.appDisplayVersion.setSync(
+            packageInfo.versionName?.let { it } ?: "Unknown"
+        )
     }
 
     /**

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

@@ -798,4 +798,16 @@ class GleanTest {
             Glean.pingTypeQueue.add(it)
         }
     }
+
+    @Test
+    fun `test dirty flag is reset to false`() {
+        // Set the dirty flag.
+        LibGleanFFI.INSTANCE.glean_set_dirty_flag(true.toByte())
+
+        resetGlean(context, Glean.configuration.copy(
+            logPings = true
+        ), false)
+
+        assertFalse(LibGleanFFI.INSTANCE.glean_is_dirty_flag_set().toBoolean())
+    }
 }

glean-core/metrics.yaml

@@ -149,6 +149,9 @@ glean.internal.metrics:
       - glean_client_info
     description: |
       The build identifier generated by the CI system (e.g. "1234/A").
+
+      In the unlikely event that the build identifier can not be retrieved from
+      the OS, it is set to "inaccessible".
     bugs:
       - https://bugzilla.mozilla.org/1508305
     data_reviews:
@@ -164,6 +167,12 @@ glean.internal.metrics:
       - glean_client_info
     description: |
       The user visible version string (e.g. "1.0.3").
+
+      In the unlikely event that the build identifier can not be retrieved from
+      the OS, it is set to "inaccessible".
+
+      If it is retrievable but no value has been set by the application, it is
+      set to "Unknown".
     bugs:
       - https://bugzilla.mozilla.org/1508305
     data_reviews: