Extract metrics to own package and refactor implementations #1968

mjeri · 2017-08-17T16:23:05Z

This PR should be the first step to an extension of the metrics implementations. In our Traefik fork that we are running in my project we have extended the Prometheus implementation considerably, but to get there some refactoring and alignment has to be done first. I created once already a PR for the extension of the Prometheus implementation, but it was diverging after Datadog and StatsD implementations were added at the same time to Traefik.

Therefore this PR is only about refactoring of the structure and extracting the Metrics implementations into their own package. Once the structure is aligned I will create follow-up metrics extending the functionality. I think it is a better approach this way, as it should make reviewing considerably easier.

Motivation

This PR is changing the design and I want to elaborate a bit in what it changes it and why:

extraction into a new metrics package: This is important in order to be able to track metrics outside of the middleware package. Meaningful metrics could be for example configuration reloads. Those happen in the server package and so an implementation inside of middleware can not be used (cyclic dependency). Another positive point about the extraction is the better separation of concerns in regards and a more clear project structure.
initialisation of metrics implementation: They all happen now exactly once in NewServer when Traefik boots up, which is the desired behaviour. Through the alignment of the initialisation the logic is now more straightforward and easier to understand.

mjeri · 2017-08-17T16:23:31Z

@aantono I think this is also interesting for you, I would be glad if you can have a look too :)

ldez

~~could you move the code to middleware/metrics/?~~

ldez · 2017-08-17T17:16:29Z

metrics/datadog_test.go

