Ignore dynamically stored labels if Glean is not initialized

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

@@ -0,0 +1,92 @@
+/* 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 mozilla.telemetry.glean.private
+
+import android.content.Context
+import androidx.test.core.app.ApplicationProvider
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.work.testing.WorkManagerTestInitHelper
+import mozilla.telemetry.glean.Glean
+import mozilla.telemetry.glean.Dispatchers
+import mozilla.telemetry.glean.config.Configuration
+import org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Before
+import org.junit.Ignore
+import org.junit.Test
+import org.junit.runner.RunWith
+
+/**
+ * Note that this test file MUST NOT use the `GleanTestRule` as it requires metric
+ * accumulation to happen before Glean is initialized.
+ **/
+
+@RunWith(AndroidJUnit4::class)
+class AccumulationsBeforeGleanInitTest {
+
+    val context: Context
+        get() = ApplicationProvider.getApplicationContext()
+
+    @After
+    @Before
+    fun cleanup() {
+        Glean.testDestroyGleanHandle()
+        @Suppress("EXPERIMENTAL_API_USAGE")
+        Dispatchers.API.setTaskQueueing(true)
+        WorkManagerTestInitHelper.initializeTestWorkManager(context)
+    }
+
+    fun forceInitGlean() {
+        Glean.enableTestingMode()
+        Glean.setUploadEnabled(true)
+        Glean.initialize(context, Configuration())
+    }
+
+    @Test
+    fun `LabeledMetricTypes must allow accumulation before Glean inits`() {
+        val counterMetric = CounterMetricType(
+            disabled = false,
+            category = "test.telemetry",
+            lifetime = Lifetime.Application,
+            name = "pre_init_counter",
+            sendInPings = listOf("metrics")
+        )
+
+        val labeledCounterMetric = LabeledMetricType(
+            disabled = false,
+            category = "test.telemetry",
+            lifetime = Lifetime.Application,
+            name = "pre_init_counter",
+            sendInPings = listOf("metrics"),
+            subMetric = counterMetric
+        )
+
+        labeledCounterMetric["label1"].add(1)
+
+        forceInitGlean()
+
+        assertEquals(1, labeledCounterMetric["label1"].testGetValue())
+    }
+
+    @Ignore("FIXME(bug 1588452): Timing distributions currently depend on a valid Glean instance to start")
+    @Test
+    fun `TimingDistributionMetricType must allow accumulation before Glean inits`() {
+        val timingDistribution = TimingDistributionMetricType(
+            disabled = false,
+            category = "test.telemetry",
+            lifetime = Lifetime.Application,
+            name = "pre_init_counter",
+            sendInPings = listOf("metrics")
+        )
+
+        val id = timingDistribution.start()
+        timingDistribution.stopAndAccumulate(id)
+
+        forceInitGlean()
+
+        assertTrue(timingDistribution.testHasValue())
+    }
+}

glean-core/ffi/src/labeled.rs

@@ -76,12 +76,22 @@ macro_rules! impl_labeled_metric {
         /// Create a new instance of the sub-metric of this labeled metric.
         #[no_mangle]
         pub extern "C" fn $get_name(glean_handle: u64, handle: u64, label: FfiStr) -> u64 {
-            GLEAN.call_infallible(glean_handle, |glean| {
+            // If Glean is not yet initialized on the platform side (no handle available),
+            // let's not crash, but let the core handle it by ignoring previously stored dynamic
+            // labels.
+            if glean_handle == 0 {
                 $global.call_infallible_mut(handle, |labeled| {
-                    let metric = labeled.get(glean, label.as_str());
+                    let metric = labeled.get(None, label.as_str());
                     $metric_global.insert_with_log(|| Ok(metric))
                 })
-            })
+            } else {
+                GLEAN.call_infallible(glean_handle, |glean| {
+                    $global.call_infallible_mut(handle, |labeled| {
+                        let metric = labeled.get(Some(glean), label.as_str());
+                        $metric_global.insert_with_log(|| Ok(metric))
+                    })
+                })
+            }
         }
     };
 }

glean-core/src/metrics/labeled.rs

@@ -38,6 +38,30 @@ lazy_static! {
     static ref LABEL_REGEX: Regex = Regex::new("^[a-z_][a-z0-9_-]{0,29}(\\.[a-z0-9_-]{0,29})*$").unwrap();
 }
 
+/// Validate a label against the restrictions.
+///
+/// * Shorter than the allowed `MAX_LABEL_LENGTH`.
+/// * Matches the label regex (only alphanumeric characters and dots, underscores, hyphens)
+///
+/// Returns an error message to be reported through Glean.
+fn validate_label(label: &str) -> Result<&str, String> {
+    if label.len() > MAX_LABEL_LENGTH {
+        let msg = format!(
+            "label length {} exceeds maximum of {}",
+            label.len(),
+            MAX_LABEL_LENGTH
+        );
+        return Err(msg);
+    }
+
+    if !LABEL_REGEX.is_match(label) {
+        let msg = format!("label must be snake_case, got '{}'", label);
+        return Err(msg);
+    }
+
+    Ok(label)
+}
+
 /// A labeled metric.
 ///
 /// Labeled metrics allow to record multiple sub-metrics of the same type under different string labels.
@@ -131,24 +155,7 @@ where
             if self.seen_labels.len() >= MAX_LABELS {
                 return OTHER_LABEL;
             } else {
-                if label.len() > MAX_LABEL_LENGTH {
-                    let msg = format!(
-                        "label length {} exceeds maximum of {}",
-                        label.len(),
-                        MAX_LABEL_LENGTH
-                    );
-                    record_error(
-                        glean,
-                        &self.submetric.meta(),
-                        ErrorType::InvalidLabel,
-                        msg,
-                        None,
-                    );
-                    return OTHER_LABEL;
-                }
-
-                if !LABEL_REGEX.is_match(label) {
-                    let msg = format!("label must be snake_case, got '{}'", label);
+                if let Err(msg) = validate_label(label) {
                     record_error(
                         glean,
                         &self.submetric.meta(),
@@ -177,10 +184,29 @@ where
     ///
     /// Labels must be `snake_case` and less than 30 characters.
     /// If an invalid label is used, the metric will be recorded in the special `OTHER_LABEL` label.
-    pub fn get(&mut self, glean: &Glean, label: &str) -> T {
-        let label = match self.labels {
-            Some(_) => self.static_label(label),
-            None => self.dynamic_label(glean, label),
+    pub fn get<'a, G: Into<Option<&'a Glean>>>(&mut self, glean: G, label: &str) -> T {
+        // We have 3 scenarios to consider:
+        // * Static labels. No database access needed. We just look at what is in memory.
+        // * Dynamic labels, initialized Glean. We look up in the database all previously stored
+        //   labels in order to keep a maximum of allowed labels.
+        // * Dynamic labels, uninitialized Glean. We ignore potentially previously stored labels
+        //   and just take what the user passed in.
+        //   This potentially allows creating more than `MAX_LABELS` labels, but only before Glean
+        //   is initialized.
+        //   This behavior is not publicly documented and should not be abused/depended upon.
+        // The last case is buggy behavior, tracked in bug 1588451.
+        let glean = glean.into();
+        let label = match (&self.labels, glean) {
+            (Some(_), _) => self.static_label(label),
+            (None, Some(glean)) => self.dynamic_label(glean, label),
+            (None, None) => {
+                if let Ok(label) = validate_label(label) {
+                    label
+                } else {
+                    // We can't report any error, as we don't have a Glean instance.
+                    OTHER_LABEL
+                }
+            }
         };
         let label = format!("{}/{}", self.submetric.meta().name, label);