1588452: Don't crash TimingDistribution.start() prior to Glean.init

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 [Full changelog](https://github.com/mozilla/glean/compare/v19.0.0...master)
 
+* Fixed a crash calling `start` on a timing distribution metric before Glean is initialized.
+  Timings are always measured, but only recorded when upload is enabled.
+
 # v19.0.0 (2019-10-22)
 
 [Full changelog](https://github.com/mozilla/glean/compare/v0.0.1-TESTING6...v19.0.0)

docs/user/metrics/timing_distribution.md

@@ -7,6 +7,10 @@ To measure the distribution of single timespans, see [Timespans](timespan.md). T
 Timing distributions are recorded in a histogram where the buckets have an exponential distribution, specifically with 8 buckets for every power of 2.
 This makes them suitable for measuring timings on a number of time scales without any configuration.
 
+Timings always span the full length between `start` and `stopAndAccumulate`.
+If the Glean upload is disabled when calling `start`, the timer is still started.
+If the Glean upload is disabled at the time `stopAndAccumulate` is called, nothing is recorded.
+
 ## Configuration
 
 If you wanted to create a timing distribution to measure page load times, first you need to add an entry for it to the `metrics.yaml` file:

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

@@ -82,9 +82,9 @@ class TimingDistributionMetricType internal constructor(
 
         // No dispatcher, we need the return value
         return LibGleanFFI.INSTANCE.glean_timing_distribution_set_start(
-                Glean.handle,
-                [email protected],
-                startTime)
+            this@TimingDistributionMetricType.handle,
+            startTime
+        )
     }
 
     /**

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

@@ -313,7 +313,7 @@ internal interface LibGleanFFI : Library {
 
     fun glean_destroy_timing_distribution_metric(handle: Long)
 
-    fun glean_timing_distribution_set_start(glean_handle: Long, metric_id: Long, start_time: Long): GleanTimerId
+    fun glean_timing_distribution_set_start(metric_id: Long, start_time: Long): GleanTimerId
 
     fun glean_timing_distribution_set_stop_and_accumulate(
         glean_handle: Long,

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

@@ -4,8 +4,11 @@
 
 package mozilla.telemetry.glean.private
 
+import android.content.Context
 import androidx.test.core.app.ApplicationProvider
 import androidx.test.ext.junit.runners.AndroidJUnit4
+import mozilla.telemetry.glean.Dispatchers
+import mozilla.telemetry.glean.Glean
 import mozilla.telemetry.glean.testing.GleanTestRule
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertTrue
@@ -20,6 +23,9 @@ import org.mockito.Mockito.`when`
 @RunWith(AndroidJUnit4::class)
 class TimingDistributionMetricTypeTest {
 
+    val context: Context
+        get() = ApplicationProvider.getApplicationContext()
+
     @get:Rule
     val gleanRule = GleanTestRule(ApplicationProvider.getApplicationContext())
 
@@ -139,7 +145,7 @@ class TimingDistributionMetricTypeTest {
 
     @Test
     fun `The accumulateSamples API correctly stores timing values`() {
-        // Define a timing distribution metric which will be stored in multiple stores
+        // Define a timing distribution metric
         val metric = TimingDistributionMetricType(
             disabled = false,
             category = "telemetry",
@@ -168,4 +174,48 @@ class TimingDistributionMetricTypeTest {
         assertEquals(1L, snapshot.values[1969251187])
         assertEquals(1L, snapshot.values[2784941737])
     }
+
+    @Test
+    fun `Starting a timer before initialization doesn't crash`() {
+        Glean.testDestroyGleanHandle()
+        @Suppress("EXPERIMENTAL_API_USAGE")
+        Dispatchers.API.setTaskQueueing(true)
+
+        val metric = TimingDistributionMetricType(
+            disabled = false,
+            category = "telemetry",
+            lifetime = Lifetime.Ping,
+            name = "timing_distribution_samples",
+            sendInPings = listOf("store1"),
+            timeUnit = TimeUnit.Second
+        )
+
+        val timerId = metric.start()
+        Glean.initialize(context)
+        metric.stopAndAccumulate(timerId)
+
+        metric.testGetValue().sum >= 0
+    }
+
+    @Test
+    fun `Starting and stopping a timer before initialization doesn't crash`() {
+        Glean.testDestroyGleanHandle()
+        @Suppress("EXPERIMENTAL_API_USAGE")
+        Dispatchers.API.setTaskQueueing(true)
+
+        val metric = TimingDistributionMetricType(
+            disabled = false,
+            category = "telemetry",
+            lifetime = Lifetime.Ping,
+            name = "timing_distribution_samples",
+            sendInPings = listOf("store1"),
+            timeUnit = TimeUnit.Second
+        )
+
+        val timerId = metric.start()
+        metric.stopAndAccumulate(timerId)
+        Glean.initialize(context)
+
+        metric.testGetValue().sum >= 0
+    }
 }

glean-core/ffi/glean.h

@@ -401,8 +401,7 @@ void glean_timing_distribution_accumulate_samples(uint64_t glean_handle,
 
 void glean_timing_distribution_cancel(uint64_t metric_id, TimerId timer_id);
 
-TimerId glean_timing_distribution_set_start(uint64_t glean_handle,
-                                            uint64_t metric_id,
+TimerId glean_timing_distribution_set_start(uint64_t metric_id,
                                             uint64_t start_time);
 
 void glean_timing_distribution_set_stop_and_accumulate(uint64_t glean_handle,

glean-core/ffi/src/timing_distribution.rs

@@ -17,15 +17,9 @@ define_metric!(TimingDistributionMetric => TIMING_DISTRIBUTION_METRICS {
 });
 
 #[no_mangle]
-pub extern "C" fn glean_timing_distribution_set_start(
-    glean_handle: u64,
-    metric_id: u64,
-    start_time: u64,
-) -> TimerId {
-    GLEAN.call_infallible(glean_handle, |glean| {
-        TIMING_DISTRIBUTION_METRICS
-            .call_infallible_mut(metric_id, |metric| metric.set_start(glean, start_time))
-    })
+pub extern "C" fn glean_timing_distribution_set_start(metric_id: u64, start_time: u64) -> TimerId {
+    TIMING_DISTRIBUTION_METRICS
+        .call_infallible_mut(metric_id, |metric| metric.set_start(start_time))
 }
 
 #[no_mangle]

glean-core/ios/Glean/GleanFfi.h

@@ -401,9 +401,7 @@ void glean_timing_distribution_accumulate_samples(uint64_t glean_handle,
 
 void glean_timing_distribution_cancel(uint64_t metric_id, TimerId timer_id);
 
-TimerId glean_timing_distribution_set_start(uint64_t glean_handle,
-                                            uint64_t metric_id,
-                                            uint64_t start_time);
+TimerId glean_timing_distribution_set_start(uint64_t metric_id, uint64_t start_time);
 
 void glean_timing_distribution_set_stop_and_accumulate(uint64_t glean_handle,
                                                        uint64_t metric_id,

glean-core/ios/Glean/Metrics/TimingDistributionMetric.swift

@@ -59,7 +59,7 @@ public class TimingDistributionMetricType {
         let startTime = timestampNanos()
 
         // No dispatcher, we need the return value
-        return glean_timing_distribution_set_start(Glean.shared.handle, self.handle, startTime)
+        return glean_timing_distribution_set_start(self.handle, startTime)
     }
 
     /// Stop tracking time for the provided metric and associated timer id. Add a

glean-core/src/metrics/timing_distribution.rs

@@ -145,11 +145,7 @@ impl TimingDistributionMetric {
     /// ## Return value
     ///
     /// Returns a unique `TimerId` for the new timer.
-    pub fn set_start(&mut self, glean: &Glean, start_time: u64) -> TimerId {
-        if !self.should_record(glean) {
-            return 0;
-        }
-
+    pub fn set_start(&mut self, start_time: u64) -> TimerId {
         self.timings.set_start(start_time)
     }
 

glean-core/tests/timing_distribution.rs

@@ -45,7 +45,7 @@ fn serializer_should_correctly_serialize_timing_distribution() {
             time_unit,
         );
 
-        let id = metric.set_start(&glean, 0);
+        let id = metric.set_start(0);
         metric.set_stop_and_accumulate(&glean, id, duration);
 
         let val = metric
@@ -89,7 +89,7 @@ fn set_value_properly_sets_the_value_in_all_stores() {
         TimeUnit::Nanosecond,
     );
 
-    let id = metric.set_start(&glean, 0);
+    let id = metric.set_start(0);
     metric.set_stop_and_accumulate(&glean, id, duration);
 
     for store_name in store_names {
@@ -129,7 +129,7 @@ fn timing_distributions_must_not_accumulate_negative_values() {
 
     // Flip around the timestamps, this should result in a negative value which should be
     // discarded.
-    let id = metric.set_start(&glean, duration);
+    let id = metric.set_start(duration);
     metric.set_stop_and_accumulate(&glean, id, 0);
 
     assert!(metric.test_get_value(&glean, "store1").is_none());
@@ -256,7 +256,7 @@ fn large_nanoseconds_values() {
     let time = Duration::from_secs(10).as_nanos() as u64;
     assert!(time > u64::from(u32::max_value()));
 
-    let id = metric.set_start(&glean, 0);
+    let id = metric.set_start(0);
     metric.set_stop_and_accumulate(&glean, id, time);
 
     let val = metric