+package metrics
+
+import (
+	"net/http"


could you reorganize the imports?

Thanks for the notice, done.

aantono · 2017-08-17T17:33:11Z

@marco-jantke I think the name change for the label from serviceName to service should not be a big deal... but out of curiosity, why the change?

P.S. Just looked at the code and didn't actually find the place where the change took place. Inside the original datadog.go the NewDataDog(name string) method uses service as the label name, not the serviceName. Am I missing something?

mjeri · 2017-08-18T05:45:40Z

@aantono my fault I mixed it up. It was service all the time. Sorry for the confusion :/

ldez · 2017-08-19T16:35:16Z

metrics/metrics.go

+// NewVoidRegistry is a noop implementation of metrics.Registry.
+// It is used to avoid nil checking in components that do metric collections.
+func NewVoidRegistry() Registry {
+	return &voidRegistry{


You can remove voidRegistry struct and replace this struct by multiRegistry struct.

return &multiRegistry{ reqDurationHistogram: &voidHistogram{}, reqsCounter: &voidCounter{}, retriesCounter: &voidCounter{}, }

Thanks, I am using the standardRegistry now.

ldez · 2017-08-19T16:36:07Z

metrics/metrics.go

+	}
+}
+
+type voidRegistry struct {


You can remove voidRegistry struct because it's the same struct (and methods) as multiRegistry struct.

Thanks, I am using the standardRegistry now.

ldez · 2017-08-19T16:42:51Z

metrics/metrics_test.go

+		cRetriesCounter := collectingRegistry.RetriesCounter().(*collectingCounter)
+
+		if cReqsCounter.counterValue != 1 {
+			t.Errorf("Got value %v for ReqsCounter, want %v", cReqsCounter.counterValue, 1)


could you replace %v by %f?

Good idea. I also extracted the 1 to a value with proper float64 type, so that I can use %f both times.

ldez · 2017-08-19T16:42:55Z

metrics/metrics_test.go

+			t.Errorf("Got value %v for ReqsCounter, want %v", cReqsCounter.counterValue, 1)
+		}
+		if cReqDurationHistogram.lastHistogramValue != 2 {
+			t.Errorf("Got last observation %v for ReqDurationHistogram, want %v", cReqDurationHistogram.lastHistogramValue, 2)


could you replace %v by %f?

Thanks, done.

ldez · 2017-08-19T16:43:06Z

metrics/metrics_test.go

+			t.Errorf("Got last observation %v for ReqDurationHistogram, want %v", cReqDurationHistogram.lastHistogramValue, 2)
+		}
+		if cRetriesCounter.counterValue != 3 {
+			t.Errorf("Got value %v for RetriesCounter, want %v", cRetriesCounter.counterValue, 3)


could you replace %v by %f?

Thanks, done.

ldez · 2017-08-19T17:06:17Z

metrics/metrics.go

+	return r.reqsCounter
+}
+
+// ReqsCounter


remove or add the good comment

Added it by accident. Removed the comment. I think they are not necessary on "Getters" and the method's struct is not exported.

ldez · 2017-08-19T17:07:59Z

metrics/prometheus_test.go

+		for _, label := range family.Metric[0].Label {
+			val, ok := test.labels[*label.Name]
+			if !ok {
+				t.Errorf("'%s' metric contains unexpected label '%s'", test.name, label)


could you replace '%s' by %q?

Good point. Adapted the error message.

ldez · 2017-08-19T17:08:04Z

metrics/prometheus_test.go

+			if !ok {
+				t.Errorf("'%s' metric contains unexpected label '%s'", test.name, label)
+			} else if val != *label.Value {
+				t.Errorf("label '%s' in metric '%s' has wrong value '%s'", label, test.name, *label.Value)


could you replace '%s' by %q?

Good point. Adapted and improved the error message.

ldez · 2017-08-19T17:08:21Z

metrics/prometheus_test.go

+	for _, test := range tests {
+		family := findMetricFamily(test.name, metricsFamilies)
+		if family == nil {
+			t.Errorf("gathered metrics do not contain '%s'", test.name)


could you replace '%s' by %q?

ldez · 2017-08-19T17:08:57Z

metrics/prometheus_test.go

+				cv := uint(family.Metric[0].Counter.GetValue())
+				expectedCv := uint(1)
+				if cv != expectedCv {
+					t.Errorf("gathered metrics do not contain correct value for total retries, got '%d' expected '%d'", cv, expectedCv)


could you replace '%d' by %q?

Doesn't sound right to me. When I change this it gets from:
gathered metrics do not contain correct value for total requests, got 2 expected 23
to
gathered metrics do not contain correct value for total requests, got '\x02' expected '\x17'.

Maybe though I should rather use floats for comparison and print messages with %f. WDYT?

mjeri · 2017-08-21T07:02:05Z

Thanks for the review @ldez. I rebased the branch to latest master version and addressed all your comments.

ldez · 2017-08-21T08:46:35Z

@marco-jantke

These files are not properly gofmt'd:

server/server.go

ldez

LGTM

nmengin

LGTM 👏

emilevauge

@marco-jantke Great job, it really simplifies the metrics :)
I have one comment though.

emilevauge · 2017-08-22T20:21:51Z

server/server.go

@@ -53,6 +54,8 @@ type Server struct {
 	routinesPool                  *safe.Pool
 	leadership                    *cluster.Leadership
 	defaultForwardingRoundTripper http.RoundTripper
+	metricsRegistry               metrics.Registry
+	metricsEnabled                bool


I'm not fan of adding this flag. Can't we remove it and use metricsRegistry instead?

metricsRegistry now always contains an implementation. When nothing is configured it will hold a VoidRegistry. This means we could write a util function like metricsEnabled() and it will return the ok of a type assertion on the metrics implementation. You would prefer this way?

Personally I am not sure about what is the more explicit option, but both is not hard to implement so you are free to choose :D

I double checked the implementation and my described approach above won't work. All different registries that we create through e.g. NewVoidRegistry, RegisterStatsd, RegisterPrometheus.. will have the internal type standardRegistry and therefore I can't use a type assertion.

This means a metricsEnabled() utility would have to check on the global configuration once more. E.g. if globalConfiguration.Web.Metrics.Prometheus != nil || globalConfiguration.Web.Metrics.Statsd != nil .... Personally I think that having this conditions only in registerMetricClients is the more clean approach and tend to leave it like it is. WDYT?

As discussed on Slack refactored now towards an IsEnabled() method on the Registry.

ldez · 2017-08-23T12:23:07Z

metrics/metrics.go

+}
+
+type standardRegistry struct {
+	isEnabled            bool


could you rename isEnabled to enabled?
But keep the method name IsEnabled()

sorry enabled not enable

Sure thing, done.

ldez

LGTM

emilevauge

Awesome @marco-jantke !
LGTM

in order to avoid a name clash for the soon to come metrics package.

This commit is just refactoring and should provide the same functionality as before. It extracts the metrics implementation into its own package and makes metrics therefore extendable to be used outside of the middleware package in the future. Another design goal is to simplify and align the initialisation of the Metrics implementations. Each Metric provider should only be initialised once and reused when configuration reloads happen. This commit accomplishes this.

in order to have less state on the server struct.

ldez requested changes Aug 17, 2017

View reviewed changes

ldez added area/middleware/metrics kind/enhancement a new or improved feature. status/2-needs-review labels Aug 17, 2017

ldez added this to the 1.5 milestone Aug 17, 2017

ldez added the size/L label Aug 17, 2017

ldez modified the milestones: 1.4, 1.5 Aug 19, 2017

ldez requested changes Aug 19, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Aug 19, 2017

ldez added the contributor/needs-resolve-conflicts label Aug 19, 2017

mjeri force-pushed the metrics-refactoring branch 2 times, most recently from ca62bdb to 2db1e86 Compare August 21, 2017 07:01

ldez removed the contributor/needs-resolve-conflicts label Aug 21, 2017

ldez approved these changes Aug 21, 2017

View reviewed changes

traefiker removed the contributor/waiting-for-corrections label Aug 21, 2017

nmengin approved these changes Aug 21, 2017

View reviewed changes

emilevauge reviewed Aug 22, 2017

View reviewed changes

mjeri force-pushed the metrics-refactoring branch from 886ea96 to 60d4dfb Compare August 23, 2017 10:39

ldez requested changes Aug 23, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Aug 23, 2017

ldez approved these changes Aug 23, 2017

View reviewed changes

traefiker removed the contributor/waiting-for-corrections label Aug 23, 2017

emilevauge approved these changes Aug 23, 2017

View reviewed changes

emilevauge added the status/3-needs-merge label Aug 23, 2017

emilevauge removed the status/2-needs-review label Aug 23, 2017

traefiker added the status/4-merge-in-progress label Aug 23, 2017

mjeri added 9 commits August 23, 2017 18:22

rename global var metrics to stats

dd166e5

in order to avoid a name clash for the soon to come metrics package.

fix import order

ea4fef4

implement review feedback

047d853

fix formatting

310ac8d

implement review feedback

baba69a

remove obosolete and wrong comment

9a385f4

extract metricsEnabled to own Registry method

9ee02d6

in order to have less state on the server struct.

rename field isEnabled to enabled

6a4221b

traefiker force-pushed the metrics-refactoring branch from ed480cb to 6a4221b Compare August 23, 2017 18:22

traefiker removed the status/4-merge-in-progress label Aug 23, 2017

traefiker merged commit e6c2040 into traefik:master Aug 23, 2017

traefiker removed the status/3-needs-merge label Aug 23, 2017

mjeri deleted the metrics-refactoring branch August 23, 2017 18:52

emilevauge mentioned this pull request Aug 24, 2017

Refactor globalConfiguration / WebProvider #1938

Merged

ldez mentioned this pull request Sep 1, 2017

Prometheus request metrics tracked twice #1759

Closed

Extract metrics to own package and refactor implementations #1968

Extract metrics to own package and refactor implementations #1968

Conversation

mjeri commented Aug 17, 2017 • edited

Motivation

mjeri commented Aug 17, 2017

ldez left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aantono commented Aug 17, 2017 • edited

mjeri commented Aug 18, 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

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

mjeri commented Aug 21, 2017

ldez commented Aug 21, 2017

ldez left a comment

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

ldez Aug 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

mjeri commented Aug 17, 2017 •

edited

ldez left a comment •

edited

aantono commented Aug 17, 2017 •

edited

ldez Aug 23, 2017 •

edited