client_rust
client_rust copied to clipboard
prometheus
Add Summary metric type
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.
sorry i completely forgot about this PR @mxinden can you please take a look
@palash25 do you need a hand here? I'm also wanting this metric, I started implementing it myself before I saw your PR. Looks like you still need to implement changes to proto.rs, can I help with this?
@mxinden in your opinion is anything besides this needed here before releasing?
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.
@palash25 #105 introduced a fairly large change. I am sorry for the conflict. Let me know in case you need any help updating.
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
error[E0282]: type annotations needed
--> src/encoding.rs:145:15
|
145 | e.encode_summary(sum, count, quantiles)
| ^^^^^^^^^^^^^^ cannot infer type of the type parameter `S` declared on the associated function `encode_summary`
|
help: consider specifying the generic argument
|
145 | e.encode_summary::<S>(sum, count, quantiles)
| +++++
if I do as the compiler says and add this annotation e.encode_summary::<S>(sum, count, quantiles) all the tests start passing but then my code looks a bit inconsistent in comparison to other encode_* methods in the file.
Do you know what I might be doing wrong here?
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 https://github.com/OpenObservability/OpenMetrics/pull/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
how it differs to the golang implementation?
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 https://github.com/prometheus/client_golang/pull/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 Merge method @palash25 has mentioned above is still broken, and it cannot be fixed, because the used algorithm doesn't support merging. But that method is also not used in client_golang.
We want to observe multiple sample at once. Please consider adding fn observe(value: f64, times: usize).
