-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
enhancement(prometheus_exporter sink): expire metrics #9769
Conversation
Closes #5867 Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
✔️ Deploy Preview for vector-project canceled. 🔨 Explore the source changes: 0dfa3ed 🔍 Inspect the deploy log: https://app.netlify.com/sites/vector-project/deploys/6182e39bf0c21c00078f2835 |
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
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 have some doubts to this approach. Did you consider making the update time part of the "value" part of the hash map instead of bundling it with the value?
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
I do agree it'd make more sense for this to live on the value side of the map, but it's awkward right now because the value itself lives in the key. I can move it in that direction, but I think my thought process was mostly that this is part of the "metrics entry". The whole thing would make more sense if we did something like |
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
@bruceg I switched over to the approach you suggested and I agree that's probably the way to go for now. |
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.
Logic looks good. I have an optimization suggestion below for the filter loop.
src/sinks/prometheus/exporter.rs
Outdated
(entry, metadata) | ||
}) | ||
.filter(|(_metric, metadata)| { | ||
metadata.updated_at.elapsed().as_secs() < self.config.flush_period_secs |
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 could use an Instant::now
computed outside of the filter and start.duration_since(metadata.updated_at).as_secs()
here.
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
This field took on more responsibility in #9769. This PR updates the doc to reflect its new semantics. Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
* docs(prometheus_exporter sink): Update `flush_period_secs` docs This field took on more responsibility in #9769. This PR updates the doc to reflect its new semantics. Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
Closes #5867