Add metrics for backend_retries_total #1504

mjeri · 2017-04-27T16:20:18Z

This PR adds metrics for the backend_retries_total. On the way to implement this, I did a couple of clean up tasks and improved the testing coverage of the existing implementation.

Cleanup tasks:

I renamed variables/methods for the request duration metrics, from latency to requestDuration. The concept was confused before in the code and in fact the request duration is tracked and the exported prometheus name was traefik_request_duration_seconds all the time. Therefore this is no breaking change, just clarification inside the code base.
I remove the Handler() method from the Metrics interface as this is a concern not relevant for consumers of the interface, which are concerned about the collection of metrics. Also the method was not even used.
I made the ResponseRecorder in the retry middleware private, as this is something you would not expect from the package interface to provide.

Apart of this I added unit tests for the retry.go and the parts I touched in the server.go. Especially the server.go method is hard to test, due to its current design, but I hope it is a first step and having the file will motivate others to increase the coverage there as well.

mjeri · 2017-04-27T16:34:46Z

I will fix the build tomorrow morning, please don't hesitate to have a look at the PR already. That happens when you only execute make test-unit and make test-integration locally 🐼

timoreimann

I work with Marco, we have reviewed the PR internally over the last few days. Hence, I think it makes sense if I give my LGTM. :-)

ldez · 2017-04-27T22:48:19Z

Welcome and thanks for your contribution!
Can you fix the "formatting bug" ? https://travis-ci.org/containous/traefik/jobs/226464395#L826

mjeri · 2017-04-28T06:37:58Z

Thanks for the welcome and for the introduction @timoreimann. I should probably also have said a couple of words :D I am happy to be able to contribute to the project. Linting errors are fixed by the way.

mjeri · 2017-05-02T08:41:44Z

Hi @vdemeester and @emilevauge. Can you have a look at this PR? @timoreimann told me that one of you has to give the approve the PR in order to get it merged :)

mjeri · 2017-05-12T08:03:56Z

I just updated PR against latest master again. @vdemeester or @emilevauge can you have a look?

emilevauge

Hey @marco-jantke, thanks a lot for your contribution!
I made few comments, I also what has been done in server.go could be improved a bit :)

emilevauge · 2017-05-12T16:45:59Z

middlewares/prometheus.go

