-
Notifications
You must be signed in to change notification settings - Fork 10
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
update metriken to replace heatmaps #52
Conversation
Updates metriken library to replace heatmaps with histograms which have better runtime performance characteristics and lower memory footprint.
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.
clippy found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
pub m: u32, | ||
pub r: u32, | ||
pub n: u32, | ||
pub grouping_power: u8, |
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 might end up changing this to include the entire Config when we move this to rustcommon, but I think this is fine for now.
Changes the behavior to keep just a single set of previous snapshots and cached deltas. We assume secondly collection and will serve percentiles up to one second stale, but with timestamps in the prometheus format to aid in correlating with other metrics.
pub deltas: HashMap<String, metriken::histogram::Snapshot>, | ||
} | ||
|
||
pub struct MetricsSnapshot { |
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.
Just to make sure I understand: now when the admin thread serves out snapshotted metrics from METRICS_SNAPSHOT
, both histogram and counter values will be from the snapshot (and potentially upto 1s stale), as opposed to the previous version where counters would be fresh and the histogram from the snapshot, right?
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. The admin thread will still serve fresh counters + gauges, but stale percentiles (with timestamps for percentile metrics only). See L#272 in admin/mod.rs.
This means that each polling agent may see the counters at different points in time. I don't have a strong objection to serving counters + gauges from the metrics snapshot, but would prefer to increase the refresh frequency.
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.
Ack. I'm fine with it the way it is -- was mostly just trying to wrap my head around the semantics of what to expect.
Updates metriken library to replace heatmaps with histograms which have better runtime performance characteristics and lower memory footprint.