Skip to content

Commit 1bc9183

Browse files
committed
fix(aggregate): Fix incorrectly dropped events bug in metrics aggregate processor
1 parent c5959d7 commit 1bc9183

2 files changed

Lines changed: 139 additions & 66 deletions

File tree

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
The `aggregate` transform now correctly passes through metrics whose kind is not supported by the configured mode, rather than silently dropping them. For example, `absolute` metrics flowing through a `sum`-mode aggregate (and `incremental` metrics through `latest`, `diff`, `max`, `min`, `mean`, or `stdev` modes) are forwarded to the output unchanged without any aggregation.
2+
3+
authors: ArunPiduguDD

src/transforms/aggregate.rs

Lines changed: 136 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,14 @@ impl TransformConfig for AggregateConfig {
165165

166166
type MetricEntry = (MetricData, EventMetadata);
167167

168+
// Never stored in a collection, only ephemeral/transient to forward ignored events
169+
#[allow(clippy::large_enum_variant)]
170+
#[derive(Debug)]
171+
pub enum RecordOutcome {
172+
Aggregated,
173+
Passthrough(Event),
174+
}
175+
168176
#[derive(Debug)]
169177
pub struct Aggregate {
170178
interval: Duration,
@@ -181,9 +189,26 @@ impl Aggregate {
181189
})
182190
}
183191

