Simplify the Prometheus stuff #2878
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ndr-ds
reviewed
Nov 13, 2024
linera-base/src/prometheus_util.rs
Outdated
Comment on lines
41
to
76
| /// Construct the latencies starting from 0.0001 and ending at the maximum latency | ||
| pub fn bucket_latencies(max_latency: f64) -> Option<Vec<f64>> { | ||
| let mut latencies = Vec::new(); | ||
| let mut latency = 0.0001_f64; | ||
| let max_delta = 0.01; | ||
| loop { | ||
| for mult in [1.0_f64, 2.5_f64, 5.0_f64] { | ||
| let target_latency = latency * mult; | ||
| latencies.push(target_latency); | ||
| let delta = (target_latency - max_latency).abs() / max_latency; | ||
| if delta < max_delta { | ||
| return Some(latencies); | ||
| } | ||
| } | ||
| latency *= 10_f64; | ||
| } | ||
| } | ||
|
|
||
| /// Construct the bucket interval starting from a value and an ending value. | ||
| pub fn bucket_interval(start_value: f64, end_value: f64) -> Option<Vec<f64>> { | ||
| let mut values = Vec::new(); | ||
| let mut value = start_value; | ||
| let max_delta = 0.01; | ||
| loop { | ||
| for mult in [1.0_f64, 2.5_f64, 5.0_f64] { | ||
| let target_value = value * mult; | ||
| values.push(target_value); | ||
| let delta = (target_value - end_value).abs() / end_value; | ||
| if delta < max_delta { | ||
| return Some(values); | ||
| } | ||
| } | ||
| value *= 10_f64; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Prometheus already provides utility functions to generate these buckets, like I mentioned in the issue: #2795 (comment)
Couldn't we use those instead of implementing it from scratch?
There was a problem hiding this comment.
Yes, we can use those instead of this code.
However, the obtained buckets would be different, it would be a strictly exponential growth. Which may be fine.
I do not think anyone of us is using the bucket.
ndr-ds
approved these changes
Nov 19, 2024
Comment on lines
+43
to
+45
| let quot = end_value / start_value; | ||
| let factor = 3.0_f64; | ||
| let count_approx = quot.ln() / factor.ln(); |
Co-authored-by: Andre da Silva <[email protected]> Signed-off-by: Mathieu Dutour Sikiric <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The Prometheus entries is creating a lot of copy-pasted code which is bad taste.
Proposal
This PR does two things:
bucket_latenciesandbucket_intervalfor creating the bucket of values in a more organized way.Test Plan
The CI
Release Plan
The intervals have changed a bit. So, if the operation code strictly depends on the exact values of the buckets then
something else would have to be done.
Links
None.