diff --git a/CHANGELOG.md b/CHANGELOG.md index f8bf33ff66..508b47e664 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ [Full changelog](https://github.com/mozilla/glean/compare/v69.0.0...main) +* General + * Accept 0-duration samples in timing_distribution metrics ([bug 2049040](https://bugzilla.mozilla.org/show_bug.cgi?id=2049040)) + # v69.0.0 (2026-06-22) [Full changelog](https://github.com/mozilla/glean/compare/v68.0.0...v69.0.0) diff --git a/docs/user/reference/metrics/timing_distribution.md b/docs/user/reference/metrics/timing_distribution.md index 704de4bffd..46c5728f79 100644 --- a/docs/user/reference/metrics/timing_distribution.md +++ b/docs/user/reference/metrics/timing_distribution.md @@ -28,7 +28,8 @@ controls the minimum and maximum values that will recorded: - `millisecond`: 1ms <= x <= ~19 years Overflowing this range is considered an error and is reported through the error reporting mechanism. -Underflowing this range is not an error and the value is silently truncated to the minimum value. +Underflowing this range is not an error and the value is silently promoted to the minimum value. +Values of zero are preserved as zero. Additionally, when a metric comes from GeckoView (the `geckoview_datapoint` parameter is present), the `time_unit` parameter specifies the unit that the samples are in when passed to Glean. diff --git a/glean-core/src/metrics/timing_distribution.rs b/glean-core/src/metrics/timing_distribution.rs index 5d8dd9c053..7581dce130 100644 --- a/glean-core/src/metrics/timing_distribution.rs +++ b/glean-core/src/metrics/timing_distribution.rs @@ -241,7 +241,7 @@ impl TimingDistributionMetric { let min_sample_time = self.time_unit.as_nanos(1); let max_sample_time = self.time_unit.as_nanos(MAX_SAMPLE_TIME); - duration = if duration < min_sample_time { + duration = if duration < min_sample_time && duration > 0 { // If measurement is less than the minimum, just truncate. This is // not recorded as an error. min_sample_time @@ -389,9 +389,7 @@ impl TimingDistributionMetric { // Check the range prior to converting the incoming unit to // nanoseconds, so we can compare against the constant // MAX_SAMPLE_TIME. - if sample == 0 { - sample = 1; - } else if sample > MAX_SAMPLE_TIME { + if sample > MAX_SAMPLE_TIME { num_too_long_samples += 1; sample = MAX_SAMPLE_TIME; } @@ -502,7 +500,7 @@ impl TimingDistributionMetric { for &sample in samples.iter() { let mut sample = sample; - if sample < min_sample_time { + if sample < min_sample_time && sample > 0 { sample = min_sample_time; } else if sample > max_sample_time { num_too_long_samples += 1; @@ -664,9 +662,7 @@ impl<'a> LocalTimingDistribution<'a> { // Check the range prior to converting the incoming unit to // nanoseconds, so we can compare against the constant // MAX_SAMPLE_TIME. - let sample = if sample == 0 { - 1 - } else if sample > MAX_SAMPLE_TIME { + let sample = if sample > MAX_SAMPLE_TIME { self.errors += 1; MAX_SAMPLE_TIME } else { diff --git a/glean-core/tests/memory_distribution.rs b/glean-core/tests/memory_distribution.rs index e81e7ecdbb..2f1faeb033 100644 --- a/glean-core/tests/memory_distribution.rs +++ b/glean-core/tests/memory_distribution.rs @@ -127,7 +127,7 @@ fn the_accumulate_samples_api_correctly_stores_memory_values() { // Accumulate the samples. We intentionally do not report // negative values to not trigger error reporting. - metric.accumulate_samples_sync(&glean, [1, 2, 3].to_vec()); + metric.accumulate_samples_sync(&glean, [0, 1, 2, 3].to_vec()); let snapshot = metric .get_value(&glean, "store1") @@ -138,9 +138,13 @@ fn the_accumulate_samples_api_correctly_stores_memory_values() { // Check that we got the right sum of samples. assert_eq!(snapshot.sum, 6 * kb); - // We should get a sample in 3 buckets. + // Check that we have the right count of samples. + assert_eq!(snapshot.count, 4); + + // We should get a sample in 4 buckets. // These numbers are a bit magic, but they correspond to // `hist.sample_to_bucket_minimum(i * kb)` for `i = 1..=3`. + assert_eq!(1, snapshot.values[&0]); assert_eq!(1, snapshot.values[&1023]); assert_eq!(1, snapshot.values[&2047]); assert_eq!(1, snapshot.values[&3024]); diff --git a/glean-core/tests/timing_distribution.rs b/glean-core/tests/timing_distribution.rs index cd85f62fe7..4cbfa342c0 100644 --- a/glean-core/tests/timing_distribution.rs +++ b/glean-core/tests/timing_distribution.rs @@ -167,7 +167,7 @@ fn the_accumulate_samples_api_correctly_stores_timing_values() { // Accumulate the samples. We intentionally do not report // negative values to not trigger error reporting. - metric.accumulate_samples_sync(&glean, &[1, 2, 3]); + metric.accumulate_samples_sync(&glean, &[0, 1, 2, 3]); let snapshot = metric .get_value(&glean, "store1") @@ -179,11 +179,12 @@ fn the_accumulate_samples_api_correctly_stores_timing_values() { assert_eq!(snapshot.sum, 6 * seconds_to_nanos); // Check that we got the right number of samples. - assert_eq!(snapshot.count, 3); + assert_eq!(snapshot.count, 4); - // We should get a sample in 3 buckets. + // We should get a sample in 4 buckets. // These numbers are a bit magic, but they correspond to // `hist.sample_to_bucket_minimum(i * seconds_to_nanos)` for `i = 1..=3`. + assert_eq!(1, snapshot.values[&0]); assert_eq!(1, snapshot.values[&984625593]); assert_eq!(1, snapshot.values[&1969251187]); assert_eq!(1, snapshot.values[&2784941737]); @@ -399,7 +400,7 @@ fn raw_samples_api_error_cases() { metric.accumulate_raw_samples_nanos_sync( &glean, &[ - 0, /* rounded up to 1 */ + 0, /* valid */ 1, /* valid */ max_sample_time + 1, /* larger then the maximum, will record an error and the maximum */ ], @@ -410,7 +411,7 @@ fn raw_samples_api_error_cases() { .expect("Value should be stored"); // Check that we got the right sum. - assert_eq!(snapshot.sum, 2 + max_sample_time as i64); + assert_eq!(snapshot.sum, 1 + max_sample_time as i64); // Check that we got the right number of samples. assert_eq!(snapshot.count, 3); @@ -418,7 +419,8 @@ fn raw_samples_api_error_cases() { // We should get a sample in 3 buckets. // These numbers are a bit magic, but they correspond to // `hist.sample_to_bucket_minimum(i * seconds_to_nanos)` for `i = {1, max_sample_time}`. - assert_eq!(2, snapshot.values[&1]); + assert_eq!(1, snapshot.values[&0]); + assert_eq!(1, snapshot.values[&1]); assert_eq!(1, snapshot.values[&599512966122]); // 1 error should be reported.