-
Notifications
You must be signed in to change notification settings - Fork 89
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
impl TypedMetric for Counter/HistogramWithExemplars #96
impl TypedMetric for Counter/HistogramWithExemplars #96
Conversation
4157909
to
a4dcecc
Compare
9bd6291
to
7b0007c
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.
Oh, my bad. Thanks for the fix. Much appreciated.
examples/families-exemplars.rs
Outdated
@@ -0,0 +1,47 @@ | |||
use prometheus_client::{ |
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.
Thanks for also adding an example. I wonder whether people would easier discover this as a doc example on HistogramWithExemplars
or Family
(potentially one linking to the other). What do you think @adamchalmers?
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.
Sounds good, I'll remove this example and replace it with examples on the docs for CounterWithExemplar and HistogramWithExemplars.
Cargo.toml
Outdated
@@ -38,6 +38,9 @@ actix-web = "4" | |||
[build-dependencies] | |||
prost-build = { version = "0.9.0", optional = true } | |||
|
|||
[[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.
Cross-referencing question on whether this is needed: #94 (comment)
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.
Ah, it's not needed! I don't know why I thought it was. Removed anyway in favour of using examples on the docs.
👍 Yes, happy to cut a new release once this is merged, especially since that would publish protobuf support #83.
Wonderful feedback. Thank you ❤️ |
ce0de2d
to
c690316
Compare
Hmm, given that you're shipping protobuf support, you may want to include #98 in the next release too. |
c690316
to
8795179
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.
Thanks for the follow-ups. Two more comments. Otherwise ready to merge.
… both families and exemplars Fixes issue prometheus#95. Signed-off-by: Adam Chalmers <[email protected]>
4c34fb8
to
7150bba
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.
🙏
Fixes #95, i.e. that on master, you cannot create a Histogram or Counter which has both a Family and Exemplars. Also adds an example showing how to make such a histogram/counter. This example won't compile without my fix, so it also serves as a regression test :)
Running my example gives the expected output:
If you approve this PR, would you please also consider cutting a release 0.19 for it? I think most real-world exemplars will also want to use families, so I think this feature is worth releasing soon. My team at Cloudflare is gonna use my fork with this PR for now, but I'd like to start using this crate in more projects internally, and we try to avoid relying on Github forks :)
Thanks again for making this library. I've been using the old prometheus crate since 2018, and made a few PRs here and there, so I really appreciate you modernizing it and adding full OpenMetrics compliance.