First commit prometheus middleware. #1022

enxebre · 2017-01-06T15:13:25Z

I started to look at this so I wanted to get familiar with the source code.
This is a trivial prometheus middleware based on https://github.com/zbindenren/negroni-prometheus

It could be an starting point for #839
I'm happy to continue working on this if there's interest on this feature.

As we've agreed on some points and this is taking shape I'll add a progress tracker here:

Discuss design
First commit prometheus middleware. #1022 (comment)
Define Metrics middleware and Interface
Create implementation for Prometheus
Agree on basic labels
Ability to choose exporter at run time (flag)
--web.metrics.prometheus
Support configurable params for exporters at runtime
--web.metrics.prometheus.Buckets="100,200"
Update Docs

emilevauge · 2017-01-06T15:29:40Z

ping @SantoDE :)

SantoDE · 2017-01-07T15:01:29Z

Hey @enxebre ,

thank's a lot for your contribution :-)

You're idea is in the right direction and a good starting point, but I'm looking for a slightly different approach to not "vendor lock in" on just prometheus. If you're interested in going on further here, let us have a short talk on Slack and / or drop me an email [email protected]. I guess, together we can move that further fast :)

/cc: @containous/traefik

Regards,
Santo

ralphtheninja · 2017-01-07T16:31:57Z

It would be awesome to have some kind of support for Prometheus. Looking forward to what you can come up with! :)

bamarni · 2017-01-08T12:41:48Z

There seems to be a nice generic package for that purpose : https://github.com/rcrowley/go-metrics

Looking forward to this too 👍

enxebre · 2017-01-09T08:42:07Z

cool, lets discuss on slack then. @SantoDE

enxebre · 2017-01-09T12:56:43Z

Potential solutions for a more generic approach could be:

https://github.com/go-kit/kit/tree/master/metrics which already includes support for prometheus
https://github.com/rcrowley/go-metrics + define a custom interface + specific implementations (e.g https://github.com/deathowl/go-metrics-prometheus or similiar to https://github.com/armon/go-metrics/blob/master/prometheus/prometheus.go)

It'd be nice to have:

Basic Grafana Traefik dashboard
Kubernetes Helm chart example for Traefik + Prometheus + Grafana dashboard

enxebre · 2017-01-10T11:14:59Z

Added a second commit with a more vendor neutral approach using github.com/go-kit/kit/metrics b9bc081

emilevauge · 2017-01-10T13:46:07Z

glide.yaml

@@ -101,7 +101,7 @@ import:
 - package: k8s.io/client-go
  version: ^v1.5.0
 - package: github.com/gogo/protobuf
-  version: 0.3
+  version: "0.3"


why any " here ?

I think it was just glide adding that

bamarni · 2017-01-10T15:00:12Z

middlewares/prometheus.go

+)
+
+var (
+	dflBuckets = []float64{300, 1200, 5000}


300ms seems high already, as those are cumulative how about adding some lower ones too?

bamarni · 2017-01-10T15:03:21Z

middlewares/metrics.go

+	start := time.Now()
+	prw := &responseRecorder{rw, http.StatusOK}
+	next(prw , r)
+	labels := []string{ "code", strconv.Itoa(prw.StatusCode()), "method", r.Method, "path", r.URL.Path }


I'm not sure the request path is suitable as a label for a generic metrics handler, because it has an unbounded cardinality.

We should probably use something else then the path. Sadly, I can't think of something yet. Will update once I have an idea :-)

bamarni · 2017-01-10T15:05:11Z

middlewares/prometheus.go

+		[]string{"code", "method", "path"},
+	)
+
+	if len(buckets) == 0 {


At the moment this check seems unneeded as the buckets are not configurable.

SantoDE · 2017-01-11T08:59:52Z

server.go

@@ -680,6 +682,8 @@ func (server *Server) loadConfig(configurations configs, globalConfiguration Glo
 						}

 						var negroni = negroni.New()
+						metricsMiddlewareBackend := middlewares.NewMetricsWrapper(middlewares.NewPrometheus(frontend.Backend))


We should make it configurable (for the feature), which specific "exporter" to use. WDYT @enxebre?

yup, I agree

SantoDE · 2017-01-11T09:01:35Z

middlewares/prometheus.go

+)
+
+var (
+	dflBuckets = []float64{300, 1200, 5000}


SantoDE · 2017-01-11T09:02:21Z

glide.yaml

+- package: github.com/mvdan/xurls
+- package: github.com/prometheus/client_golang


Do we need the client, if we're going with go-kit? Shouldn't go-kit bring it's dependencies?

You're right

SantoDE · 2017-01-11T09:03:02Z

middlewares/metrics.go

+	start := time.Now()
+	prw := &responseRecorder{rw, http.StatusOK}
+	next(prw , r)
+	labels := []string{ "code", strconv.Itoa(prw.StatusCode()), "method", r.Method, "path", r.URL.Path }


We should probably use something else then the path. Sadly, I can't think of something yet. Will update once I have an idea :-)

SantoDE · 2017-01-11T09:03:38Z

Hey @enxebre,

thank's for your contribution 👍

I'm okay with using go-kit as it's supporting labels (over e.g. go-metrics, which doesn't). I also like the approach of having a metrics middleware which then uses the specific "exporter". I added some feedback to the Review.

enxebre · 2017-01-11T14:12:34Z

Added a new commit with updated tests, docs and runtime config for monitoring.
Some explicit logic for the promhttp.handler on web.go and for instantiating the monitoring implementation on server.go is intentionally left there. This can be hidden in the metricsWrapper in the future once we have a least two implementations so we can see the right abstractions. 2fe8c82

SantoDE · 2017-01-12T07:07:35Z

Hey @enxebre,

that looks good 👍 When do you will have time to fix the conficts?

enxebre · 2017-01-12T12:09:15Z

Conflicts fixed and green build now

SantoDE · 2017-01-12T12:19:58Z

Hey @enxebre ,

Thanks again! Really :)

LGTM 👼

/cc: @containous/traefik

Could you please squash and rebase your branch? Thanks! :-)