+	if err != nil {
+		e, ok := err.(stdprometheus.AlreadyRegisteredError)
+		if !ok {
+			panic(err)


Please, don't use panic, return an error instead. You shouldn't stop a reverse proxy because of this.

Ok, I rebuilt it now in a way that if any of the Prometheus Metrics can not be registered, an error will be returned and the *Prometheus will be nil. This will lead to no Metrics instrumentation and an log message on Error level. Does it make sense to you this way?

emilevauge · 2017-05-12T16:52:39Z

middlewares/retry.go

 }

 // NewRetry returns a new Retry instance
-func NewRetry(attempts int, next http.Handler) *Retry {
+func NewRetry(attempts int, next http.Handler, metrics RetryMetrics) *Retry {


I think it would be better to add an interface here, instead of passing RetryMetrics directly. WDYT ?

type Listener interface { Retried(attempt int) error }

Usually I refrain from building abstractions before they are actually required. Therefore personally I would rather leave this part as is, extending it with a more generic implementation once we have an use-case for this.

@emilevauge WDYT? :)

The retry middleware should be loosely coupled to the metrics middleware. If I agree with you on not building abstractions too early, here we rather need isolation.

Ok, I see your point and I am fine with implementing the abstraction layer. Though I would like to make sure that we are on the same page with our understanding of the code.

The Retry middleware is not coupled to the metrics middleware. It only knows about the RetryMetrics interface, which happens to be right now in the middleware package (IMHO this is the rather confusing part). The Metrics and RetryMetrics interfaces are in my opinion already a layer of abstraction. The metrics middleware in turn also "only" depends on the Metrics interface.

Though the abstraction you suggest would remove all knowledge about metrics from the retry middleware. Do I understand your intention correctly? If so and you suggest that way, I will adapt the PR accordingly.

Though the abstraction you suggest would remove all knowledge about metrics from the retry middleware. Do I understand your intention correctly?

Exactly :)

Ok, thanks for the feedback. I implemented it now. See 7ff5692146cc1e0f8a804a1ef54e59cfeb4e4fb1

emilevauge · 2017-05-12T16:56:32Z

server/server.go

@@ -180,7 +180,8 @@ func (server *Server) startHTTPServers() {
 		serverMiddlewares := []negroni.Handler{server.accessLoggerMiddleware, server.loggerMiddleware, metrics}
 		if server.globalConfiguration.Web != nil && server.globalConfiguration.Web.Metrics != nil {
 			if server.globalConfiguration.Web.Metrics.Prometheus != nil {
-				metricsMiddleware := middlewares.NewMetricsWrapper(middlewares.NewPrometheus(newServerEntryPointName, server.globalConfiguration.Web.Metrics.Prometheus))
+				promMetrics, _ := middlewares.NewPrometheus(newServerEntryPointName, server.globalConfiguration.Web.Metrics.Prometheus)


Shouldn't you use newMetrics here too ?

Good catch, fixed it.

mjeri · 2017-05-16T06:47:35Z

Closing/Reopening to get SemaphoreCI working.

mjeri · 2017-05-19T09:53:09Z

@emilevauge can you have a look again? :)

ldez · 2017-05-25T00:21:34Z

@marco-jantke could you resolve the conflicts?

mjeri · 2017-05-30T13:43:46Z

Ok, everything should be ready now.

mjeri · 2017-06-06T10:24:08Z

@emilevauge ping

emilevauge · 2017-06-06T12:21:39Z

server/server.go

+	return nil
+}
+
+func registerRetryMiddleware(


@marco-jantke could you remove this new function? It is called only one time, has too few lines and is then hard to read.

I basically introduced to enable testing, that the middleware is constructed properly. I think this would not be possible anymore, when I inline the logic back into the loadConfig. So I can remove it of course if you prefer it this way, but I guess then the test also has to die.

Indeed, I was not thinking about this ;) It's all good then!

emilevauge

Thanks for your work @marco-jantke
LGTM

mjeri · 2017-06-07T07:22:00Z

Thanks for the reviews and merging 🎉

ldez added area/api kind/enhancement a new or improved feature. labels Apr 27, 2017

timoreimann approved these changes Apr 27, 2017

View reviewed changes

ldez added area/api status/2-needs-review and removed area/api labels Apr 27, 2017

mjeri force-pushed the add-retry-metrics branch from bca47c6 to dd93e3e Compare May 2, 2017 08:19

mjeri force-pushed the add-retry-metrics branch from dd93e3e to 4f2ae4a Compare May 4, 2017 06:40

mjeri force-pushed the add-retry-metrics branch from 4f2ae4a to 3df3265 Compare May 12, 2017 08:03

ldez added this to the 1.4 milestone May 12, 2017

emilevauge requested changes May 12, 2017

View reviewed changes

aantono mentioned this pull request May 15, 2017

StatsD (e.g. DataDog) support #834

Closed

mjeri force-pushed the add-retry-metrics branch 2 times, most recently from 716162c to 5d9e9ba Compare May 16, 2017 06:43

mjeri closed this May 16, 2017

mjeri reopened this May 16, 2017

ldez added the contributor/needs-resolve-conflicts label May 18, 2017

timoreimann mentioned this pull request May 19, 2017

[Traefik] Add traefik check DataDog/integrations-extras#64

Closed

ldez added area/middleware/metrics and removed area/api labels May 25, 2017

mjeri force-pushed the add-retry-metrics branch from 7ff5692 to 2383210 Compare May 29, 2017 07:09

aantono mentioned this pull request Jun 2, 2017

DataDog and StatsD Metrics Support #1701

Merged

ldez removed the contributor/needs-resolve-conflicts label Jun 3, 2017

mjeri force-pushed the add-retry-metrics branch from 2383210 to 9fdad4b Compare June 6, 2017 10:22

emilevauge reviewed Jun 6, 2017

View reviewed changes

emilevauge approved these changes Jun 6, 2017

View reviewed changes

add metrics for backend_retries_total

48b149f

mjeri force-pushed the add-retry-metrics branch from 9fdad4b to 48b149f Compare June 7, 2017 06:10

timoreimann merged commit e007bb7 into traefik:master Jun 7, 2017

mjeri deleted the add-retry-metrics branch June 7, 2017 07:22

ldez removed the status/2-needs-review label Jun 27, 2017

ldez changed the title ~~add metrics for backend_retries_total~~ Add metrics for backend_retries_total Aug 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metrics for backend_retries_total #1504

Add metrics for backend_retries_total #1504

mjeri commented Apr 27, 2017

mjeri commented Apr 27, 2017

timoreimann left a comment

ldez commented Apr 27, 2017

mjeri commented Apr 28, 2017

mjeri commented May 2, 2017

mjeri commented May 12, 2017

emilevauge left a comment

emilevauge May 12, 2017

mjeri May 13, 2017

emilevauge May 12, 2017

mjeri May 13, 2017 •

edited

mjeri May 18, 2017

emilevauge May 18, 2017

mjeri May 18, 2017

emilevauge May 18, 2017

mjeri May 19, 2017

emilevauge May 12, 2017

mjeri May 13, 2017

mjeri commented May 16, 2017

mjeri commented May 19, 2017

ldez commented May 25, 2017

mjeri commented May 30, 2017

mjeri commented Jun 6, 2017

emilevauge Jun 6, 2017

mjeri Jun 6, 2017

emilevauge Jun 6, 2017

emilevauge left a comment

mjeri commented Jun 7, 2017

Add metrics for backend_retries_total #1504

Add metrics for backend_retries_total #1504

Conversation

mjeri commented Apr 27, 2017

mjeri commented Apr 27, 2017

timoreimann left a comment

Choose a reason for hiding this comment

ldez commented Apr 27, 2017

mjeri commented Apr 28, 2017

mjeri commented May 2, 2017

mjeri commented May 12, 2017

emilevauge left a 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

mjeri May 13, 2017 • edited

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

mjeri commented May 16, 2017

mjeri commented May 19, 2017

ldez commented May 25, 2017

mjeri commented May 30, 2017

mjeri commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

mjeri commented Jun 7, 2017

mjeri May 13, 2017 •

edited