From 351b1489c02f6c0d9cbc3be95db1036080a2d24c Mon Sep 17 00:00:00 2001 From: Vladimir Zhuk Date: Tue, 7 Jul 2026 16:34:21 +0200 Subject: [PATCH 1/8] fix(metrics): preserve exact sum/avg when converting histogram buckets to DDSketch AggregatedHistogram already tracks an exact running sum/count as each observation is recorded (see Histogram::record). transform_to_sketch discarded both and rebuilt approximate versions purely from bucket-boundary interpolation, which is a poor approximation when a bucket's true values sit far from its edges (e.g. nearly all mass in an unbounded first/last bucket, where interpolation collapses the bucket to a point mass at its single finite edge). count already survives interpolation exactly (interpolation always redistributes the exact input count, never losing or gaining any), so only override sum/avg with the values the source histogram already had. quantile() is unaffected since it only reads the sketch bins, not sum/count. Signed-off-by: Vladimir Zhuk --- lib/vector-core/src/metrics/ddsketch.rs | 67 ++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 2 deletions(-) diff --git a/lib/vector-core/src/metrics/ddsketch.rs b/lib/vector-core/src/metrics/ddsketch.rs index 9ad81a0f729dc..35cbcf47d8200 100644 --- a/lib/vector-core/src/metrics/ddsketch.rs +++ b/lib/vector-core/src/metrics/ddsketch.rs @@ -799,10 +799,21 @@ impl AgentDDSketch { } Some(sketch) } - MetricValue::AggregatedHistogram { buckets, .. } => { + MetricValue::AggregatedHistogram { buckets, sum, .. } => { let delta_buckets = mem::take(buckets); + let true_sum = *sum; let mut sketch = AgentDDSketch::with_agent_defaults(); sketch.insert_interpolate_buckets(delta_buckets)?; + // Bucket interpolation can only guess where within each bucket the + // observations fell, which biases `sum`/`avg`. The source histogram + // already tracked the exact sum as each observation was recorded + // (see `Histogram::record`), so prefer it over the bucket-derived + // approximation. `count` is unaffected: interpolation always + // redistributes the exact input count, never losing or gaining any. + if sketch.count > 0 { + sketch.avg = true_sum / f64::from(sketch.count); + } + sketch.sum = true_sum; Some(sketch) } // We can't convert from any other metric value. @@ -1108,7 +1119,7 @@ fn round_to_even(v: f64) -> f64 { #[cfg(test)] mod tests { use super::{AGENT_DEFAULT_EPS, AgentDDSketch, Config, MAX_KEY, round_to_even}; - use crate::event::metric::Bucket; + use crate::event::{Metric, MetricKind, MetricValue, metric::Bucket}; const FLOATING_POINT_ACCEPTABLE_ERROR: f64 = 1.0e-10; @@ -1245,6 +1256,58 @@ mod tests { assert_eq!(sketch, AgentDDSketch::with_agent_defaults()); } + #[test] + fn test_transform_to_sketch_preserves_exact_sum_for_unbounded_first_bucket() { + // Regression test: when nearly all observations fall in the first bucket + // (which has no explicit lower bound), bucket interpolation collapses that + // bucket to a point mass at its single finite edge. If the true values sit + // far below that edge, the bucket-derived sum is wildly wrong even though + // the exact sum was available on the source histogram all along. + let true_sum = 3.6182999999999996e-5; // two observations, ~1.8e-5 each + let true_count = 2; + + let metric = Metric::new( + "source_send_latency_seconds", + MetricKind::Absolute, + MetricValue::AggregatedHistogram { + buckets: vec![ + Bucket { + upper_limit: 0.000_244_140_625, + count: true_count, + }, + Bucket { + upper_limit: f64::INFINITY, + count: 0, + }, + ], + count: true_count, + sum: true_sum, + }, + ); + + let transformed = + AgentDDSketch::transform_to_sketch(metric).expect("valid histogram converts"); + let MetricValue::Sketch { sketch } = transformed.value() else { + panic!("expected a sketch value"); + }; + let crate::event::metric::MetricSketch::AgentDDSketch(sketch) = sketch; + + assert_eq!(sketch.count(), true_count as u32); + assert_eq!(sketch.sum(), Some(true_sum)); + assert_eq!(sketch.avg(), Some(true_sum / true_count as f64)); + + // Sanity check on the bug this guards against: naive bucket interpolation + // for this exact case reconstructs a sum around 4.6e-2 (roughly 1260x too + // large), because it collapses both observations to the bucket's upper + // edge (~2.4e-4) instead of their true, much smaller values. + let naive_bucket_derived_sum = 0.000_244_140_625 * true_count as f64; + assert!( + (naive_bucket_derived_sum - true_sum) / true_sum > 5.0, + "sanity check: the naive interpolation should be off by a large margin \ + for this input, otherwise this test isn't exercising the bug" + ); + } + #[test] fn test_merge() { let mut all_values = AgentDDSketch::with_agent_defaults(); From cb3b45442d346d62b3efd4801b7c9201f76aaddb Mon Sep 17 00:00:00 2001 From: Yoenn Burban Date: Tue, 7 Jul 2026 16:01:49 +0200 Subject: [PATCH 2/8] add a changelog --- changelog.d/25752_ddsketch_histogram_exact_sum.fix.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog.d/25752_ddsketch_histogram_exact_sum.fix.md diff --git a/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md b/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md new file mode 100644 index 0000000000000..24c27a4e6de57 --- /dev/null +++ b/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md @@ -0,0 +1,3 @@ +Fixed a bug where converting an `AggregatedHistogram` into a DDSketch (used by the `datadog_metrics` sink, among others) discarded the histogram's already-exact `sum`/`count` and instead rebuilt approximate versions purely from bucket-boundary interpolation. This broke down badly for buckets whose true values sit far from their edges, most notably the unbounded first/last bucket, where interpolation collapses to a point mass at a single finite edge — in some cases inflating the reported `avg`/`sum` by over 1000%. `sum`/`avg` are now taken directly from the source histogram's exact running totals instead of being reconstructed from bucket interpolation. + +authors: vladimir-dd gwenaskell From 8f5526299c45a2e3213d41c3eaf7f724d23f81b1 Mon Sep 17 00:00:00 2001 From: Yoenn Burban Date: Tue, 7 Jul 2026 16:02:08 +0200 Subject: [PATCH 3/8] fix clippy --- lib/vector-core/src/metrics/ddsketch.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/vector-core/src/metrics/ddsketch.rs b/lib/vector-core/src/metrics/ddsketch.rs index 35cbcf47d8200..bb601055eac97 100644 --- a/lib/vector-core/src/metrics/ddsketch.rs +++ b/lib/vector-core/src/metrics/ddsketch.rs @@ -1263,8 +1263,9 @@ mod tests { // bucket to a point mass at its single finite edge. If the true values sit // far below that edge, the bucket-derived sum is wildly wrong even though // the exact sum was available on the source histogram all along. - let true_sum = 3.6182999999999996e-5; // two observations, ~1.8e-5 each - let true_count = 2; + let true_sum = 3.618_299_999_999_999_6e-5; // two observations, ~1.8e-5 each + let true_count: u64 = 2; + let true_count_u32 = u32::try_from(true_count).expect("count fits in u32"); let metric = Metric::new( "source_send_latency_seconds", @@ -1292,15 +1293,15 @@ mod tests { }; let crate::event::metric::MetricSketch::AgentDDSketch(sketch) = sketch; - assert_eq!(sketch.count(), true_count as u32); + assert_eq!(sketch.count(), true_count_u32); assert_eq!(sketch.sum(), Some(true_sum)); - assert_eq!(sketch.avg(), Some(true_sum / true_count as f64)); + assert_eq!(sketch.avg(), Some(true_sum / f64::from(true_count_u32))); // Sanity check on the bug this guards against: naive bucket interpolation // for this exact case reconstructs a sum around 4.6e-2 (roughly 1260x too // large), because it collapses both observations to the bucket's upper // edge (~2.4e-4) instead of their true, much smaller values. - let naive_bucket_derived_sum = 0.000_244_140_625 * true_count as f64; + let naive_bucket_derived_sum = 0.000_244_140_625 * f64::from(true_count_u32); assert!( (naive_bucket_derived_sum - true_sum) / true_sum > 5.0, "sanity check: the naive interpolation should be off by a large margin \ From 3045e37fff00b0b36533b1d4dba9fc47cf6f04ea Mon Sep 17 00:00:00 2001 From: Yoenn Burban Date: Tue, 7 Jul 2026 17:12:06 +0200 Subject: [PATCH 4/8] fix(metrics): address review feedback on DDSketch histogram sum/avg fix - Use the histogram's own exact count (not the sketch's bucket-derived count) as the divisor for avg, since sources like Prometheus drop their cumulative +Inf bucket when converting to deltas, which can make the bucket-derived count undercount relative to the true total. - Represent OTLP's optional histogram sum as NaN instead of defaulting to an exact 0.0, so the sum/avg override is skipped (falling back to the bucket-derived estimate) rather than corrupting non-empty histograms that legitimately omit sum. - Extend min/max to keep them consistent with the now-exact avg, since interpolation can otherwise report an impossible avg outside [min, max] (most notably for the unbounded first/last bucket). --- .../25752_ddsketch_histogram_exact_sum.fix.md | 9 +- lib/opentelemetry-proto/src/metrics.rs | 12 +- lib/vector-core/src/metrics/ddsketch.rs | 154 +++++++++++++++++- 3 files changed, 164 insertions(+), 11 deletions(-) diff --git a/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md b/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md index 24c27a4e6de57..27de823b15208 100644 --- a/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md +++ b/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md @@ -1,3 +1,10 @@ -Fixed a bug where converting an `AggregatedHistogram` into a DDSketch (used by the `datadog_metrics` sink, among others) discarded the histogram's already-exact `sum`/`count` and instead rebuilt approximate versions purely from bucket-boundary interpolation. This broke down badly for buckets whose true values sit far from their edges, most notably the unbounded first/last bucket, where interpolation collapses to a point mass at a single finite edge — in some cases inflating the reported `avg`/`sum` by over 1000%. `sum`/`avg` are now taken directly from the source histogram's exact running totals instead of being reconstructed from bucket interpolation. +Fixed several bugs in how converting an `AggregatedHistogram` into a DDSketch (used by the `datadog_metrics` sink, among others) reported `sum`/`avg`: + +- Bucket-boundary interpolation discarded the histogram's already-exact `sum`/`count` and rebuilt approximate versions purely from interpolation. This broke down badly for buckets whose true values sit far from their edges, most notably the unbounded first/last bucket, where interpolation collapses to a point mass at a single finite edge — in some cases inflating the reported `avg`/`sum` by over 1000%. `sum`/`avg` are now taken directly from the source histogram's exact running totals instead. +- Sources that hand us fewer buckets than the histogram's true count (Prometheus always drops its cumulative `+Inf` bucket once converted to deltas) previously caused `avg` to be computed using the smaller, bucket-derived count instead of the histogram's true count, inflating `avg`. +- OTLP histograms that legitimately omit `sum` were defaulted to an exact `0.0`, which is indistinguishable from a genuinely all-zero histogram and was treated as authoritative, corrupting the `sum`/`avg` of otherwise-healthy non-empty histograms. Unknown sums are now represented distinctly so the bucket-derived estimate is used instead of a fabricated zero. +- `min`/`max` (still derived from bucket interpolation) could end up inconsistent with the now-exact `avg`, reporting impossible summary stats such as `avg < min`. `min`/`max` are now adjusted to keep `min <= avg <= max`. + +Note that quantile/percentile estimates are unaffected by these fixes, since they are derived from the sketch's bins rather than from `sum`/`count`. authors: vladimir-dd gwenaskell diff --git a/lib/opentelemetry-proto/src/metrics.rs b/lib/opentelemetry-proto/src/metrics.rs index ade00ab189be4..53252109dc131 100644 --- a/lib/opentelemetry-proto/src/metrics.rs +++ b/lib/opentelemetry-proto/src/metrics.rs @@ -332,7 +332,13 @@ impl HistogramMetric { MetricValue::AggregatedHistogram { buckets, count: self.point.count, - sum: self.point.sum.unwrap_or(0.0), + // OTLP histograms may legitimately omit `sum`. Represent that as + // `NaN` rather than `0.0`, since a silently defaulted zero would be + // indistinguishable from a genuinely all-zero histogram and would be + // treated as exact downstream (see + // `AgentDDSketch::transform_to_sketch`), corrupting otherwise-good + // bucket-derived estimates for any non-empty histogram that omits it. + sum: self.point.sum.unwrap_or(f64::NAN), }, ) .with_timestamp(timestamp) @@ -388,7 +394,9 @@ impl ExpHistogramMetric { MetricValue::AggregatedHistogram { buckets, count: self.point.count, - sum: self.point.sum.unwrap_or(0.0), + // See the comment in `HistogramMetric::into_metric` above: `NaN` + // signals "unknown" rather than a misleading exact `0.0`. + sum: self.point.sum.unwrap_or(f64::NAN), }, ) .with_timestamp(timestamp) diff --git a/lib/vector-core/src/metrics/ddsketch.rs b/lib/vector-core/src/metrics/ddsketch.rs index bb601055eac97..90d45af0ef74d 100644 --- a/lib/vector-core/src/metrics/ddsketch.rs +++ b/lib/vector-core/src/metrics/ddsketch.rs @@ -799,21 +799,54 @@ impl AgentDDSketch { } Some(sketch) } - MetricValue::AggregatedHistogram { buckets, sum, .. } => { + MetricValue::AggregatedHistogram { + buckets, + sum, + count, + } => { let delta_buckets = mem::take(buckets); let true_sum = *sum; + let true_count = *count; let mut sketch = AgentDDSketch::with_agent_defaults(); sketch.insert_interpolate_buckets(delta_buckets)?; // Bucket interpolation can only guess where within each bucket the // observations fell, which biases `sum`/`avg`. The source histogram - // already tracked the exact sum as each observation was recorded - // (see `Histogram::record`), so prefer it over the bucket-derived - // approximation. `count` is unaffected: interpolation always - // redistributes the exact input count, never losing or gaining any. - if sketch.count > 0 { - sketch.avg = true_sum / f64::from(sketch.count); + // already tracked an exact running sum as each observation was + // recorded (see `Histogram::record`), so prefer it over the + // bucket-derived approximation -- but only when it's actually + // known. Some sources (e.g. OTLP histograms, which may legitimately + // omit `sum`) represent "unknown" as `NaN` rather than a misleading + // `0.0` (see `HistogramMetric::into_metric`), so fall back to the + // bucket-derived `sum`/`avg` in that case instead of overriding them + // with a fabricated zero. + if true_sum.is_finite() { + // Some sources hand us fewer buckets than the histogram's true + // total count -- notably Prometheus, which always drops its + // cumulative "+Inf" bucket once converted to deltas -- so + // `sketch.count` (built purely from the buckets we were actually + // given) can undercount relative to `true_count`. Use the + // histogram's own exact count as the divisor for `avg` instead of + // `sketch.count`, to avoid inflating `avg` in that case. We + // deliberately leave `sketch.count` itself untouched, since + // `quantile()` relies on it lining up with the total weight + // actually distributed across the sketch's bins. + if true_count > 0 { + // Real histogram counts are always far below 2^52, so this + // conversion never loses meaningful precision in practice. + #[allow(clippy::cast_precision_loss)] + let true_count_f64 = true_count as f64; + sketch.avg = true_sum / true_count_f64; + // Interpolation places `min`/`max` at a bucket edge, which can + // fall outside this exact `avg` -- most notably for the + // unbounded first/last bucket, where interpolation collapses + // to a single point mass. Extend them just enough to keep the + // reported summary stats internally consistent + // (`min <= avg <= max`). + sketch.min = sketch.min.min(sketch.avg); + sketch.max = sketch.max.max(sketch.avg); + } + sketch.sum = true_sum; } - sketch.sum = true_sum; Some(sketch) } // We can't convert from any other metric value. @@ -1307,6 +1340,111 @@ mod tests { "sanity check: the naive interpolation should be off by a large margin \ for this input, otherwise this test isn't exercising the bug" ); + + // Regression test: overriding `sum`/`avg` without adjusting `min`/`max` + // can report an impossible `avg` outside of `[min, max]`, since + // interpolation still places `min`/`max` at the bucket's edge + // (~2.4e-4) while the true `avg` (~1.8e-5) sits far below it. + let min = sketch.min().expect("non-empty sketch has a min"); + let max = sketch.max().expect("non-empty sketch has a max"); + let avg = sketch.avg().expect("non-empty sketch has an avg"); + assert!( + min <= avg && avg <= max, + "summary stats must stay internally consistent: min={min} avg={avg} max={max}" + ); + } + + #[test] + fn test_transform_to_sketch_uses_true_count_when_buckets_undercount() { + // Regression test: Prometheus always drops its cumulative "+Inf" bucket + // once converted to per-bucket deltas (see + // `src/sources/prometheus/parser.rs`), so the buckets handed to + // `insert_interpolate_buckets` can sum to less than the histogram's true + // `count` whenever observations exist beyond the last finite bucket. + // `avg` must be derived from the histogram's own exact count, not from + // `sketch.count` (which only reflects the buckets we were actually given), + // otherwise it comes out too high. + let true_sum = 100.0; + let true_count: u64 = 10; + // Only 4 of the true 10 observations are represented in the (already + // "+Inf"-dropped) buckets we hand to the sketch. + let undercounted_buckets_count = 4; + + let metric = Metric::new( + "source_send_latency_seconds", + MetricKind::Absolute, + MetricValue::AggregatedHistogram { + buckets: vec![Bucket { + upper_limit: 1.0, + count: undercounted_buckets_count, + }], + count: true_count, + sum: true_sum, + }, + ); + + let transformed = + AgentDDSketch::transform_to_sketch(metric).expect("valid histogram converts"); + let MetricValue::Sketch { sketch } = transformed.value() else { + panic!("expected a sketch value"); + }; + let crate::event::metric::MetricSketch::AgentDDSketch(sketch) = sketch; + + // Real histogram counts are always far below 2^52, so this conversion + // never loses meaningful precision in practice. + #[allow(clippy::cast_precision_loss)] + let true_count_f64 = true_count as f64; + assert_eq!(sketch.sum(), Some(true_sum)); + assert_eq!( + sketch.avg(), + Some(true_sum / true_count_f64), + "avg must divide by the histogram's true count (10), not the \ + undercounted bucket-derived count (4)" + ); + } + + #[test] + fn test_transform_to_sketch_skips_override_for_unknown_sum() { + // Regression test: OTLP histograms may legitimately omit `sum`, which the + // OTLP parser represents as `NaN` rather than a misleading `0.0` (see + // `HistogramMetric::into_metric`). When `sum` is unknown, we must not + // override the bucket-derived `sum`/`avg` with it, since doing so would + // corrupt an otherwise-reasonable estimate into a hard `sum = 0`/`avg = 0` + // for what may be a large, non-empty histogram. + let metric = Metric::new( + "unknown_sum_histogram", + MetricKind::Absolute, + MetricValue::AggregatedHistogram { + buckets: vec![ + Bucket { + upper_limit: 50.0, + count: 50, + }, + Bucket { + upper_limit: 100.0, + count: 50, + }, + ], + count: 100, + sum: f64::NAN, + }, + ); + + let transformed = + AgentDDSketch::transform_to_sketch(metric).expect("valid histogram converts"); + let MetricValue::Sketch { sketch } = transformed.value() else { + panic!("expected a sketch value"); + }; + let crate::event::metric::MetricSketch::AgentDDSketch(sketch) = sketch; + + let sum = sketch.sum().expect("non-empty sketch has a sum"); + let avg = sketch.avg().expect("non-empty sketch has an avg"); + assert!( + sum.is_finite() && avg.is_finite(), + "sum/avg must fall back to the bucket-derived estimate, not NaN: sum={sum} avg={avg}" + ); + assert_ne!(sum, 0.0, "sum must not be corrupted to a hard zero"); + assert_ne!(avg, 0.0, "avg must not be corrupted to a hard zero"); } #[test] From 625bb962685be53292d93bda5a75d5442c27184a Mon Sep 17 00:00:00 2001 From: Yoenn Burban Date: Tue, 7 Jul 2026 19:04:19 +0200 Subject: [PATCH 5/8] simplify changelog and comment --- changelog.d/25752_ddsketch_histogram_exact_sum.fix.md | 1 - lib/vector-core/src/metrics/ddsketch.rs | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md b/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md index 27de823b15208..64b615c706c2c 100644 --- a/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md +++ b/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md @@ -3,7 +3,6 @@ Fixed several bugs in how converting an `AggregatedHistogram` into a DDSketch (u - Bucket-boundary interpolation discarded the histogram's already-exact `sum`/`count` and rebuilt approximate versions purely from interpolation. This broke down badly for buckets whose true values sit far from their edges, most notably the unbounded first/last bucket, where interpolation collapses to a point mass at a single finite edge — in some cases inflating the reported `avg`/`sum` by over 1000%. `sum`/`avg` are now taken directly from the source histogram's exact running totals instead. - Sources that hand us fewer buckets than the histogram's true count (Prometheus always drops its cumulative `+Inf` bucket once converted to deltas) previously caused `avg` to be computed using the smaller, bucket-derived count instead of the histogram's true count, inflating `avg`. - OTLP histograms that legitimately omit `sum` were defaulted to an exact `0.0`, which is indistinguishable from a genuinely all-zero histogram and was treated as authoritative, corrupting the `sum`/`avg` of otherwise-healthy non-empty histograms. Unknown sums are now represented distinctly so the bucket-derived estimate is used instead of a fabricated zero. -- `min`/`max` (still derived from bucket interpolation) could end up inconsistent with the now-exact `avg`, reporting impossible summary stats such as `avg < min`. `min`/`max` are now adjusted to keep `min <= avg <= max`. Note that quantile/percentile estimates are unaffected by these fixes, since they are derived from the sketch's bins rather than from `sum`/`count`. diff --git a/lib/vector-core/src/metrics/ddsketch.rs b/lib/vector-core/src/metrics/ddsketch.rs index 90d45af0ef74d..1fc5a51b846c2 100644 --- a/lib/vector-core/src/metrics/ddsketch.rs +++ b/lib/vector-core/src/metrics/ddsketch.rs @@ -815,10 +815,9 @@ impl AgentDDSketch { // recorded (see `Histogram::record`), so prefer it over the // bucket-derived approximation -- but only when it's actually // known. Some sources (e.g. OTLP histograms, which may legitimately - // omit `sum`) represent "unknown" as `NaN` rather than a misleading - // `0.0` (see `HistogramMetric::into_metric`), so fall back to the - // bucket-derived `sum`/`avg` in that case instead of overriding them - // with a fabricated zero. + // omit `sum`) represent "unknown" as `NaN` (see `HistogramMetric::into_metric`), + // so fall back to the bucket-derived `sum`/`avg` in that case instead of + // overriding them with a fabricated zero. if true_sum.is_finite() { // Some sources hand us fewer buckets than the histogram's true // total count -- notably Prometheus, which always drops its @@ -845,6 +844,7 @@ impl AgentDDSketch { sketch.min = sketch.min.min(sketch.avg); sketch.max = sketch.max.max(sketch.avg); } + sketch.sum = true_sum; } Some(sketch) From 5ab4c0092780150df17e755fffe1a474354fe4b7 Mon Sep 17 00:00:00 2001 From: Yoenn Burban Date: Wed, 8 Jul 2026 10:53:04 +0200 Subject: [PATCH 6/8] revert use of NaN and skip exact computation if counts mismatch --- .../25752_ddsketch_histogram_exact_sum.fix.md | 8 +- lib/opentelemetry-proto/src/metrics.rs | 12 +- lib/vector-core/src/metrics/ddsketch.rs | 146 +++++++++--------- 3 files changed, 73 insertions(+), 93 deletions(-) diff --git a/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md b/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md index 64b615c706c2c..3b087f5142988 100644 --- a/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md +++ b/changelog.d/25752_ddsketch_histogram_exact_sum.fix.md @@ -1,9 +1,3 @@ -Fixed several bugs in how converting an `AggregatedHistogram` into a DDSketch (used by the `datadog_metrics` sink, among others) reported `sum`/`avg`: - -- Bucket-boundary interpolation discarded the histogram's already-exact `sum`/`count` and rebuilt approximate versions purely from interpolation. This broke down badly for buckets whose true values sit far from their edges, most notably the unbounded first/last bucket, where interpolation collapses to a point mass at a single finite edge — in some cases inflating the reported `avg`/`sum` by over 1000%. `sum`/`avg` are now taken directly from the source histogram's exact running totals instead. -- Sources that hand us fewer buckets than the histogram's true count (Prometheus always drops its cumulative `+Inf` bucket once converted to deltas) previously caused `avg` to be computed using the smaller, bucket-derived count instead of the histogram's true count, inflating `avg`. -- OTLP histograms that legitimately omit `sum` were defaulted to an exact `0.0`, which is indistinguishable from a genuinely all-zero histogram and was treated as authoritative, corrupting the `sum`/`avg` of otherwise-healthy non-empty histograms. Unknown sums are now represented distinctly so the bucket-derived estimate is used instead of a fabricated zero. - -Note that quantile/percentile estimates are unaffected by these fixes, since they are derived from the sketch's bins rather than from `sum`/`count`. +Fixed `sum`/`avg` reported by the DDSketch generated when converting an `AggregatedHistogram` (used by the `datadog_metrics` sink, among others). Both were previously reconstructed purely from bucket-boundary interpolation, which breaks down badly when a bucket's true values sit far from its edges — most notably the unbounded first/last bucket, where interpolation collapses to a point mass at a single finite edge, in some cases inflating `avg`/`sum` by over 1000%. `sum`/`avg` are now taken directly from the histogram's own exact running totals instead (skipped if the buckets don't fully account for the histogram's true count — e.g. Prometheus dropping its cumulative `+Inf` bucket). Quantile/percentile estimates are unaffected either way, since they're derived from the sketch's bins rather than `sum`/`count`. authors: vladimir-dd gwenaskell diff --git a/lib/opentelemetry-proto/src/metrics.rs b/lib/opentelemetry-proto/src/metrics.rs index 53252109dc131..ade00ab189be4 100644 --- a/lib/opentelemetry-proto/src/metrics.rs +++ b/lib/opentelemetry-proto/src/metrics.rs @@ -332,13 +332,7 @@ impl HistogramMetric { MetricValue::AggregatedHistogram { buckets, count: self.point.count, - // OTLP histograms may legitimately omit `sum`. Represent that as - // `NaN` rather than `0.0`, since a silently defaulted zero would be - // indistinguishable from a genuinely all-zero histogram and would be - // treated as exact downstream (see - // `AgentDDSketch::transform_to_sketch`), corrupting otherwise-good - // bucket-derived estimates for any non-empty histogram that omits it. - sum: self.point.sum.unwrap_or(f64::NAN), + sum: self.point.sum.unwrap_or(0.0), }, ) .with_timestamp(timestamp) @@ -394,9 +388,7 @@ impl ExpHistogramMetric { MetricValue::AggregatedHistogram { buckets, count: self.point.count, - // See the comment in `HistogramMetric::into_metric` above: `NaN` - // signals "unknown" rather than a misleading exact `0.0`. - sum: self.point.sum.unwrap_or(f64::NAN), + sum: self.point.sum.unwrap_or(0.0), }, ) .with_timestamp(timestamp) diff --git a/lib/vector-core/src/metrics/ddsketch.rs b/lib/vector-core/src/metrics/ddsketch.rs index 1fc5a51b846c2..d97034b617c05 100644 --- a/lib/vector-core/src/metrics/ddsketch.rs +++ b/lib/vector-core/src/metrics/ddsketch.rs @@ -809,42 +809,36 @@ impl AgentDDSketch { let true_count = *count; let mut sketch = AgentDDSketch::with_agent_defaults(); sketch.insert_interpolate_buckets(delta_buckets)?; - // Bucket interpolation can only guess where within each bucket the - // observations fell, which biases `sum`/`avg`. The source histogram - // already tracked an exact running sum as each observation was - // recorded (see `Histogram::record`), so prefer it over the - // bucket-derived approximation -- but only when it's actually - // known. Some sources (e.g. OTLP histograms, which may legitimately - // omit `sum`) represent "unknown" as `NaN` (see `HistogramMetric::into_metric`), - // so fall back to the bucket-derived `sum`/`avg` in that case instead of - // overriding them with a fabricated zero. - if true_sum.is_finite() { - // Some sources hand us fewer buckets than the histogram's true - // total count -- notably Prometheus, which always drops its - // cumulative "+Inf" bucket once converted to deltas -- so - // `sketch.count` (built purely from the buckets we were actually - // given) can undercount relative to `true_count`. Use the - // histogram's own exact count as the divisor for `avg` instead of - // `sketch.count`, to avoid inflating `avg` in that case. We - // deliberately leave `sketch.count` itself untouched, since - // `quantile()` relies on it lining up with the total weight - // actually distributed across the sketch's bins. - if true_count > 0 { - // Real histogram counts are always far below 2^52, so this - // conversion never loses meaningful precision in practice. - #[allow(clippy::cast_precision_loss)] - let true_count_f64 = true_count as f64; - sketch.avg = true_sum / true_count_f64; - // Interpolation places `min`/`max` at a bucket edge, which can - // fall outside this exact `avg` -- most notably for the - // unbounded first/last bucket, where interpolation collapses - // to a single point mass. Extend them just enough to keep the - // reported summary stats internally consistent - // (`min <= avg <= max`). - sketch.min = sketch.min.min(sketch.avg); - sketch.max = sketch.max.max(sketch.avg); - } - + // Some sources hand us fewer buckets than the histogram's true + // total count -- notably Prometheus, which always drops its + // cumulative "+Inf" bucket once converted to deltas, so any + // observations beyond the last finite bound go missing from the + // buckets we actually receive. In that case, `sketch.count()` + // (built purely from those buckets) won't match the histogram's + // own exact `count`. We can't just use `true_count` as the divisor + // for `avg` while leaving `sketch.count()` as-is: the encoder + // reports `sketch.count()` as the sketch's `cnt`, so a mismatched + // pair would ship e.g. `cnt=4` alongside `sum`/`avg` that reflect + // all 10 true observations, corrupting the `sum`/`avg`/`cnt` + // invariant for downstream consumers. So only apply the exact + // override when the counts agree; otherwise, fall back to the + // bucket-derived `sum`/`avg`, which stay self-consistent with + // `sketch.count()` since both are derived from the same + // (possibly incomplete) buckets. + if true_count > 0 && true_count == u64::from(sketch.count()) { + // Real histogram counts are always far below 2^52, so this + // conversion never loses meaningful precision in practice. + #[allow(clippy::cast_precision_loss)] + let true_count_f64 = true_count as f64; + sketch.avg = true_sum / true_count_f64; + // Interpolation places `min`/`max` at a bucket edge, which can + // fall outside this exact `avg` -- most notably for the + // unbounded first/last bucket, where interpolation collapses + // to a single point mass. Extend them just enough to keep the + // reported summary stats internally consistent + // (`min <= avg <= max`). + sketch.min = sketch.min.min(sketch.avg); + sketch.max = sketch.max.max(sketch.avg); sketch.sum = true_sum; } Some(sketch) @@ -1355,15 +1349,18 @@ mod tests { } #[test] - fn test_transform_to_sketch_uses_true_count_when_buckets_undercount() { + fn test_transform_to_sketch_skips_override_when_buckets_undercount() { // Regression test: Prometheus always drops its cumulative "+Inf" bucket // once converted to per-bucket deltas (see // `src/sources/prometheus/parser.rs`), so the buckets handed to // `insert_interpolate_buckets` can sum to less than the histogram's true - // `count` whenever observations exist beyond the last finite bucket. - // `avg` must be derived from the histogram's own exact count, not from - // `sketch.count` (which only reflects the buckets we were actually given), - // otherwise it comes out too high. + // `count` whenever observations exist beyond the last finite bucket. In + // that case, overriding `sum`/`avg` with the histogram's exact values + // while `sketch.count()` (exported as the sketch's `cnt`) still reflects + // only the undercounted buckets would corrupt the `sum`/`avg`/`cnt` + // invariant downstream. So the override must be skipped entirely, + // falling back to the bucket-derived `sum`/`avg`, which stay consistent + // with `sketch.count()`. let true_sum = 100.0; let true_count: u64 = 10; // Only 4 of the true 10 observations are represented in the (already @@ -1390,43 +1387,43 @@ mod tests { }; let crate::event::metric::MetricSketch::AgentDDSketch(sketch) = sketch; - // Real histogram counts are always far below 2^52, so this conversion - // never loses meaningful precision in practice. - #[allow(clippy::cast_precision_loss)] - let true_count_f64 = true_count as f64; - assert_eq!(sketch.sum(), Some(true_sum)); assert_eq!( + u64::from(sketch.count()), + undercounted_buckets_count, + "sketch.count() must reflect only the buckets actually given, not the \ + histogram's true count" + ); + assert_ne!( + sketch.sum(), + Some(true_sum), + "sum must not be overridden with the true value while cnt is undercounted" + ); + assert_ne!( sketch.avg(), - Some(true_sum / true_count_f64), - "avg must divide by the histogram's true count (10), not the \ - undercounted bucket-derived count (4)" + Some(true_sum / f64::from(u32::try_from(true_count).unwrap())), + "avg must not be derived from the true count while cnt is undercounted" ); } #[test] - fn test_transform_to_sketch_skips_override_for_unknown_sum() { - // Regression test: OTLP histograms may legitimately omit `sum`, which the - // OTLP parser represents as `NaN` rather than a misleading `0.0` (see - // `HistogramMetric::into_metric`). When `sum` is unknown, we must not - // override the bucket-derived `sum`/`avg` with it, since doing so would - // corrupt an otherwise-reasonable estimate into a hard `sum = 0`/`avg = 0` - // for what may be a large, non-empty histogram. + fn test_transform_to_sketch_applies_override_when_counts_match() { + // Sanity check for the above: when the buckets we're given fully account + // for the histogram's true count (the common case -- Vector's own + // internal histograms and spec-conformant OTLP histograms never drop + // buckets), the exact override still applies as normal. + let true_sum = 100.0; + let true_count: u64 = 10; + let metric = Metric::new( - "unknown_sum_histogram", + "source_send_latency_seconds", MetricKind::Absolute, MetricValue::AggregatedHistogram { - buckets: vec![ - Bucket { - upper_limit: 50.0, - count: 50, - }, - Bucket { - upper_limit: 100.0, - count: 50, - }, - ], - count: 100, - sum: f64::NAN, + buckets: vec![Bucket { + upper_limit: 1.0, + count: true_count, + }], + count: true_count, + sum: true_sum, }, ); @@ -1437,14 +1434,11 @@ mod tests { }; let crate::event::metric::MetricSketch::AgentDDSketch(sketch) = sketch; - let sum = sketch.sum().expect("non-empty sketch has a sum"); - let avg = sketch.avg().expect("non-empty sketch has an avg"); - assert!( - sum.is_finite() && avg.is_finite(), - "sum/avg must fall back to the bucket-derived estimate, not NaN: sum={sum} avg={avg}" + assert_eq!(sketch.sum(), Some(true_sum)); + assert_eq!( + sketch.avg(), + Some(true_sum / f64::from(u32::try_from(true_count).unwrap())) ); - assert_ne!(sum, 0.0, "sum must not be corrupted to a hard zero"); - assert_ne!(avg, 0.0, "avg must not be corrupted to a hard zero"); } #[test] From a7270b9befb4b168e3ea5f5acd3cb8fb90753cb5 Mon Sep 17 00:00:00 2001 From: Yoenn Burban <62966490+gwenaskell@users.noreply.github.com> Date: Thu, 9 Jul 2026 09:52:58 +0200 Subject: [PATCH 7/8] apply suggestion Co-authored-by: Bruce Guenter --- lib/vector-core/src/metrics/ddsketch.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/vector-core/src/metrics/ddsketch.rs b/lib/vector-core/src/metrics/ddsketch.rs index d97034b617c05..0d2d3b32a93f5 100644 --- a/lib/vector-core/src/metrics/ddsketch.rs +++ b/lib/vector-core/src/metrics/ddsketch.rs @@ -826,9 +826,10 @@ impl AgentDDSketch { // `sketch.count()` since both are derived from the same // (possibly incomplete) buckets. if true_count > 0 && true_count == u64::from(sketch.count()) { - // Real histogram counts are always far below 2^52, so this - // conversion never loses meaningful precision in practice. - #[allow(clippy::cast_precision_loss)] + #[allow( + clippy::cast_precision_loss, + reason="Real histogram counts are always far below 2^52, so this conversion never loses meaningful precision in practice" + )] let true_count_f64 = true_count as f64; sketch.avg = true_sum / true_count_f64; // Interpolation places `min`/`max` at a bucket edge, which can From 21060e697b0ca7e1c430937126faf0137bc5209b Mon Sep 17 00:00:00 2001 From: Yoenn Burban Date: Thu, 9 Jul 2026 12:03:12 +0200 Subject: [PATCH 8/8] fmt --- lib/vector-core/src/metrics/ddsketch.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/vector-core/src/metrics/ddsketch.rs b/lib/vector-core/src/metrics/ddsketch.rs index 0d2d3b32a93f5..ac435a85fe500 100644 --- a/lib/vector-core/src/metrics/ddsketch.rs +++ b/lib/vector-core/src/metrics/ddsketch.rs @@ -828,7 +828,7 @@ impl AgentDDSketch { if true_count > 0 && true_count == u64::from(sketch.count()) { #[allow( clippy::cast_precision_loss, - reason="Real histogram counts are always far below 2^52, so this conversion never loses meaningful precision in practice" + reason = "Real histogram counts are always far below 2^52, so this conversion never loses meaningful precision in practice" )] let true_count_f64 = true_count as f64; sketch.avg = true_sum / true_count_f64;