jaegertracing / jaeger-client-python

🛑 This library is DEPRECATED!
https://jaegertracing.io/
Apache License 2.0
408 stars 155 forks source link

Allow adding 'service' label to Prometheus metrics #269

Closed alexppg closed 4 years ago

alexppg commented 4 years ago

Add support for the prometheus metrics to have a label called service, which can be used to identify every microservice.

Resolves #268

alexppg commented 4 years ago

Hey! Thanks for the comments. Why you want to default it to none? I've default it to an empty string because is the same is done with namespace. Also, why you say it's assumed that it exists? It doesn't. @yurishkuro.

yurishkuro commented 4 years ago

I don't know why namespace is defaulted to empty string, maybe it was a requirement of Prom client, or just an oversight. We don't need to follow that pattern. None is the idiomatic way to indicate that the parameter was not passed, especially when the receiver is planning not to use it in that case.

The reason I say it's "required" in the current PR is because the label is always added to the metric, regardless of the value passed. To be backwards compatible it should only be added if the value is not None.

codecov[bot] commented 4 years ago

Codecov Report

Merging #269 into master will increase coverage by 0.11%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #269      +/-   ##
==========================================
+ Coverage   94.71%   94.83%   +0.11%     
==========================================
  Files          25       25              
  Lines        1931     1935       +4     
  Branches      259      260       +1     
==========================================
+ Hits         1829     1835       +6     
+ Misses         67       66       -1     
+ Partials       35       34       -1
Impacted Files Coverage Δ
jaeger_client/metrics/prometheus.py 100% <100%> (ø) :arrow_up:
jaeger_client/throttler.py 96.36% <0%> (+1.81%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 900c509...14accc4. Read the comment docs.

alexppg commented 4 years ago

Thanks for your explanations! I've just pushed most of the changes. I haven't pushed the one you say to make it DRYer. The prometheus metrics needs the labels when declaring the object (gauge or counter) and also when updating the metric. So, since both the declaration and the update happens in the create_counter and create_gauge, I think it's the only way to do it, but if you see another way, I'll do it gladly.

yurishkuro commented 4 years ago

Advice for the future: do not create pull requests from master branch of your fork, always create a feature branch first. Your master should ideally always match the upstream master.

alexppg commented 4 years ago

Ok, thank you very much for your advices and commits!

Edit: Is there an estimated time of the arrival of the new pypi package?

alexppg commented 4 years ago

ping @yurishkuro

yurishkuro commented 4 years ago

released 4.3.0

alexppg commented 4 years ago

Thanks!