Add functions that instruments http handler using promhttp #15

tifayuki · 2017-11-10T00:45:10Z

In the latest prometheus go client, handlers provided before in prometheus package are deprecated. Instead, promhttp package is recommended.

This PR implements functions that wraps original methods inpromhttp.

It also fix the compilation error, when building against prometheus/client-golang master.

tifayuki · 2017-11-13T23:52:29Z

@stevvooe
can you please review this PR, I would like to use it in docker/distribution to provide http prometheus metrics

cc @manishtomar

tifayuki · 2017-11-14T18:58:24Z

cc @nandhini915

stevvooe · 2017-11-16T22:40:24Z

cc @crosbymichael

stevvooe · 2017-11-16T22:47:10Z

handler.go

+
+// InstrumentHandlerFuncWithOpts instruments http handler with in-flight gauge,
+// request count, handle duration, request size, and response size
+func InstrumentHandlerFuncWithOpts(opts HttpHandlerOpts, handlerFunc func(http.ResponseWriter, *http.Request)) http.HandlerFunc {


This should take the handler type, not the function type. Also, it should return a handler.

@stevvooe The deprecated functions uses http.HandlerFunc. I can added another wrapper that uses/returns a handler type. Do we need that ?

I see. http.HandlerFunc is an implementation of http.Handler. I would generally only deal in http.Handler for exported types, but I can see this is a specific function. Doing it the other way around would be better, since it is odd to take a method value:

func InstrumentHandlerWithOpts(ops HTTPHandlerOpts, handler http.Handler) http.Handler { // body of this function. } func InstrumentHandlerFuncWithOpts(opts HttpHandlerOpts, handlerFunc func(http.ResponseWriter, *http.Request)) http.Handler { return InstrumentHandlerWithOpts(opts, http.HandlerFunc(handlerFunc)) }

stevvooe · 2017-11-16T22:48:17Z

handler.go

+	inFlightGauge := prometheus.NewGauge(prometheus.GaugeOpts{
+		Namespace: opts.Namespace,
+		Subsystem: opts.Subsystem,
+		Name:      fmt.Sprintf("%s_in_flight_requests", opts.HandlerName),


Shouldn't handler names be labels?

I agree that using it is better to use labels. However the promhttp API only allows "code" and "method" as labels:
https://godoc.org/github.com/prometheus/client_golang/prometheus/promhttp#InstrumentHandlerCounter

Label the counter before you pass it to that function. Is that possible?

The issue here is there is nothing to enforce naming standard on the handler name.

I can give a try to see if we can label the counter. Well, not only counter, but almost all the APIs only accept "code" and "method" as labels.

If the handler name is not standard, say use "blob-upload" instead of "blob_upload" the API will panic at run time.

Ok, these are fine then. It would be a lot easier to service-wide rollups if these are labels, but you'd have to rewrite all of these.

stevvooe · 2017-11-16T22:51:52Z

Looks like this is copy pasted from https://github.com/prometheus/client_golang/blob/master/prometheus/http.go#L232. Why not use the standard layout?

stevvooe · 2017-11-16T22:54:03Z

Answered my own question in godoc: "- It uses Summaries rather than Histograms. Summaries are not useful if aggregation across multiple instances is required.".

stevvooe · 2017-11-16T22:58:17Z

handler.go

+	}
+	responseSizeHistogram := prometheus.NewHistogramVec(responseSizeHistogramOpts, []string{})
+
+	prometheus.MustRegister(inFlightGauge, requestTotal, requestDurationHistogram, requestSizeHistogram, responseSizeHistogram)


This should register with the go-metrics registry.

stevvooe · 2017-11-16T22:58:32Z

handler.go

+	if len(responseSizeHistogramOpts.Buckets) == 0 {
+		responseSizeHistogramOpts.Buckets = defaultResponseSizeBucket
+	}
+	responseSizeHistogram := prometheus.NewHistogramVec(responseSizeHistogramOpts, []string{})


Do these need to define the buckets?

The buckets can be defined in HttpHandlerOpts. If not provided, it uses the default bucket: defaultResponseSizeBucket = prometheus.ExponentialBuckets(1024, 2, 22) //1K to 4G

stevvooe · 2017-11-16T23:06:15Z

handler.go

+}
+
+// HttpHandlerOpts describes a set of configurable options of http metrics
+type HttpHandlerOpts struct {


Please follow Go style guidelines: HTTPHandlerOpts.

stevvooe · 2017-11-16T23:09:39Z

timer.go

@@ -36,7 +36,7 @@ type labeledTimer struct {
 }

 func (lt *labeledTimer) WithValues(labels ...string) Timer {
-	return &timer{m: lt.m.WithLabelValues(labels...)}
+	return &timer{m: lt.m.WithLabelValues(labels...).(prometheus.Histogram)}


What is going on here? Was there a problem?

yes, due to the api change, it cannot compile against prom master now:
#12

Please send a separate PR. We may need to change the field type instead of adding a type assertion.

stevvooe · 2017-11-16T23:10:34Z

Ok, this generally looks okay.

@cpuguy83 Did we instrument the moby api in prometheus yet? We should probably use this functions, if we can.

cpuguy83 · 2017-11-17T20:43:49Z

.gitignore

@@ -0,0 +1 @@
+.idea/


Shouldn't this go into ~/.gitconfig?

cpuguy83 · 2017-11-17T21:24:12Z

I'm not crazy about the API here.
Name at least should be a func argument in all cases, this would make the split WithOpts and no-opts functions a bit more useful.

Should this panic if name is empty?
Should this also be tied to a namespace rather than making the namespace optional?

tifayuki · 2017-11-17T23:09:28Z

I have pushed another commit with the following changes:

remove the .gitignore file
rename HttHandlerOpts to HTTPHandlerOpts
add namespace paramenter to non-WithOpts functions
panic if namespace or handlerName is not given
use handler name as constant label, avoiding use them as the name of a metrics
use register from go-metrics instead of MustRegister from prometheus/client-golang

cpuguy83 · 2017-11-20T15:33:44Z

@tifayuki I really meant that it would be good to bind the HTTP instrumentation helpers to the *Namespece type and that is one less thing that needs to be passed in.

stevvooe · 2017-11-21T01:35:46Z

@tifayuki @cpuguy83 Yes, these should be a function of the namespace. All metrics should descend from the namespace, per this package.

If that is not done, it makes these additions a little redundant.

tifayuki · 2017-11-30T01:21:34Z

@stevvooe @cpuguy83
I have re-implemented the http handlers using namespace. Can you take a look?

tifayuki · 2017-12-06T22:22:39Z

@stevvooe
Can you please review the PR again? Need it to be merged so that we can use it to provide metrics in docker/distribution
Thank you

stevvooe · 2018-01-30T22:50:08Z

namespace.go

+
+func (n *Namespace) NewHttpMetricsWithOpts(opts HttpHandlerOpts) []*HttpMetric {
+	if _, ok := n.labels["handler"]; !ok {
+		panic("label \"handler\" must be provided in the namespace")


Was this part of the original interface?

Ok, so it looks like we need a specialized constructor that creates this for the particular handler. Looking at the example, it should not be required to construct the namespace with a specific label. That should be part of creating the handler metric.

stevvooe · 2018-01-30T23:54:27Z

namespace.go

+
+func (n *Namespace) NewResponseSizeMetric(buckets []float64) *HttpMetric {
+	if len(buckets) == 0 {
+		panic("ResponseSizeBuckets must be provided")


If not provided, use a default.

stevvooe · 2018-01-30T23:55:49Z

handler.go

+	InstrumentHandlerInFlight
+)
+
+type HttpMetric struct {


HTTPMetric

Signed-off-by: tifayuki <tifayuki@gmail.com>

stevvooe · 2018-02-07T00:54:02Z

namespace.go

+		},
+		[]string{"code", "method"},
+	)
+	httpMetric := &HTTPMetric{metric, InstrumentHandlerCounter}


Make sure to specify the fields that these are getting set to.

Make sure to fix this one.

stevvooe · 2018-02-09T00:06:20Z

namespace.go

+		ConstLabels: prometheus.Labels(labels),
+	}
+	metric := prometheus.NewHistogramVec(opts, []string{"method"})
+	httpMetric := &HTTPMetric{metric, InstrumentHandlerDuration}


Make sure to fix this one.

stevvooe · 2018-02-09T00:06:31Z

namespace.go

+		ConstLabels: prometheus.Labels(labels),
+	}
+	metric := prometheus.NewHistogramVec(opts, []string{})
+	httpMetric := &HTTPMetric{metric, InstrumentHandlerRequestSize}


Fix this one.

Signed-off-by: tifayuki <tifayuki@gmail.com>

stevvooe · 2018-02-09T00:36:23Z

handler.go

+}
+
+var (
+	defaultDurationBuckets     = []float64{0.05, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1., 2., 5., 10., 20., 30., 40., 50.}


[]float64{.005, .01, .025, .05, .1, .25, .5, 1, 2.5, 5, 10, 25, 60}

Note that I went with 60, instead of 50 at the top end.

Signed-off-by: tifayuki <tifayuki@gmail.com>

stevvooe · 2018-02-09T01:25:17Z

LGTM

full diff: docker/go-metrics@4ea375f...v0.0.1 - docker/go-metrics#15 Add functions that instruments http handler using promhttp - docker/go-metrics#20 Rename LICENSE.code → LICENSE - docker/go-metrics#22 Support Go Modules Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

bump docker/go-metrics v0.0.1: full diff: docker/go-metrics@d466d4f...v0.0.1 - docker/go-metrics#16 fix the compilation error against prometheus/client-golang master - fixes docker/go-metrics#12 No longer builds against Prom master - docker/go-metrics#18 metrics: address compile error correctly - fixes docker/go-metrics#12 No longer builds against Prom master - docker/go-metrics#15 Add functions that instruments http handler using promhttp - docker/go-metrics#20 Rename LICENSE.code → LICENSE - docker/go-metrics#22 Support Go Modules bump prometheus/client_golang v0.9.4: full diff: prometheus/client_golang@c5b7fcc...v0.9.4 version v0.9.0 is the minimum required version to work with go-metrics v0.0.1, as it depends on `prometheus.Observer`: vendor/github.com/docker/go-metrics/timer.go:39:4: undefined: prometheus.Observer Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

bump docker/go-metrics v0.0.1: full diff: docker/go-metrics@d466d4f...v0.0.1 - docker/go-metrics#16 fix the compilation error against prometheus/client-golang master - fixes docker/go-metrics#12 No longer builds against Prom master - docker/go-metrics#18 metrics: address compile error correctly - fixes docker/go-metrics#12 No longer builds against Prom master - docker/go-metrics#15 Add functions that instruments http handler using promhttp - docker/go-metrics#20 Rename LICENSE.code → LICENSE - docker/go-metrics#22 Support Go Modules bump prometheus/client_golang v0.9.4: full diff: prometheus/client_golang@c5b7fcc...v0.9.4 version v0.9.0 is the minimum required version to work with go-metrics v0.0.1, as it depends on `prometheus.Observer`: vendor/github.com/docker/go-metrics/timer.go:39:4: undefined: prometheus.Observer Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: b2db7c8bc967b52609877f5d856c338ea51e4c46 Component: engine

full diff: docker/go-metrics@4ea375f...v0.0.1 - docker/go-metrics#15 Add functions that instruments http handler using promhttp - docker/go-metrics#20 Rename LICENSE.code → LICENSE - docker/go-metrics#22 Support Go Modules Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

tifayuki force-pushed the promhttp branch from 24357ed to d969d27 Compare November 10, 2017 23:54

tifayuki force-pushed the promhttp branch from d969d27 to 4f039ad Compare November 16, 2017 19:42

stevvooe reviewed Nov 16, 2017

View reviewed changes

cpuguy83 reviewed Nov 17, 2017

View reviewed changes

.gitignore Outdated

@@ -0,0 +1 @@

.idea/

Copy link

cpuguy83 Nov 17, 2017

Choose a reason for hiding this comment

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

Shouldn't this go into ~/.gitconfig?

tifayuki force-pushed the promhttp branch from 4f039ad to 22916a0 Compare November 17, 2017 23:07

tifayuki force-pushed the promhttp branch 3 times, most recently from c6e608f to bb4f080 Compare November 30, 2017 01:19

tifayuki mentioned this pull request Nov 30, 2017

fix the compilation error against prometheus/client-golang master #16

Merged

tifayuki mentioned this pull request Dec 7, 2017

add prometheus metrics distribution/distribution#2466

Merged

stevvooe reviewed Jan 30, 2018

View reviewed changes

handler.go Outdated

InstrumentHandlerInFlight

)

type HttpMetric struct {

Copy link

Contributor

stevvooe Jan 30, 2018

Choose a reason for hiding this comment

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

HTTPMetric

tifayuki added 2 commits January 31, 2018 15:49

implement http instrumented handler using promhttp

5ef4ea1

Signed-off-by: tifayuki <tifayuki@gmail.com>

changes according to the reviews

be4fb1c

Signed-off-by: tifayuki <tifayuki@gmail.com>

tifayuki force-pushed the promhttp branch from bb4f080 to be4fb1c Compare February 2, 2018 23:51

stevvooe reviewed Feb 7, 2018

View reviewed changes

tifayuki force-pushed the promhttp branch from 9a480fd to 465e8ca Compare February 7, 2018 22:16

stevvooe reviewed Feb 9, 2018

View reviewed changes

initialize HTTPMetrics with fields specified

a3315a9

Signed-off-by: tifayuki <tifayuki@gmail.com>

tifayuki force-pushed the promhttp branch from 465e8ca to a3315a9 Compare February 9, 2018 00:11

stevvooe reviewed Feb 9, 2018

View reviewed changes

udpate the default value of DurationBuckets

86f91d0

Signed-off-by: tifayuki <tifayuki@gmail.com>

stevvooe merged commit 399ea8c into docker:master Feb 9, 2018

thaJeztah mentioned this pull request Jan 13, 2020

Bump prometheus/client_golang and dependencies v1.3.0 moby/moby#40371

Merged

1 task

thaJeztah mentioned this pull request Jan 13, 2020

Bump prometheus/client_golang and dependencies v1.3.0 containerd/containerd#3959

Merged

Add functions that instruments http handler using promhttp #15

Add functions that instruments http handler using promhttp #15

Conversation

tifayuki commented Nov 10, 2017

tifayuki commented Nov 13, 2017

tifayuki commented Nov 14, 2017

stevvooe commented Nov 16, 2017

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

tifayuki Nov 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Nov 16, 2017

stevvooe commented Nov 16, 2017

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

stevvooe commented Nov 16, 2017

Choose a reason for hiding this comment

cpuguy83 commented Nov 17, 2017

tifayuki commented Nov 17, 2017

cpuguy83 commented Nov 20, 2017

stevvooe commented Nov 21, 2017

tifayuki commented Nov 30, 2017

tifayuki commented Dec 6, 2017

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

stevvooe commented Feb 9, 2018

tifayuki Nov 16, 2017 •

edited