search

jaegertracing / jaeger-lib

A collection of shared infrastructure libraries used by different components of Jaeger.
https://github.com/uber/jaeger
Apache License 2.0
67 stars 34 forks source link

Add first class support for Histograms #63

Closed objectiser closed 5 years ago

objectiser commented 5 years ago

Resolves #62

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

codecov[bot] commented 5 years ago

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #63    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          25     26     +1     
  Lines         711    890   +179     
======================================
+ Hits          711    890   +179
Impacted Files Coverage Δ
metrics/factory.go 100% <100%> (ø) :arrow_up:
metrics/adapters/tagless.go 100% <100%> (ø) :arrow_up:
metrics/go-kit/factory.go 100% <100%> (ø) :arrow_up:
metrics/prometheus/factory.go 100% <100%> (ø) :arrow_up:
metrics/go-kit/metrics.go 100% <100%> (ø) :arrow_up:
metrics/tally/metrics.go 100% <100%> (ø) :arrow_up:
metrics/adapters/cache.go 100% <100%> (ø) :arrow_up:
metrics/histogram.go 100% <100%> (ø)
metrics/adapters/factory.go 100% <100%> (ø) :arrow_up:
metrics/expvar/factory.go 100% <100%> (ø) :arrow_up:
... and 5 more

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 3fa9aa0...7b4ca24. Read the comment docs.

objectiser commented 5 years ago

@yurishkuro Currently just adds Histogram alongside Timer - although your suggestion in #62 to replace Timer, was wondering whether would be better to leave Timer as-is - to minimise breaking changes, but also because the Timer reports durations, whereas the histogram reports floats.

yurishkuro commented 5 years ago

I am ok with leaving times in the interface.

objectiser commented 5 years ago

@yurishkuro Ok will work on the test coverage and fixing the failing test. Let me know if there are other areas that need addressing.

objectiser commented 5 years ago

@yurishkuro Updated to use TimerOptions and fixed coverage. Let me know if there is anything else.

objectiser commented 5 years ago

Updated. Resolves #64

objectiser commented 5 years ago

@yurishkuro Ready for another review.

objectiser commented 5 years ago

@yurishkuro Anything further required on this one?

objectiser commented 5 years ago

@yurishkuro updated.

yurishkuro commented 5 years ago

🎉