184-
pub fn record(&mut self, event: Event) {
185-
let (series, data, metadata) = event.into_metric().into_parts();
192+
pub fn record(&mut self, event: Event) -> RecordOutcome {
193+
// Check kind before consuming the event so pass-through cases avoid a deconstruct/reconstruct.
194+
let kind = event.as_metric().kind();
195+
if matches!(
196+
(&self.mode, kind),
197+
(InnerMode::Sum, MetricKind::Absolute)
198+
| (
199+
InnerMode::Latest | InnerMode::Diff { .. },
200+
MetricKind::Incremental
201+
)
202+
| (InnerMode::Max | InnerMode::Min, MetricKind::Incremental)
203+
| (
204+
InnerMode::Mean { .. } | InnerMode::Stdev { .. },
205+
MetricKind::Incremental
206+
)
207+
) {
208+
return RecordOutcome::Passthrough(event);
209+
}
186210

211+
let (series, data, metadata) = event.into_metric().into_parts();
187212
match &mut self.mode {
188213
InnerMode::Auto => match data.kind {
189214
MetricKind::Incremental => self.record_sum(series, data, metadata),
@@ -192,33 +217,30 @@ impl Aggregate {
192217
}
193218
},
194219
InnerMode::Sum => self.record_sum(series, data, metadata),
195-
InnerMode::Latest | InnerMode::Diff { .. } => match data.kind {
196-
MetricKind::Incremental => (),
197-
MetricKind::Absolute => {
198-
self.map.insert(series, (data, metadata));
199-
}
200-
},
220+
InnerMode::Latest | InnerMode::Diff { .. } => {
221+
self.map.insert(series, (data, metadata));
222+
}
201223
InnerMode::Count => self.record_count(series, data, metadata),
202224
InnerMode::Max | InnerMode::Min => self.record_comparison(series, data, metadata),
203-
InnerMode::Mean { multi_map } | InnerMode::Stdev { multi_map } => match data.kind {
204-
MetricKind::Incremental => (),
205-
MetricKind::Absolute => {
206-
if matches!(data.value, MetricValue::Gauge { value: _ }) {
207-
match multi_map.entry(series) {
208-
Entry::Occupied(mut entry) => {
209-
let existing = entry.get_mut();
210-
existing.push((data, metadata));
211-
}
212-
Entry::Vacant(entry) => {
213-
entry.insert(vec![(data, metadata)]);
214-
}
225+
InnerMode::Mean { multi_map } | InnerMode::Stdev { multi_map } => {
226+
if matches!(data.value, MetricValue::Gauge { value: _ }) {
227+
match multi_map.entry(series) {
228+
Entry::Occupied(mut entry) => entry.get_mut().push((data, metadata)),
229+
Entry::Vacant(entry) => {
230+
entry.insert(vec![(data, metadata)]);
215231
}
216232
}
217233
}
218-
},
234+
}
219235
}
220-
221236
emit!(AggregateEventRecorded);
237+
RecordOutcome::Aggregated
238+
}
239+
240+
pub fn record_into(&mut self, event: Event, output: &mut Vec<Event>) {
241+
if let RecordOutcome::Passthrough(event) = self.record(event) {
242+
output.push(event);
243+
}
222244
}
223245

224246
fn record_count(
@@ -241,23 +263,20 @@ impl Aggregate {
241263
}
242264

243265
fn record_sum(&mut self, series: MetricSeries, data: MetricData, metadata: EventMetadata) {
244-
match data.kind {
245-
MetricKind::Incremental => match self.map.entry(series) {
246-
Entry::Occupied(mut entry) => {
247-
let existing = entry.get_mut();
248-
// In order to update (add) the new and old kind's must match
249-
if existing.0.kind == data.kind && existing.0.update(&data) {
250-
existing.1.merge(metadata);
251-
} else {
252-
emit!(AggregateUpdateFailed);
253-
*existing = (data, metadata);
254-
}
255-
}
256-
Entry::Vacant(entry) => {
257-
entry.insert((data, metadata));
266+
match self.map.entry(series) {
267+
Entry::Occupied(mut entry) => {
268+
let existing = entry.get_mut();
269+
// In order to update (add) the new and old kind's must match
270+
if existing.0.kind == data.kind && existing.0.update(&data) {
271+
existing.1.merge(metadata);
272+
} else {
273+
emit!(AggregateUpdateFailed);
274+
*existing = (data, metadata);
258275
}
259-
},
260-
MetricKind::Absolute => {}
276+
}
277+
Entry::Vacant(entry) => {
278+
entry.insert((data, metadata));
279+
}
261280
}
262281
}
263282

@@ -267,36 +286,33 @@ impl Aggregate {
267286
data: MetricData,
268287
metadata: EventMetadata,
269288
) {
270-
match data.kind {
271-
MetricKind::Incremental => (),
272-
MetricKind::Absolute => match self.map.entry(series) {
273-
Entry::Occupied(mut entry) => {
274-
let existing = entry.get_mut();
275-
// In order to update (add) the new and old kind's must match
276-
if existing.0.kind == data.kind {
277-
if let MetricValue::Gauge {
278-
value: existing_value,
279-
} = existing.0.value()
280-
&& let MetricValue::Gauge { value: new_value } = data.value()
281-
{
282-
let should_update = match self.mode {
283-
InnerMode::Max => new_value > existing_value,
284-
InnerMode::Min => new_value < existing_value,
285-
_ => false,
286-
};
287-
if should_update {
288-
*existing = (data, metadata);
289-
}
289+
match self.map.entry(series) {
290+
Entry::Occupied(mut entry) => {
291+
let existing = entry.get_mut();
292+
// In order to update (add) the new and old kind's must match
293+
if existing.0.kind == data.kind {
294+
if let MetricValue::Gauge {
295+
value: existing_value,
296+
} = existing.0.value()
297+
&& let MetricValue::Gauge { value: new_value } = data.value()
298+
{
299+
let should_update = match self.mode {
300+
InnerMode::Max => new_value > existing_value,
301+
InnerMode::Min => new_value < existing_value,
302+
_ => false,
303+
};
304+
if should_update {
305+
*existing = (data, metadata);
290306
}
291-
} else {
292-
emit!(AggregateUpdateFailed);
293-
*existing = (data, metadata);
294307
}
308+
} else {
309+
emit!(AggregateUpdateFailed);
310+
*existing = (data, metadata);
295311
}
296-
Entry::Vacant(entry) => {
297-
entry.insert((data, metadata));
298-
}
299-
},
312+
}
313+
Entry::Vacant(entry) => {
314+
entry.insert((data, metadata));
315+
}
300316
}
301317
}
302318

@@ -405,7 +421,7 @@ impl TaskTransform<Event> for Aggregate {
405421
self.flush_into(&mut output);
406422
done = true;
407423
}
408-
Some(event) => self.record(event),
424+
Some(event) => self.record_into(event, &mut output),
409425
}
410426
}
411427
};
@@ -949,6 +965,60 @@ mod tests {
949965
assert_eq!(&stdev_result, &out[0]);
950966
}
951967

968+
#[test]
969+
fn passes_through_ignored_kind() {
970+
// Sum mode aggregates incremental, passes through absolute without collapsing.
971+
let mut agg = Aggregate::new(&AggregateConfig {
972+
interval_ms: 1000_u64,
973+
mode: AggregationMode::Sum,
974+
})
975+
.unwrap();
976+
977+
let counter_1 = make_metric(
978+
"counter_a",
979+
MetricKind::Incremental,
980+
MetricValue::Counter { value: 10.0 },
981+
);
982+
let counter_2 = make_metric(
983+
"counter_a",
984+
MetricKind::Incremental,
985+
MetricValue::Counter { value: 5.0 },
986+
);
987+
let counter_summed = make_metric(
988+
"counter_a",
989+
MetricKind::Incremental,
990+
MetricValue::Counter { value: 15.0 },
991+
);
992+
let gauge_1 = make_metric(
993+
"gauge_a",
994+
MetricKind::Absolute,
995+
MetricValue::Gauge { value: 42.0 },
996+
);
997+
let gauge_2 = make_metric(
998+
"gauge_a",
999+
MetricKind::Absolute,
1000+
MetricValue::Gauge { value: 99.0 },
1001+
);
1002+
1003+
// Absolute metrics pass through immediately (not held until flush).
1004+
assert!(
1005+
matches!(agg.record(gauge_1.clone()), RecordOutcome::Passthrough(e) if e == gauge_1)
1006+
);
1007+
assert!(
1008+
matches!(agg.record(gauge_2.clone()), RecordOutcome::Passthrough(e) if e == gauge_2)
1009+
);
1010+
1011+
// Each is returned individually — no collapsing to latest.
1012+
agg.record(counter_1);
1013+
agg.record(counter_2);
1014+
1015+
let mut out = vec![];
1016+
agg.flush_into(&mut out);
1017+
// Only the summed incremental counter appears at flush; the gauges already passed through.
1018+
assert_eq!(1, out.len());
1019+
assert_eq!(&counter_summed, &out[0]);
1020+
}
1021+
9521022
#[test]
9531023
fn conflicting_value_type() {
9541024
let mut agg = Aggregate::new(&AggregateConfig {

0 commit comments

Comments
 (0)