Repository navigation
feat(aggregate transform): Add set_interval_ms option
#26597
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.
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
66c3d55
03f5992
62d0f91
9a4c15a
497f5d8
ac4ea38
caa499c
4899e91
f7407f8
b14115f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| The `aggregate` transform now has a `set_interval_ms` option. When enabled, each flushed | ||
| incremental metric gets `interval_ms` set to the configured flush interval, so sinks such as | ||
| `datadog_metrics` send aggregated counters as rates instead of counts. | ||
|
|
||
| authors: gremlinops |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ use super::{AggregateConfig, AggregationMode}; | |
|
|
||
| use std::{ | ||
| collections::{BTreeMap, HashMap, hash_map::Entry}, | ||
| num::NonZeroU32, | ||
| pin::Pin, | ||
| time::Duration, | ||
| }; | ||
|
|
@@ -95,6 +96,8 @@ pub struct Aggregate { | |
| BTreeMap<BucketKey, HashMap<MetricSeries, Vec<MetricEntry>>>, | ||
| pub(crate) watermark: Option<BucketKey>, | ||
| pub(crate) config: AggregateConfig, | ||
| /// Interval to set on flushed incremental metrics; `Some` only when `set_interval_ms` is enabled. | ||
| output_interval_ms: Option<NonZeroU32>, | ||
| } | ||
|
|
||
| /// Upper bound for any millisecond-valued duration field that is later cast | ||
|
|
@@ -129,6 +132,19 @@ impl Aggregate { | |
| .into()); | ||
| } | ||
| } | ||
| let output_interval_ms = if config.set_interval_ms { | ||
| let interval_ms = u32::try_from(config.interval_ms).map_err(|_| { | ||
| format!( | ||
| "`interval_ms` ({}) exceeds the maximum supported value of {} ms when `set_interval_ms` is enabled", | ||
| config.interval_ms, | ||
| u32::MAX | ||
| ) | ||
| })?; | ||
| // Non-zero was validated above. | ||
| NonZeroU32::new(interval_ms) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| Ok(Self { | ||
| interval: Duration::from_millis(config.interval_ms), | ||
|
|
@@ -139,6 +155,7 @@ impl Aggregate { | |
| event_time_multi_buckets: Default::default(), | ||
| watermark: None, | ||
| config: *config, | ||
| output_interval_ms, | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -324,11 +341,13 @@ impl Aggregate { | |
| } | ||
| } | ||
| pub fn flush_into(&mut self, output: &mut Vec<Event>) { | ||
| let start = output.len(); | ||
| if self.config.is_event_time() { | ||
| self.flush_event_time_buckets(output, false); | ||
| } else { | ||
| self.flush_system_time(output); | ||
| } | ||
| self.set_output_interval(&mut output[start..]); | ||
| } | ||
|
|
||
| /// Final flush invoked when the input stream closes. In event-time mode | ||
|
|
@@ -337,11 +356,27 @@ impl Aggregate { | |
| /// shutdown or topology reload, matching system-time semantics where | ||
| /// `flush_system_time` always empties `self.map`. | ||
| pub fn flush_final(&mut self, output: &mut Vec<Event>) { | ||
| let start = output.len(); | ||
| if self.config.is_event_time() { | ||
| self.flush_event_time_buckets(output, true); | ||
| } else { | ||
| self.flush_system_time(output); | ||
| } | ||
| self.set_output_interval(&mut output[start..]); | ||
| } | ||
|
|
||
| /// Overwrites `interval_ms` on flushed incremental metrics with the flush interval, so a | ||
| /// counter aggregated over the window can be read as a rate over that window. | ||
| fn set_output_interval(&self, flushed: &mut [Event]) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah... With Proposal for both
Tradeoff: for a non-whole-second interval the point value is exact, but Datadog's
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 62d0f91 |
||
| let Some(interval_ms) = self.output_interval_ms else { | ||
| return; | ||
| }; | ||
| for event in flushed { | ||
| let metric = event.as_mut_metric(); | ||
| if metric.kind() == MetricKind::Incremental { | ||
| metric.data_mut().time.interval_ms = Some(interval_ms); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid, fixed in 9a4c15a, but the reason differs: the Datadog Agent stamps count and rate series with the bucket start, and Vector's
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted in caa499c to keep this PR focused There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in ac4ea38. Opposite of what I said on the earlier thread about system-time mode. With
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted in caa499c to keep this PR focused There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When system-time mode starts with buffered input and Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point, but I'm leaving it out of this PR. The immediate first tick is existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deferred to the follow-up PR, see "Not in this PR" in the description. Since f7407f8, metrics without a timestamp get their window start. Metrics that already have a timestamp keep it, so a rate can be placed later than the window it covers. The per-window value is not affected. |
||
| } | ||
| } | ||
| } | ||
|
|
||
| fn flush_system_time(&mut self, output: &mut Vec<Event>) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the input closes or a topology reload happens partway through the default system-time window,
flush_finaldrains the partial aggregate and this call still stamps the entire configured interval. Thus a transform stopped one second after the previous flush withinterval_ms = 10000sends its count as a 10-second rate (the Datadog encoder divides by 10), underreporting that final window by 10x. Track the elapsed final-window duration or leave the interval unset on an incomplete final flush.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deferred, see "Not in this PR" in the description. The count is kept: the point is the window's count divided by the interval, so rate × interval gives the count back; a partial window only reads low per second. Fixing it needs the real window length, which neither this PR nor the follow-up tracks yet. Leaving the interval off that point instead would send it as a count while the rest of the metric is a rate.