Skip to content

Commit 497f5d8

Browse files
committed
Reject metric interval merges that overflow
MetricData::update added the timestamp delta and an interval with unchecked u32 arithmetic. Intervals near u32::MAX panicked in debug builds and wrapped in release builds. Return false instead, as the function already does when the timestamp delta does not fit. Round the Datadog whole-second interval with integer arithmetic so it needs no float cast, and fix two pedantic lints in the new tests.
1 parent 9a4c15a commit 497f5d8

5 files changed

Lines changed: 39 additions & 12 deletions

File tree

‎changelog.d/26597_datadog_metrics_fractional_interval.fix.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ division. Counters with an interval under one second were sent as rates with inf
33
values, and intervals that are not whole seconds overstated the rate. The rate value now uses
44
the exact interval, and the interval field is rounded to the nearest whole second (at least 1).
55
When the sink combines counters with an interval that share a series and second, it now also
6-
combines their intervals, so the rate is not overstated.
6+
combines their intervals, so the rate is not overstated. Merging two metric intervals whose
7+
combined window does not fit in `interval_ms` no longer panics or wraps.
78

89
authors: gremlinops

‎lib/vector-core/src/event/metric/data.rs‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -108,16 +108,19 @@ impl MetricData {
108108
return false;
109109
};
110110

111-
if t1 > t2 {
111+
let (start, end_interval) = if t1 > t2 {
112112
// The interval window starts from the beginning of `other` (aka `t2`)
113113
// and goes to the end of `self` (which is `t1 + i1`).
114-
(Some(t2), NonZeroU32::new(delta_t + i1.get()))
114+
(t2, i1)
115115
} else {
116116
// The interval window starts from the beginning of `self` (aka `t1`)
117117
// and goes to the end of `other` (which is `t2 + i2`).
118-
119-
(Some(t1), NonZeroU32::new(delta_t + i2.get()))
120-
}
118+
(t1, i2)
119+
};
120+
let Some(interval) = delta_t.checked_add(end_interval.get()) else {
121+
return false;
122+
};
123+
(Some(start), NonZeroU32::new(interval))
121124
}
122125
(Some(t), _, None, _) | (None, _, Some(t), _) => (Some(t), None),
123126
(Some(t1), _, Some(t2), _) => (Some(t1.max(t2)), None),

‎lib/vector-core/src/event/metric/mod.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1203,4 +1203,23 @@ mod test {
12031203
assert!(gauge.data.add(&delta.data));
12041204
assert_eq!(gauge, expected);
12051205
}
1206+
1207+
#[test]
1208+
fn merge_interval_overflow_is_rejected() {
1209+
let mut counter = Metric::new(
1210+
"counter",
1211+
MetricKind::Incremental,
1212+
MetricValue::Counter { value: 1.0 },
1213+
)
1214+
.with_timestamp(Some(ts()))
1215+
.with_interval_ms(std::num::NonZeroU32::new(u32::MAX));
1216+
1217+
let delta = counter
1218+
.clone()
1219+
.with_timestamp(Some(ts() + chrono::Duration::milliseconds(1)));
1220+
1221+
let before = counter.clone();
1222+
assert!(!counter.data.add(&delta.data));
1223+
assert_eq!(counter, before);
1224+
}
12061225
}

‎src/sinks/datadog/metrics/encoder.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -570,10 +570,11 @@ pub(super) struct SeriesInterval {
570570
/// Converts a metric's millisecond `interval_ms`, if any, into a [`SeriesInterval`].
571571
pub(super) fn series_interval(metric: &Metric) -> Option<SeriesInterval> {
572572
metric.interval_ms().map(|interval_ms| {
573-
let exact_secs = f64::from(interval_ms.get()) / 1000.0;
573+
let ms = interval_ms.get();
574+
let rounded_secs = ms / 1000 + u32::from(ms % 1000 >= 500);
574575
SeriesInterval {
575-
whole_secs: (exact_secs.round() as u32).max(1),
576-
exact_secs,
576+
whole_secs: rounded_secs.max(1),
577+
exact_secs: f64::from(ms) / 1000.0,
577578
}
578579
})
579580
}

‎src/transforms/aggregate/tests.rs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1711,7 +1711,10 @@ fn with_set_interval_ms(config: AggregateConfig) -> AggregateConfig {
17111711
}
17121712

17131713
fn interval_ms_of(event: &Event) -> Option<u32> {
1714-
event.as_metric().interval_ms().map(|i| i.get())
1714+
event
1715+
.as_metric()
1716+
.interval_ms()
1717+
.map(std::num::NonZeroU32::get)
17151718
}
17161719

17171720
#[test]
@@ -1728,10 +1731,10 @@ fn set_interval_ms_parses_and_defaults_to_false() {
17281731
assert!(!cfg.set_interval_ms);
17291732

17301733
let cfg: AggregateConfig = toml::from_str(
1731-
r#"
1734+
r"
17321735
interval_ms = 5000
17331736
set_interval_ms = true
1734-
"#,
1737+
",
17351738
)
17361739
.unwrap();
17371740
assert!(cfg.set_interval_ms);

0 commit comments

Comments
 (0)