search

docker / go-metrics

Package for metrics collection in Docker projects
Apache License 2.0
87 stars 32 forks source link

Add functions that instruments http handler using promhttp #15

Closed tifayuki closed 6 years ago

tifayuki commented 7 years ago

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 commented 6 years ago

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

cc @manishtomar

tifayuki commented 6 years ago

cc @nandhini915

stevvooe commented 6 years ago

cc @crosbymichael

stevvooe commented 6 years ago

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 commented 6 years ago

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

stevvooe commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

I have pushed another commit with the following changes: 1) remove the .gitignore file 2) rename HttHandlerOpts to HTTPHandlerOpts 3) add namespace paramenter to non-WithOpts functions 4) panic if namespace or handlerName is not given 5) use handler name as constant label, avoiding use them as the name of a metrics 6) use register from go-metrics instead of MustRegister from prometheus/client-golang

cpuguy83 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

tifayuki commented 6 years ago

@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 commented 6 years ago

LGTM