bamarni · 2017-01-12T12:33:26Z

How about the path label, isn't the backend label enough? My concern with it is that it might create a lot of time series, if for instance backends contain dynamic placeholders such as order_id or user_id, which is very common.

IMO a label based on the request path is more suited for backends which have a well-known and limited set of paths, thus should be registered explicitly at application level as we cannot assume it at the proxy level.

SantoDE · 2017-01-12T12:35:52Z

@enxebre Yeah, I would probably keep out the path labels (as mentioned in the review). I don't see any real advantage just yet. A label per backend should be fine :)

enxebre · 2017-01-13T19:23:03Z

Never mind, just found the way :) should be ok now.
This is supported now --web.metrics.prometheus.Buckets="100,200"
Thoughts?

emilevauge

Thanks a lot @enxebre , that's awesome :)
LGTM (once tests pass, you need to go fmt)
/cc @containous/traefik

enxebre · 2017-01-16T14:37:03Z

Cool, green build now 🙌

SantoDE · 2017-01-16T14:39:51Z

Merged 👍 👼

Thanks a lot @enxebre

ralphtheninja · 2017-01-16T21:00:40Z

docs/toml.md

+
+```bash
+$ traefik --web.metrics.prometheus --web.metrics.prometheus.buckets="100,300"
+```


Is this all it takes? No steps needed to configure prometheus?

@ralphtheninja this is how Traefik exposes metrics to Prometheus. As a separate thing you must run Prometheus and configure it to scrape the Traefik endpoint . It be nice to have a Kubernetes Helm chart example for Traefik + Prometheus + Grafana dashboard showing the whole thing

Ok. Thanks for info!

brian-brazil · 2017-01-16T22:15:00Z

You're idea is in the right direction and a good starting point, but I'm looking for a slightly different approach to not "vendor lock in" on just prometheus.

The way we handle this in Prometheus is that the client libraries aren't tied to Prometheus, so you can for example instrument with Prometheus and output to Graphite without ever running a Prometheus server. Thus we recommend to use the standard Prometheus client libraries where possible.

brian-brazil · 2017-01-16T22:10:36Z

middlewares/prometheus.go

