Skip to content

bug 2049040 - Accept 0-duration samples in timing_distribution metrics #3512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
chutten wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

bug 2049040 - Accept 0-duration samples in timing_distribution metrics #3512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion docs/user/reference/metrics/timing_distribution.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
12 changes: 4 additions & 8 deletions glean-core/src/metrics/timing_distribution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 6 additions & 2 deletions glean-core/tests/memory_distribution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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]);
Expand Down
14 changes: 8 additions & 6 deletions glean-core/tests/timing_distribution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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]);
Expand Down Expand Up @@ -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 */
],
Expand All @@ -410,15 +411,16 @@ 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);

// 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.
Expand Down