-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add Summary metric type #67
base: master
Are you sure you want to change the base?
Conversation
sorry for the radio silence on this one, I was moving to another city and completely forgot about it, will try to get back to this next week. |
dc2cba3
to
1455fcc
Compare
c4a57ff
to
6458eb3
Compare
sorry i completely forgot about this PR @mxinden can you please take a look |
6458eb3
to
a0d7e4d
Compare
Signed-off-by: Palash Nigam <npalash25@gmail.com>
a0d7e4d
to
29661c0
Compare
i will add some comments in the |
oh you are right, i missed the proto.rs file thanks for pointing it out, i will try to make the remaining changes in the next 2 days, if I can't i will ping you for help. |
moving this back to draft till i make the missing changes |
6af2018
to
e038f4e
Compare
sorry for the late response @mxinden the notification got buried. I tried to make the changes according to the new encoding API but I am stuck here. Right now my code errors with this
if I do as the compiler says and add this annotation Do you know what I might be doing wrong here? |
Hi @mxinden could you take a look at the comment? ☝🏽 |
Signed-off-by: Palash Nigam <npalash25@gmail.com>
e038f4e
to
9462cd6
Compare
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.
Sorry for the delay here. Implementation of encoding in text and protobuf looks good to me. Couple minor comments.
Before I dive deeper into the implementation of Summary, can you expand on why you chose CKMS and how it differs to the golang implementation?
@beorn7 given that you wrote most of the client-golang Summary implementation and given your involvement in OpenObservability/OpenMetrics#256 I would appreciate your input here. Do you have opinions on which quantile algorithm to use? |
so about using CKMS, i went through both go and java libraries, the underlying quantile library that go uses has some inacurracies mentioned here https://github.com/beorn7/perks/blob/master/quantile/stream.go#L174 so I looked at the java client implementation and it seemed to be using a CKMS library https://github.com/prometheus/client_java/blob/c28b901225e35e7c1df0eacae8b58fdfbb390162/simpleclient/src/main/java/io/prometheus/client/TimeWindowQuantiles.java#L14 at the same time I was able to find a rust crate implementing CKMS so I just went ahead with it. Thats all I can remember since it was a long time ago
Not sure I remember the differences but this is what I wrote in my notes at the time which seems to be a similarity between the go library and the rust crate: "Rust crate uses the LowBiased invariant defined in the golang one quantiles/store.rs at master · blt/quantiles · GitHub" |
tl;dr: Use whatever algorithm you find appropriate. It would require a lot of research to assess all the algorithms out there and pick the "best" one for client_rust. The algorithm client_golang uses was picked even before my time. I don't have strong opinions about it. It's perfectly possible that there are "better" algorithms out there. It has been a hot topic of research for a while, so there are many algorithms available. It's of course also hard to decide about the "best" algorithm because that surely depends on your use case. There was an attempt to add a now algorithm or replace the existing one, see prometheus/client_golang#859 . It was closed for lack of follow-up when we tried to discuss the aspects in which it would be a better algorithm. I do believe that the algorithm in client_golang is implemented correctly (i.e. as it is described in the paper). client_golang uses my fork, which includes fixes. The |
Signed-off-by: Palash Nigam <npalash25@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.
pub fn observe(&self, v: f64) { | ||
self.rotate_buckets(); | ||
|
||
let mut inner = self.inner.write(); |
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.
Both rotate_buckets
and later on observe
itself grab the write lock. The lock will be unlocked in between. Thus another thread could potentially execute in between. Does that violate any consistency guarantees. In other words does observe
depend on rotate_buckets
to be called right before?
See similar race condition in #102.
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.
Are we blocked by this issue?
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.
@cz-cube definitely needs some investigation.
@@ -24,6 +24,7 @@ parking_lot = "0.12" | |||
prometheus-client-derive-encode = { version = "0.4.1", path = "derive-encode" } | |||
prost = { version = "0.11.0", optional = true } | |||
prost-types = { version = "0.11.0", optional = true } | |||
quantiles = "0.7.1" |
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 would guess that most users of this library don't need the Summary
type. What do you think of feature flagging the type as well as the quantiles
dependency. The latter will help keeping ones dependency tree lean, thus compile times short.
use super::{MetricType, TypedMetric}; | ||
use parking_lot::RwLock; | ||
use std::sync::Arc; | ||
use std::time::{Duration, Instant}; |
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.
std::time::Instant
is problematic with wasm. Consider using https://crates.io/crates/instant instead.
} | ||
|
||
/// Retrieve the values of the summary metric. | ||
pub fn get(&self) -> (f64, u64, Vec<(f64, f64)>) { |
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.
pub fn get(&self) -> (f64, u64, Vec<(f64, f64)>) { | |
pub(crate) fn get(&self) -> (f64, u64, Vec<(f64, f64)>) { |
Is this needed outside of the encode module?
let inner = self.inner.read(); | ||
let sum = inner.sum; | ||
let count = inner.count; | ||
let mut quantile_values: Vec<(f64, f64)> = Vec::new(); |
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.
Instead of allocating, you could consider returning an Iterator
instead. Though likely premature optimization without any proof that this is an issue.
quantile_streams: Vec<CKMS<f64>>, | ||
// head_stream is like a cursor which carries the index | ||
// of the stream in the quantile_streams that we want to query. | ||
head_stream_idx: u64, |
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.
head_stream_idx: u64, | |
head_stream_idx: usize, |
Why not use a usize
here as it is an index into a datastructure? You compare it to max_age_buckets
, but that could be a usize
as well.
We want to observe multiple sample at once. Please consider adding |
I am sorry for the inactivity here. I have not been in the best of health for the past few months. Please feel free to take over this PR and make whatever changes are required. You can even close and let someone else have a crack at it if they want to start from scratch. |
Not ready for review yet, just opening a draft for visibility
Closes: #40