+
+const (
+	reqsName    = "requests_total"
+	latencyName = "request_duration_milliseconds"


The Prometheus convention is to use seconds.

brian-brazil · 2017-01-16T22:12:33Z

middlewares/prometheus.go

+// NewPrometheus returns a new prometheus Metrics implementation.
+func NewPrometheus(name string, config *types.Prometheus) *Prometheus {
+	var m Prometheus
+	m.reqsCounter = prometheus.NewCounterFrom(


This is already covered by the latency histogram's _count timeseries.

It's recommend not to split out latency results by success/failure/status code as that's very commonly misinterpreted. What I'd suggest is removing the code label from the latency metric.

It's also unwise on cardinality grounds to have too many labels on a histogram, as it already has one itself.

SantoDE · 2017-01-16T22:34:33Z

Hey @brian-brazil,

thank's a lot for your valuable feedback :-) Do you have any more? Shall we have a talk on Slack? Or are you even interested in putting up a PR on your?

This one is already merged so we need to put up another one anyways.

Cheers

brian-brazil · 2017-01-16T22:51:26Z

I saw it mentioned it on twitter, so I put in a few comments on common issues around instrumentation.

From another look, the only other comment I'd have would be to prefix your metrics with traefik_ or whatever else makes sense for you. In theory (and it's very much a theory) metric names should be globally unique, and you want a user to have an idea of what sort of request it is and where it's being measured from.

enxebre · 2017-01-16T23:18:24Z

Hey @brian-brazil thanks for the feedback!
I'll create a follow up PR to address the comments about instrumentation

enxebre force-pushed the prometheus branch from 03fdfa5 to b9bc081 Compare January 10, 2017 11:12

emilevauge reviewed Jan 10, 2017

View reviewed changes

bamarni reviewed Jan 10, 2017

View reviewed changes

SantoDE reviewed Jan 11, 2017

View reviewed changes

enxebre force-pushed the prometheus branch 3 times, most recently from aaa7f46 to 2fe8c82 Compare January 11, 2017 14:05

SantoDE added contributor/needs-resolve-conflicts kind/enhancement a new or improved feature. status/2-needs-review contributor/waiting-for-corrections labels Jan 12, 2017

enxebre force-pushed the prometheus branch 2 times, most recently from e49be88 to 34402bb Compare January 12, 2017 11:33

SantoDE approved these changes Jan 12, 2017

View reviewed changes

SantoDE removed the contributor/waiting-for-corrections label Jan 12, 2017

enxebre force-pushed the prometheus branch from f7c8a11 to 7c4a3e7 Compare January 12, 2017 13:36

enxebre force-pushed the prometheus branch 6 times, most recently from d8444da to 0d2d3b6 Compare January 16, 2017 09:38

emilevauge approved these changes Jan 16, 2017

View reviewed changes

enxebre force-pushed the prometheus branch from 0d2d3b6 to 26f3abc Compare January 16, 2017 11:12

Support for Metrics and Prometheus.

175659a

enxebre force-pushed the prometheus branch from 26f3abc to 175659a Compare January 16, 2017 14:00

SantoDE merged commit 483ef48 into traefik:master Jan 16, 2017

ralphtheninja reviewed Jan 16, 2017

View reviewed changes

brian-brazil reviewed Jan 16, 2017

View reviewed changes

SantoDE added status/3-needs-merge and removed contributor/needs-resolve-conflicts status/2-needs-review labels Jan 16, 2017

This was referenced Jan 17, 2017

Improving instrumentation. #1042

Merged

Traefik Stats System + Prometheus Exporter #839

Closed

StatsD (e.g. DataDog) support #834

Closed

More detailed metrics #156

Closed

ldez added area/middleware/metrics and removed status/3-needs-merge labels May 19, 2017

ldez added this to the 1.2 milestone Oct 1, 2017

		- package: github.com/mvdan/xurls
		- package: github.com/prometheus/client_golang

First commit prometheus middleware. #1022

First commit prometheus middleware. #1022

Conversation

enxebre commented Jan 6, 2017 • edited Loading

emilevauge commented Jan 6, 2017

SantoDE commented Jan 7, 2017

ralphtheninja commented Jan 7, 2017

bamarni commented Jan 8, 2017

enxebre commented Jan 9, 2017

enxebre commented Jan 9, 2017 • edited Loading

enxebre commented Jan 10, 2017 • 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

SantoDE commented Jan 11, 2017

enxebre commented Jan 11, 2017

SantoDE commented Jan 12, 2017

enxebre commented Jan 12, 2017 • edited Loading

SantoDE commented Jan 12, 2017 • edited Loading

bamarni commented Jan 12, 2017 • edited Loading

SantoDE commented Jan 12, 2017

enxebre commented Jan 13, 2017 • edited Loading

emilevauge left a comment • edited Loading

Choose a reason for hiding this comment

enxebre commented Jan 16, 2017

SantoDE commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-brazil commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SantoDE commented Jan 16, 2017

brian-brazil commented Jan 16, 2017

enxebre commented Jan 16, 2017

enxebre commented Jan 6, 2017 •

edited

Loading

enxebre commented Jan 9, 2017 •

edited

Loading

enxebre commented Jan 10, 2017 •

edited

Loading

enxebre commented Jan 12, 2017 •

edited

Loading

SantoDE commented Jan 12, 2017 •

edited

Loading

bamarni commented Jan 12, 2017 •

edited

Loading

enxebre commented Jan 13, 2017 •

edited

Loading

emilevauge left a comment •

edited

Loading