Skip to content
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

Merged

Conversation

adamchalmers
Copy link
Contributor

@adamchalmers adamchalmers commented Sep 19, 2022

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:

# HELP latency help text goes here.
# TYPE latency histogram
latency_sum{result="success"} 0.001345422
latency_count{result="success"} 1
latency_bucket{result="success",le="1.0"} 1 # {trace_id="3a2f90c9f80b894f"} 0.001345422

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.

@adamchalmers adamchalmers changed the title Achalmers/exemplars families bug impl TypedMetric for Counter/HistogramWithExemplars Sep 19, 2022
CHANGELOG.md Show resolved Hide resolved
@adamchalmers adamchalmers force-pushed the achalmers/exemplars-families-bug branch 5 times, most recently from 9bd6291 to 7b0007c Compare September 19, 2022 22:34
Copy link
Member

@mxinden mxinden left a 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.

@@ -0,0 +1,47 @@
use prometheus_client::{
Copy link
Member

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?

Copy link
Contributor Author

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]]
Copy link
Member

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)

Copy link
Contributor Author

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.

@mxinden
Copy link
Member

mxinden commented Sep 21, 2022

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 :)

👍 Yes, happy to cut a new release once this is merged, especially since that would publish protobuf support #83.

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.

Wonderful feedback. Thank you ❤️

@adamchalmers adamchalmers force-pushed the achalmers/exemplars-families-bug branch 2 times, most recently from ce0de2d to c690316 Compare September 21, 2022 14:59
@adamchalmers
Copy link
Contributor Author

Hmm, given that you're shipping protobuf support, you may want to include #98 in the next release too.

Copy link
Member

@mxinden mxinden left a 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.

src/metrics/exemplar.rs Outdated Show resolved Hide resolved
src/metrics/exemplar.rs Outdated Show resolved Hide resolved
… both families and exemplars

Fixes issue prometheus#95.

Signed-off-by: Adam Chalmers <adam.s.chalmers@gmail.com>
@adamchalmers
Copy link
Contributor Author

OK, I think the indentation issue is fixed. Here's what the docs look like now.
histogram_with_exemplar
counter_with_exemplar

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

@mxinden mxinden merged commit 9e1e344 into prometheus:master Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: cannot use Exemplars and Families together
3 participants