-
Notifications
You must be signed in to change notification settings - Fork 80
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
src/metrics/histogram.rs Document no default buckets #63
Conversation
As suggested and discussed in prometheus#61, this commit adds a comment that describes unavailability of default values for buckets. Signed-off-by: Doehyun Baek <doehyunbaek@gmail.com>
I think a doc test showcasing the default values from the Go implementation, with a link to the Go code would be great. |
Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Doehyun Baek <doehyunbaek@gmail.com>
Was a little sad I could not use array.into_iter() but used IntoIterator::into_iter(array) instead. Thought about asserting with Histogram struct for doctest but seemed gnarly and settled for simple .observe() call as exponential_buckets example. |
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.
Could you wrap the lines?
src/metrics/histogram.rs
Outdated
@@ -9,6 +9,17 @@ use std::sync::{Arc, Mutex, MutexGuard}; | |||
|
|||
/// Open Metrics [`Histogram`] to measure distributions of discrete events. | |||
/// | |||
/// [`Histogram`] does not implement [`Default`], given that the choice of bucket values depends on the situation [`Histogram`] is used in. As an example, to measure HTTP request latency, the values suggested in the Golang implementation might work for you: |
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.
How about moving this section below the vanilla doc example (line 12 / 23)?
Oh, haven't looked deeply into this. Shouldn't https://doc.rust-lang.org/std/primitive.array.html#impl-IntoIterator |
I should have explained more specifically.. Sorry for that. Arrays have special behavior prior to 2021 edition. If we use 2021 edition, array.into_iter would work, if not, we should resort to Intoiterator::into_iter(array) to iterate by value. |
Signed-off-by: Doehyun Baek <doehyunbaek@gmail.com>
Done! |
Oh, right. Thanks for the links. Given that editions don't split the ecosystem what do you think of moving Sorry for derailing your initial proposal here. While not directly related, I think this is very valuable. |
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.
🙏 thank you for the help.
Agreed. I will work on edition 2021 PR this week. |
As suggested and discussed in prometheus#61, this commit adds a comment that describes unavailability of default values for buckets. Signed-off-by: Doehyun Baek <doehyunbaek@gmail.com> Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
As suggested and discussed in #61,
this commit adds a comment that describes unavailability of default
values for buckets.
I thought about providing test case that demonstrates using iterator for buckets as arguments. But thought that would be a bit overkill. If you think it would help let me know.