-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(new transform): Add new incremental_to_absolute transform #23374
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?
feat(new transform): Add new incremental_to_absolute transform #23374
Conversation
Box::pin(stream! { | ||
let mut done = false; | ||
while !done { | ||
tokio::select! { |
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.
maybe this shouldn't be here? I just copied this from the aggregate transform
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.
no reason to use select with only one future to await.
Hey @GreyLilac09, thanks for the PR. Please update the test plan in your description including a vector config and expected output. This part allows us to run your code with a config and easily validate what the expected output should look like. Here is an example config you can build on top of:
or you can create one from scratch. Thanks! |
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.
All set from Docs!
@@ -58,11 +58,14 @@ impl MetricData { | |||
|
|||
/// Consumes this metric, returning it as an absolute metric. | |||
/// | |||
/// If the metric was already absolute, nothing is changed. | |||
/// The interval_ms is removed. If the metric was already absolute, nothing is changed. |
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.
I think without this change, DD sink (which use the presence of interval_ms to calculate rate) will be messed up
into_absolute appears to be only used by the remote write sink, and the resulting value doesn't have the interval_ms involved in the serialization, so I think this change is fine
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.
This makes sense to me. An absolute metric value represents a value at time t
.
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.
Pull Request Overview
This PR introduces a new incremental_to_absolute
transform that converts incremental metrics to absolute metrics while preserving the cumulative values. This is useful for avoiding duplicate metric caches at sink levels and creating historical records for scenarios with lossy connections or file-based backfilling.
Key changes:
- Implements the core transform logic with TTL-based metric expiration
- Adds comprehensive documentation and configuration examples
- Includes unit tests covering incremental-to-absolute conversion and pass-through behavior for already-absolute metrics
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/transforms/incremental_to_absolute.rs |
Core implementation of the transform with metric conversion logic and tests |
website/cue/reference/components/transforms/incremental_to_absolute.cue |
Documentation and configuration reference for the new transform |
lib/vector-core/src/event/metric/data.rs |
Updates into_absolute() method to remove interval_ms when converting metrics |
src/transforms/mod.rs |
Registers the new transform module |
website/cue/reference/components.cue |
Adds feature definition for the transform |
Cargo.toml |
Adds feature flags for the new transform |
changelog.d/incremental_to_absolute_transform.feature.md |
Changelog entry documenting the new feature |
Comments suppressed due to low confidence (2)
website/cue/reference/components/transforms/incremental_to_absolute.cue:50
- The example title 'Aggregate over 5 seconds' is misleading. This transform converts incremental to absolute metrics but doesn't aggregate over time windows. A more accurate title would be 'Convert incremental counter to absolute'.
title: "Aggregate over 5 seconds"
website/cue/reference/components/transforms/incremental_to_absolute.cue
Outdated
Show resolved
Hide resolved
}) | ||
} | ||
pub fn transform_one(&mut self, event: Event) -> Option<Event> { | ||
if let Some(metric) = self.data.make_absolute(event.as_metric().clone()) { |
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.
We should probably modify make_absolute
to set interval_ms
to None
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.
I think this is done in lib/vector-core/src/event/metric/data.rs?
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.
Do all paths end up hitting that conversion function?
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.
yes, make_absolute hits incremental_to_absolute which hits metrics.make_absolute()
After thinking about this some more, I'm not so sure if just the
Thus, I would propose an additional configuration, eg
where Alternatively, maybe it would just be better to group the cache configs like we do for
Curious to hear your thoughts. I'd also eventually like to add this config to the prom remote write sink (eg from this PR). I can also do add the LRU cache in a separate PR, but it would not be ideal to change the config later (eg. go from |
Can you explain with an example? From a UX perspective the following is better: transforms:
incremental_to_absolute:
type: incremental_to_absolute
cache:
max_size: 268435488
timeout_secs: 120s |
For example, if we increment (+1) count every 10 minutes, and the expire_metrics_secs is 5 minutes, that count would always just show up as 1 (unchanging) in prometheus and the increase in value is never logged |
@pront that makes sense, if this is the case we should also change the config of prom remote write sink (#23286) to be the same. I think the plan would be to modify this PR to use the LRU cache with this config, and in a separate follow-up PR modify the prom remote write config to have the same? |
Hi @GreyLilac09, this makes sense to me! |
Summary
Create new incremental_to_absolute transform
Useful for:
Problem it solves: #23018
Vector configuration
How did you test this PR?
Example 1
Example configuration:
Example output:
Example 2
If the interval of metrics exceeds
expire_metrics_secs
(eg for sparse metrics), the counters will get resetOutput:
Change Type
Is this a breaking change?
Does this PR include user facing changes?
no-changelog
label to this PR.References
Notes
@vectordotdev/vector
to reach out to us regarding this PR.pre-push
hook, please see this template.cargo fmt --all
cargo clippy --workspace --all-targets -- -D warnings
cargo nextest run --workspace
(alternatively, you can runcargo test --all
)git merge origin master
andgit push
.Cargo.lock
), pleaserun
cargo vdev build licenses
to regenerate the license inventory and commit the changes (if any). More details here.