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

Add _total suffix for counters reported to prometheus #54

Merged
merged 2 commits into from
Oct 30, 2018

Conversation

objectiser
Copy link
Contributor

Which problem is this PR solving?

As discussed in the prometheus documentation, "an accumulating count has total as a suffix".

Short description of the changes

If the counter does not have the _total suffix, then it will be added.

This change will also be required to bring the Go tracer inline with the metrics being reported by the Java tracer, which is using the micrometer library that applies a similar transformation on the counter metric names. See jaegertracing/jaeger-client-java#564.

Signed-off-by: Gary Brown gary@brownuk.com

Signed-off-by: Gary Brown <gary@brownuk.com>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Lgtm but we need to decide about the earlier breaking change before we can upgrade go client.

@codecov
Copy link

codecov bot commented Oct 30, 2018

Codecov Report

Merging #54 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #54   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          26     26           
  Lines         719    723    +4     
=====================================
+ Hits          719    723    +4
Impacted Files Coverage Δ
metrics/prometheus/factory.go 100% <100%> (ø) ⬆️

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 e0a812e...ae98683. Read the comment docs.

@yurishkuro yurishkuro merged commit 92cc65b into jaegertracing:master Oct 30, 2018
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.

2 participants