From 546011c603f6356d31a413c7362dade935c35b6d Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 24 May 2024 10:37:19 +0200 Subject: [PATCH] bug 1898336 - Stop sending {memory_timing}_distribution payloads with buckets with 0 counts/ **BREAKING CHANGE**: This changes the ping payload for memory/timing distributions. --- CHANGELOG.md | 1 + glean-core/src/histogram/functional.rs | 40 +------------------ glean-core/src/metrics/memory_distribution.rs | 4 +- glean-core/src/metrics/timing_distribution.rs | 8 +--- glean-core/tests/labeled.rs | 4 +- glean-core/tests/memory_distribution.rs | 2 - glean-core/tests/timing_distribution.rs | 2 - 7 files changed, 9 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17667f14a9..6de8152893 100644 --- 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 index b55caab447..a8ee11b813 100644 --- a/glean-core/src/histogram/functional.rs +++ b/glean-core/src/histogram/functional.rs @@ -85,44 +85,8 @@ impl Histogram { /// **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 { - 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 { + &self.values } } diff --git a/glean-core/src/metrics/memory_distribution.rs b/glean-core/src/metrics/memory_distribution.rs index 5f80b98959..ef48d21e26 100644 --- 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) -> 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 index 3d969a1cf4..71b1b17e6e 100644 --- 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) -> 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 index cf848728fb..c506a1aa7a 100644 --- 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 index 83ca84ddd5..e81e7ecdbb 100644 --- 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 index 78dd53ad8b..2e87f3f5a9 100644 --- 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 {