Add first class support for Histograms #63

objectiser · 2018-12-05T16:41:57Z

Resolves #62

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

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

codecov · 2018-12-05T16:44:25Z

Codecov Report

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

@@          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%> (ø)`	⬆️
metrics/adapters/tagless.go	`100% <100%> (ø)`	⬆️
metrics/go-kit/factory.go	`100% <100%> (ø)`	⬆️
metrics/prometheus/factory.go	`100% <100%> (ø)`	⬆️
metrics/go-kit/metrics.go	`100% <100%> (ø)`	⬆️
metrics/tally/metrics.go	`100% <100%> (ø)`	⬆️
metrics/adapters/cache.go	`100% <100%> (ø)`	⬆️
metrics/histogram.go	`100% <100%> (ø)`
metrics/adapters/factory.go	`100% <100%> (ø)`	⬆️
metrics/expvar/factory.go	`100% <100%> (ø)`	⬆️
... 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 · 2018-12-05T16:45:35Z

@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 · 2018-12-05T16:53:12Z

metrics/factory.go

+	Help    string
+	Buckets []float64
+}
+
 // Factory creates new metrics
 type Factory interface {
 	Counter(metric Options) Counter
 	Timer(metric Options) Timer


my main question is: do we want to keep the Timer() as a function in the interface, or move it to a static helper function that will be based on the histogram?

Pros: This is what already happens in most implementations, so we could just do it once.
Cons: in some metrics backends timers are 1st class citizens

Your call 😄

so the only change I would propose is having the Timer() function take options struct that includes buckets (probably expressed as []time.Duration rather than []float64).

yurishkuro · 2018-12-05T16:55:48Z

I am ok with leaving times in the interface.

objectiser · 2018-12-05T17:00:14Z

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

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

objectiser · 2018-12-05T20:11:11Z

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

metrics/adapters/tagless.go

yurishkuro · 2018-12-06T18:23:18Z

metrics/expvar/factory.go

 	return xkit.NewTimer(f.factory.Histogram(name))
 }
+
+func (f *factory) Histogram(name string, help string, buckets []float64) metrics.Histogram {
+	return xkit.NewHistogram(f.factory.Histogram(name))


maybe a TODO to respect the buckets? Doesn't gokit allow buckets for histograms?

yurishkuro · 2018-12-06T18:24:15Z

metrics/factory.go

+import (
+	"time"
+)
+
 // NSOptions defines the name and tags map associated with a metric


s/with a metric/with a factory namespace/ ?

yurishkuro · 2018-12-06T18:26:23Z

metrics/histogram.go

@@ -0,0 +1,28 @@
+// Copyright (c) 2018 Uber Technologies, Inc.


The Jaeger Authors

yurishkuro · 2018-12-06T18:26:59Z

metrics/histogram.go

+
+package metrics
+
+// Histogram represents a histogram metric types.


that keeps track of a distribution of values.

yurishkuro · 2018-12-06T18:30:05Z

metrics/metrics.go

@@ -47,6 +48,7 @@ func InitOrError(m interface{}, factory Factory, globalTags map[string]string) e
 	counterPtrType := reflect.TypeOf((*Counter)(nil)).Elem()


maybe we should fix #64 while at it

I think InitOrError should be renamed to Init()

metrics/metrics.go

yurishkuro · 2018-12-06T18:35:55Z

metrics/prometheus/factory.go

+func asFloatBuckets(buckets []time.Duration) []float64 {
+	data := make([]float64, len(buckets))
+	for i := range data {
+		data[i] = float64(buckets[i])


doesn't Prometheus client expects double(1.0) == 1sec? Do we need:

data[i] = float64(buckets[i]) / float64(time.Second)

?

Possibly for Timer (duration) buckets - but for Histogram this is just a list of floats,

but this function is used for Timer buckets, is it not? Since it accepts []time.Duration. I think you need to divide this by time.Second, since this is what the Record function of the timer does:

jaeger-lib/metrics/prometheus/factory.go

Lines 214 to 216 in 3fa9aa0

func (t *timer) Record(v time.Duration) {

t.histogram.Observe(float64(v.Nanoseconds()) / float64(time.Second/time.Nanosecond))

}

Yep sorry - not sure what I was thinking.

yurishkuro · 2018-12-06T18:39:06Z

metrics/tally/factory.go

@@ -48,14 +48,22 @@ func (f *factory) Gauge(options metrics.Options) metrics.Gauge {
 	return NewGauge(scope.Gauge(options.Name))
 }

-func (f *factory) Timer(options metrics.Options) metrics.Timer {
+func (f *factory) Timer(options metrics.TimerOptions) metrics.Timer {
 	scope := f.tally
 	if len(options.Tags) > 0 {
 		scope = scope.Tagged(options.Tags)
 	}
 	return NewTimer(scope.Timer(options.Name))


hm, I thought tally supported buckets for timers. At least let's have a TODO, since we're dropping the buckets.

No, Timer doesn't, but the tally.Histogram suports a DurationBucket - so depends if we want to switch underlying representation? Will mark as TODO for now.

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

objectiser · 2018-12-07T11:25:29Z

Updated.
Resolves #64

objectiser · 2018-12-10T10:32:22Z

@yurishkuro Ready for another review.

objectiser · 2018-12-12T18:28:00Z

@yurishkuro Anything further required on this one?

yurishkuro · 2018-12-12T18:37:23Z

metrics/metrics.go

+					b, err := strconv.ParseFloat(bucket, 64)
+					if err != nil {
+						return fmt.Errorf(
+							"Field [%s]: Bucket [%s] could not be converted to float64 in 'buckets' stirng [%s]",


typo: stirng

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

objectiser · 2018-12-12T19:05:12Z

@yurishkuro updated.

yurishkuro · 2018-12-13T00:13:48Z

🎉

Add first class support for Histograms

3a2d38f

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

objectiser changed the title ~~Add first class support for Histograms~~ WIP: Add first class support for Histograms Dec 5, 2018

objectiser requested a review from yurishkuro December 5, 2018 16:42

yurishkuro reviewed Dec 5, 2018

View reviewed changes

objectiser added 3 commits December 5, 2018 19:20

Define TimerOptions with buckets array of type time.Duration

27e5c97

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

Fix test

d868613

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

Fix coverage

d2bd500

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

objectiser changed the title ~~WIP: Add first class support for Histograms~~ Add first class support for Histograms Dec 5, 2018

yurishkuro reviewed Dec 6, 2018

View reviewed changes

objectiser mentioned this pull request Dec 7, 2018

Parse timer duration buckets obtained through reflection #65

Open

Updates following review comments

291e539

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

This was referenced Dec 7, 2018

Expvar Histogram should respect buckets #66

Open

Investigate buckets support for Tally Timer #67

Open

yurishkuro reviewed Dec 12, 2018

View reviewed changes

Update based on review comments

7b4ca24

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

yurishkuro approved these changes Dec 13, 2018

View reviewed changes

yurishkuro merged commit c323da6 into jaegertracing:master Dec 13, 2018

objectiser deleted the histo branch December 13, 2018 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add first class support for Histograms #63

Add first class support for Histograms #63

objectiser commented Dec 5, 2018

codecov bot commented Dec 5, 2018 •

edited

Loading

objectiser commented Dec 5, 2018

yurishkuro Dec 5, 2018

objectiser Dec 5, 2018

yurishkuro Dec 5, 2018

yurishkuro commented Dec 5, 2018

objectiser commented Dec 5, 2018

objectiser commented Dec 5, 2018

yurishkuro Dec 6, 2018

objectiser Dec 7, 2018 •

edited

Loading

yurishkuro Dec 6, 2018

yurishkuro Dec 6, 2018

yurishkuro Dec 6, 2018

yurishkuro Dec 6, 2018

yurishkuro Dec 12, 2018

objectiser Dec 12, 2018

yurishkuro Dec 6, 2018

objectiser Dec 7, 2018

yurishkuro Dec 12, 2018

objectiser Dec 12, 2018

yurishkuro Dec 6, 2018

objectiser Dec 7, 2018

objectiser Dec 7, 2018

objectiser commented Dec 7, 2018 •

edited

Loading

objectiser commented Dec 10, 2018

objectiser commented Dec 12, 2018

yurishkuro Dec 12, 2018

objectiser Dec 12, 2018

objectiser commented Dec 12, 2018

yurishkuro commented Dec 13, 2018

		@@ -0,0 +1,28 @@
		// Copyright (c) 2018 Uber Technologies, Inc.


		package metrics

		// Histogram represents a histogram metric types.

		@@ -47,6 +48,7 @@ func InitOrError(m interface{}, factory Factory, globalTags map[string]string) e
		counterPtrType := reflect.TypeOf((*Counter)(nil)).Elem()

	func (t *timer) Record(v time.Duration) {
	t.histogram.Observe(float64(v.Nanoseconds()) / float64(time.Second/time.Nanosecond))
	}

Add first class support for Histograms #63

Add first class support for Histograms #63

Conversation

objectiser commented Dec 5, 2018

codecov bot commented Dec 5, 2018 • edited Loading

Codecov Report

objectiser commented Dec 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Dec 5, 2018

objectiser commented Dec 5, 2018

objectiser commented Dec 5, 2018

Choose a reason for hiding this comment

objectiser Dec 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

objectiser commented Dec 7, 2018 • edited Loading

objectiser commented Dec 10, 2018

objectiser commented Dec 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

objectiser commented Dec 12, 2018

yurishkuro commented Dec 13, 2018

codecov bot commented Dec 5, 2018 •

edited

Loading

objectiser Dec 7, 2018 •

edited

Loading

objectiser commented Dec 7, 2018 •

edited

Loading