bug 1898336 - Stop sending {memory_timing}_distribution payloads with…

… buckets with 0 counts/ **BREAKING CHANGE**: This changes the ping payload for memory/timing distributions.
mozilla · Oct 10, 2024 · 36d1bb1 · 36d1bb1
1 parent c7da149
commit 36d1bb1
Show file tree

Hide file tree

Showing 7 changed files with 9 additions and 52 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 * General
   * **BREAKING**: Remove LMDB-to-safe-mode migration.
     Safe-mode became the default in Glean v51. ([#TODO]())
+  * **BREAKING**: Stop sending buckets with 0 counts in memory_distribution and timing_distribution metric payloads ([bug 1898336](https://bugzilla.mozilla.org/show_bug.cgi?id=1898336))
 
 # v61.2.0 (2024-10-07)
 

diff --git a/glean-core/src/histogram/functional.rs b/glean-core/src/histogram/functional.rs
@@ -85,44 +85,8 @@ impl Histogram<Functional> {
     /// **Caution** This is a more specific implementation of `snapshot_values` on functional
     /// histograms. `snapshot_values` cannot be used with those, due to buckets not being
     /// precomputed.
-    pub fn snapshot(&self) -> HashMap<u64, u64> {
-        if self.values.is_empty() {
-            return HashMap::new();
-        }
-
-        let mut min_key = None;
-        let mut max_key = None;
-
-        // `Iterator#min` and `Iterator#max` would do the same job independently,
-        // but we want to avoid iterating the keys twice, so we loop ourselves.
-        for key in self.values.keys() {
-            let key = *key;
-
-            // safe unwrap, we checked it's not none
-            if min_key.is_none() || key < min_key.unwrap() {
-                min_key = Some(key);
-            }
-
-            // safe unwrap, we checked it's not none
-            if max_key.is_none() || key > max_key.unwrap() {
-                max_key = Some(key);
-            }
-        }
-
-        // Non-empty values, therefore minimum/maximum exists.
-        // safe unwraps, we set it at least once.
-        let min_bucket = self.bucketing.sample_to_bucket_index(min_key.unwrap());
-        let max_bucket = self.bucketing.sample_to_bucket_index(max_key.unwrap()) + 1;
-
-        let mut values = self.values.clone();
-
-        for idx in min_bucket..=max_bucket {
-            // Fill in missing entries.
-            let min_bucket = self.bucketing.bucket_index_to_bucket_minimum(idx);
-            let _ = values.entry(min_bucket).or_insert(0);
-        }
-
-        values
+    pub fn snapshot(&self) -> &HashMap<u64, u64> {
+        &self.values
     }
 }
 

diff --git a/glean-core/src/metrics/memory_distribution.rs b/glean-core/src/metrics/memory_distribution.rs
@@ -42,8 +42,8 @@ pub(crate) fn snapshot(hist: &Histogram<Functional>) -> DistributionData {
         // specialized snapshot function.
         values: hist
             .snapshot()
-            .into_iter()
-            .map(|(k, v)| (k as i64, v as i64))
+            .iter()
+            .map(|(&k, &v)| (k as i64, v as i64))
             .collect(),
         sum: hist.sum() as i64,
         count: hist.count() as i64,

diff --git a/glean-core/src/metrics/timing_distribution.rs b/glean-core/src/metrics/timing_distribution.rs
@@ -74,8 +74,8 @@ pub(crate) fn snapshot(hist: &Histogram<Functional>) -> DistributionData {
         // specialized snapshot function.
         values: hist
             .snapshot()
-            .into_iter()
-            .map(|(k, v)| (k as i64, v as i64))
+            .iter()
+            .map(|(&k, &v)| (k as i64, v as i64))
             .collect(),
         sum: hist.sum() as i64,
         count: hist.count() as i64,
@@ -713,7 +713,6 @@ mod test {
                 "8": 1,
                 "9": 1,
                 "10": 1,
-                "11": 0,
             },
         });
 
@@ -738,10 +737,7 @@ mod test {
             "values": {
                 "1024": 2,
                 "1116": 1,
-                "1217": 0,
-                "1327": 0,
                 "1448": 1,
-                "1579": 0,
             },
         });
 

diff --git a/glean-core/tests/labeled.rs b/glean-core/tests/labeled.rs
@@ -180,7 +180,7 @@ fn can_create_labeled_memory_distribution_metric() {
     assert_eq!(
         json!({
             "labeled_memory_distribution": {
-                "telemetry.labeled_metric": { "label1": { "sum": 42, "values": {"41": 1, "43": 0} } }
+                "telemetry.labeled_metric": { "label1": { "sum": 42, "values": {"41": 1} } }
             }
         }),
         snapshot
@@ -215,7 +215,7 @@ fn can_create_labeled_timing_distribution_metric() {
     assert_eq!(
         json!({
             "labeled_timing_distribution": {
-                "telemetry.labeled_metric": { "label1": { "sum": 42, "values": {"41": 1, "45": 0} } }
+                "telemetry.labeled_metric": { "label1": { "sum": 42, "values": {"41": 1} } }
             }
         }),
         snapshot

diff --git a/glean-core/tests/memory_distribution.rs b/glean-core/tests/memory_distribution.rs
@@ -59,7 +59,6 @@ fn serializer_should_correctly_serialize_memory_distribution() {
             "sum": 100_000 * kb,
             "values": {
                 "99108124": 1,
-                "103496016": 0,
             }
         });
         assert_eq!(
@@ -93,7 +92,6 @@ fn set_value_properly_sets_the_value_in_all_stores() {
         "sum": 100_000,
         "values": {
             "96785": 1,
-            "101070": 0,
         }
     });
     for store_name in store_names {

diff --git a/glean-core/tests/timing_distribution.rs b/glean-core/tests/timing_distribution.rs
@@ -64,7 +64,6 @@ fn serializer_should_correctly_serialize_timing_distribution() {
             "sum": duration,
             "values": {
                 "58": 1,
-                "64": 0,
             }
         });
         assert_eq!(
@@ -102,7 +101,6 @@ fn set_value_properly_sets_the_value_in_all_stores() {
         "sum": 1,
         "values": {
             "1": 1,
-            "2": 0,
         }
     });
     for store_name in store_names {