Metricbeat module for Traefik #7413

ycombinator · 2018-06-26T01:25:53Z

Resolves #7412.

This PR adds a Metricbeat module for the Traefik reverse proxy/load balancer. In this module, it adds the health metricset, whose data it retrieves from polling the Traefik instance's health HTTP API endpoint.

ycombinator · 2018-06-26T16:00:31Z

@ruflin Looking at the beats-ci console output, the failure is this:

08:21:59 ./module/traefik/health/health.go
08:21:59 Code differs from goimports' style ^

How can I fix this?

@exekias Is there some VS Code extension I should use to auto-fix this on save or something? I'm currently using this in my VS Code config:

  // Pick 'gofmt', 'goimports', 'goreturns' or 'goformat' to run on format.
  "go.formatTool": "goreturns",

[EDIT] I tried changing that setting to "goimports" and re-saved health.go but nothing seems to have changed. Maybe I'm barking up the wrong tree here?

exekias · 2018-06-26T16:05:29Z

You can use make fmt to fix this kind of issues.

As for VS Code, try adding this:

    "go.formatFlags": ["-local", "github.com/elastic/beats"],

ruflin

Don't forget to add a changelog.

It seems somehow the template loading test is failing. Not sure why, there must be something invalid in the template. If you run the test locally it should give you a good idea on what goes wrong in the logs.

ruflin · 2018-06-27T09:03:42Z

metricbeat/module/traefik/health/data.go

+var (
+	schema = s.Schema{
+		"uptime": s.Object{
+			"sec": c.Float("uptime_sec"),


Should this be Int?

This value is a float in the response from Traefik's health API (http://localhost:8080/health) so I kept it the same here.

In this case i would suggest we use milliseconds instead here, especially having elastic/elasticsearch#31244 in mind. TBH for uptime I would also be fine to just convert it to Int. I don't think there is a benefit of having uptime in sub second detail.

I don't think there is a benefit of having uptime in sub second detail.

Yeah, good point. Will change to Int.

ruflin · 2018-06-27T09:06:33Z

metricbeat/module/traefik/_meta/fields.yml

+- key: traefik
+  title: "traefik"
+  description: >
+    experimental[]


I think we now generate this automatically if the release setting is not set. So you can skip it here.

ycombinator · 2018-06-28T01:17:17Z

I'm investigating the integration test failures by running them locally as you suggested. From what I can tell so far I think the failure has to do with the Docker container for Traefik not starting up correctly.

Concretely, I ran this command:

$ go test -v -tags integration github.com/elastic/beats/metricbeat/module/traefik/health

And it's output got "stuck" like so:

=== RUN   TestFetch
time="2018-06-27T18:17:28-07:00" level=info msg="[0/33] [traefik]: Starting "
time="2018-06-27T18:17:28-07:00" level=warning msg="Error while reading .dockerignore (/Users/shaunak/go/src/github.com/elastic/beats/metricbeat/module/traefik/_meta/.dockerignore) : open /Users/shaunak/go/src/github.com/elastic/beats/metricbeat/module/traefik/_meta/.dockerignore: no such file or directory"
time="2018-06-27T18:17:28-07:00" level=info msg="Building metricbeat_traefik..."
time="2018-06-27T18:17:28-07:00" level=info msg="Recreating traefik"
time="2018-06-27T18:17:29-07:00" level=info msg="[1/33] [traefik]: Started "

Running docker-compose ps in another window, I see this:

$ docker-compose ps
        Name           Command            State           Ports
----------------------------------------------------------------
metricbeat_traefik_1   /traefik   Up (health: starting)   80/tcp

And eventually that becomes this:

        Name           Command        State        Ports
---------------------------------------------------------
metricbeat_traefik_1   /traefik   Up (unhealthy)   80/tcp

In either output of docker-compose ps above, notice that port 8080 is not shown, which is supposed to be the Traefik HTTP API port (which is also used by the HEALTHCHECK command in the Traefik Dockerfile, so it's not surprising that the container ultimately reports as unhealthy).

My guess is that this failing Docker health check is what's causing the integration test to be stuck. So I'm playing with the Dockerfile and docker-compose.yml to see if I can get port 8080 up somehow and try the tests again. Stay tuned...

ruflin · 2018-06-28T07:14:30Z

It seems that the container tries to expose port 80 instead of 8080. Is treafik by default running on 80 or 8080? Perhaps you have change the config here and add EXPOSE 8080 to the docker file.

ycombinator · 2018-06-28T08:54:01Z

Turned out to be a combination of issues in the Dockerfile, you can see the necessary changes to make the HEALTHCHECK work here: 2fcc0de27355bd68c2719850ed6e207992dcb094.

ycombinator · 2018-06-28T08:58:18Z

metricbeat/module/traefik/health/_meta/data.json

@@ -0,0 +1,19 @@
+{


What is the purpose of this file? How is it used?

This is an example output document that we should in the docs. It is generated by this command here: https://github.com/elastic/beats/blob/master/metricbeat/Makefile#L73 As this updates all data.json I normally run the following command:

go test -tags=integration github.com/elastic/beats/metricbeat/module/traefik/... -data

In you case this assumes the traefik docker container is running and exposes the port on localhost. This will generate a data.json file and make update will link it in the docs.

Here you can see an example in the docs: https://www.elastic.co/guide/en/beats/metricbeat/master/metricbeat-metricset-elasticsearch-index.html

ruflin · 2018-06-28T09:08:20Z

metricbeat/module/traefik/health/_meta/fields.yml

+  description: >
+    Metrics obtained from Traefik's health API endpoint
+  fields:
+    - name: uptime


One trick you can do here not to have to specify the group and describe is to directly write uptime.sec. The generators will do the rest of the magic for you.

ruflin · 2018-06-28T09:10:08Z

metricbeat/module/traefik/health/_meta/fields.yml

+            Average response time
+          fields:
+            - name: sec
+              type: float


As discussed with the other sec type I struggle that we have a sec value that is float. I understand that for the average it makes more sense but in this case I would rather use a smaller unit.

Makes sense, will use milliseconds and int here.

For proxies performance monitoring it is useful to have sub-millisecond response times if available.

ruflin · 2018-06-28T09:12:51Z

metricbeat/module/traefik/health/_meta/fields.yml

+          description: >
+            Respone status codes
+          fields:
+            - name: code


Not sure if this is correct. If understand the code correct it will be status_codes.404: 17? So code is actually the key and not the value? For such cases have a look at the dynamic templates. Search for object_type usage in our fields.yml to see how it can be used.

ruflin · 2018-06-28T09:13:20Z

metricbeat/module/traefik/health/data.go

+			"avg_time": s.Object{
+				"sec": c.Float("average_response_time_sec"),
+			},
+			"status_codes": s.Object{},


I would not define it here but leave it empty. Like this if we don't have any status codes, no object shows up in Elasticsearch.

ruflin · 2018-06-28T09:16:52Z

metricbeat/module/traefik/health/health_test.go

+)
+
+// raw response copied from Traefik instance's health API endpoint
+const response = `{


Instead of having it to define here, I would recommend to put this into a file which is loaded. This makes it then very easy to have a list of files to test against, for example different versions. See Elasticsearch module as an example.

ruflin · 2018-06-28T09:18:17Z

metricbeat/tests/system/test_traefik.py

+    COMPOSE_SERVICES = ['traefik']
+
+    @unittest.skipUnless(metricbeat.INTEGRATION_TESTS, "integration test")
+    def test_health(self):


there is a new method check_metricset in metricbeat.py. Using it could simplify your code here.

ruflin · 2018-06-28T09:18:56Z

metricbeat/tests/system/test_traefik.py

@@ -0,0 +1,36 @@
+import os


We recently started to put this file directly into the module directory (see Elasticsearch) to have all pieces in one place. But still both works.

jsoriano · 2018-06-28T10:28:49Z

CHANGELOG.asciidoc

@@ -119,6 +119,7 @@ https://github.com/elastic/beats/compare/v6.2.3...master[Check the HEAD diff]
 - Fix field mapping for the system process CPU ticks fields. {pull}7230[7230]
 - Fix Windows service metricset when using a 32-bit binary on a 64-bit OS. {pull}7294[7294]
 - Fix Jolokia attribute mapping when using wildcards and MBean names with multiple properties. {pull}7321[7321]
+- Added Traefik module with health metricset. {pull}7413[7413]


Move this entry to the Added section

Oops, good catch. Thanks!

jsoriano · 2018-06-28T10:34:40Z

metricbeat/module/traefik/health/_meta/fields.yml

+            Average response time
+          fields:
+            - name: sec
+              type: float


For proxies performance monitoring it is useful to have sub-millisecond response times if available.

jsoriano · 2018-06-28T10:57:42Z

metricbeat/module/traefik/health/health_integration_test.go

+
+	event := reporter.GetEvents()[0]
+	assert.NotNil(t, event)
+	t.Logf("%s/%s event: %+v", ms.Module().Name(), ms.Name(), event)


Could you also check something in the event fields?

ycombinator · 2018-06-28T22:52:52Z

@jsoriano @ruflin Thanks for your review. I believe I've addressed all the feedback now. Ready for your 👀 again.

jsoriano

There is a missing import in a test file, for the rest it LGTM :)

jsoriano · 2018-06-29T11:46:44Z

metricbeat/module/traefik/test_traefik.py

+import time
+from parameterized import parameterized
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '../../tests/system'))


import sys :)

Ahahaha, thank you.

Related: Is there some way to run system tests (i.e. the Python ones) for a specific module?

INTEGRATION_TESTS=1 nosetests module/traefik/ should do the trick. But you must have traefik running locally with docker exposing the port.

ruflin · 2018-06-29T13:21:54Z

metricbeat/module/traefik/health/data.go

+
+	event, _ := schema.Apply(health)
+
+	statusCodeCountMap := health["total_status_code_count"].(map[string]interface{})


you should check that the key exists and that the type assertion worked. you can add a second param ok which will contain a bool to do the second part.

ruflin · 2018-06-29T13:24:09Z

metricbeat/module/traefik/health/_meta/fields.yml

+          type: long
+          description: >
+            Average response time in microseconds
+        - name: status_codes


I would call it status_code because when someone access it in Kibana he will only look at 1 at the time:

health.status_code.404 > 17

ruflin · 2018-06-29T13:26:10Z

metricbeat/module/traefik/health/health.go

+
+	event, _ := eventMapping(data)
+	report.Event(mb.Event{
+		MetricSetFields: event,


To follow ECS, you could you add service.name: traefik to it?

ruflin · 2018-06-29T13:26:53Z

metricbeat/module/traefik/testing.go

@@ -0,0 +1,49 @@
+// Licensed to Elasticsearch B.V. under one or more contributor


@jsoriano Should these now go under mtest package in each module?

You are right, I'd prefer that, yes.

ycombinator · 2018-06-29T18:37:22Z

@ruflin Made the changes you suggested in the latest round of review. Ready for your eyes again, when you get a chance. Thanks!

ruflin

Left a few minor additional comments. Except for the float64 issue ok for me to be merged.

ruflin · 2018-07-02T09:07:09Z

metricbeat/module/traefik/health/health.go

+	"github.com/pkg/errors"
+
+	"github.com/elastic/beats/libbeat/common"
+


newline can be removed

ruflin · 2018-07-02T09:07:59Z

metricbeat/module/traefik/health/health.go

+	}.Build()
+)
+
+// MetricSet holds any configuration or state information. It must implement


I often remove the generator comments and make them more specific to the metricset or just very short. The comments were intended for someone that creates the metricset. Applies also to all other comments.

ruflin · 2018-07-02T09:08:33Z

metricbeat/module/traefik/health/data.go

+
+func eventMapping(health map[string]interface{}) (common.MapStr, *s.Errors) {
+	if averageResponseTimeSec, ok := health["average_response_time_sec"]; ok {
+		health["average_response_time_us"] = averageResponseTimeSec.(float64) * 1000 * 1000


What happens if for whatever reason averageResponseTimeSec cannot be typed to (float64)

ruflin · 2018-07-02T09:10:50Z

metricbeat/modules.d/traefik.yml.disabled

@@ -0,0 +1,7 @@
+# Module: traefik
+# Docs: https://www.elastic.co/guide/en/beats/metricbeat/master/metricbeat-module-traefik.html


I like the link to the docs. One issue this has is that we hardcode master here to every released config file will point to master. Would be nice if in the future this would be a variable that is replaced by the version during packaging.

ycombinator · 2018-07-02T17:31:45Z

@ruflin Addressed latest round of review feedback. Ready for your 👀 again. Thanks!

ruflin · 2018-07-03T07:34:13Z

@ycombinator Thanks for pushing this forward.

ycombinator · 2018-07-03T12:36:38Z

~~Backported to:~~

~~6.x / 6.4.0: 0ec0e7f~~

EDIT: This backport commit was later removed.

ycombinator added enhancement module Metricbeat Metricbeat labels Jun 26, 2018

ycombinator self-assigned this Jun 26, 2018

ycombinator force-pushed the metricbeat/module/traefik branch from cc52f5b to 8c99817 Compare June 26, 2018 02:04

ruflin added the in progress Pull request is currently in progress. label Jun 26, 2018

ycombinator force-pushed the metricbeat/module/traefik branch from 8c99817 to e55aaff Compare June 26, 2018 15:08

ruflin reviewed Jun 27, 2018

View reviewed changes

ycombinator force-pushed the metricbeat/module/traefik branch 2 times, most recently from d97f5e5 to a675b8b Compare June 27, 2018 22:47

ycombinator force-pushed the metricbeat/module/traefik branch from 6b32a9b to ce49d1a Compare June 28, 2018 08:56

ycombinator commented Jun 28, 2018

View reviewed changes

ycombinator added review and removed in progress Pull request is currently in progress. labels Jun 28, 2018

ycombinator requested review from jsoriano and ruflin June 28, 2018 09:00

ycombinator changed the title ~~[WIP] Metricbeat module for Traefik~~ Metricbeat module for Traefik Jun 28, 2018

ruflin reviewed Jun 28, 2018

View reviewed changes

jsoriano reviewed Jun 28, 2018

View reviewed changes

ycombinator force-pushed the metricbeat/module/traefik branch from ce49d1a to 0b917f4 Compare June 28, 2018 17:10

ycombinator added 4 commits June 28, 2018 17:19

Init checkin

4ac476a

Publishing uptime.sec

07afa97

Adding field to fields.yml; adding data.go

1da6229

Fixing imports formatting

6abccaf

ycombinator added 4 commits June 28, 2018 17:19

Simplifying using helper method

03d6a9f

Missed an import in the system test script

a0b5191

Forgot to define FIELDS

d9ec2fb

Copy pasta fail :(

bed5d3c

ycombinator force-pushed the metricbeat/module/traefik branch from bafb1b8 to bed5d3c Compare June 29, 2018 00:19

ycombinator added 2 commits June 29, 2018 03:49

Augmenting import path before importing metricbeat

69e433a

Fixing up formatting

2f26d10

jsoriano approved these changes Jun 29, 2018

View reviewed changes

Missed an import

3b0ef8b

ruflin reviewed Jun 29, 2018

View reviewed changes

ycombinator added 5 commits June 29, 2018 09:06

Adding guard clause

249f97f

Renaming status code field

072dda2

Adding service.name field

32a2914

Move testing config functions into mtest package

5fb7c35

Using MapStr.Put to crete service.name nested field

51d1f37

ruflin reviewed Jul 2, 2018

View reviewed changes

ycombinator added 3 commits July 2, 2018 08:53

Removing unnecessary newline

44d4e60

Shortening comments

5295345

Check that type assertion works

bb54c34

ruflin merged commit 4071bc7 into elastic:master Jul 3, 2018

ycombinator added needs_backport PR is waiting to be backported to other branches. v7.0.0-alpha1 v6.4.0 labels Jul 3, 2018

ycombinator removed the needs_backport PR is waiting to be backported to other branches. label Jul 3, 2018

ycombinator deleted the metricbeat/module/traefik branch July 5, 2018 20:32


		event, _ := schema.Apply(health)

		statusCodeCountMap := health["total_status_code_count"].(map[string]interface{})

		@@ -0,0 +1,49 @@
		// Licensed to Elasticsearch B.V. under one or more contributor

		"github.com/pkg/errors"

		"github.com/elastic/beats/libbeat/common"

		@@ -0,0 +1,7 @@
		# Module: traefik
		# Docs: https://www.elastic.co/guide/en/beats/metricbeat/master/metricbeat-module-traefik.html

Metricbeat module for Traefik #7413

Metricbeat module for Traefik #7413

Conversation

ycombinator commented Jun 26, 2018 • edited Loading

ycombinator commented Jun 26, 2018 • edited Loading

exekias commented Jun 26, 2018 • edited Loading

ruflin 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

ycombinator Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator commented Jun 28, 2018 • edited Loading

ruflin commented Jun 28, 2018

ycombinator commented Jun 28, 2018

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

ycombinator commented Jun 28, 2018

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin Jun 29, 2018 • 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

ycombinator commented Jun 29, 2018

ruflin 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

ycombinator commented Jul 2, 2018

ruflin commented Jul 3, 2018

ycombinator commented Jul 3, 2018 • edited Loading

ycombinator commented Jun 26, 2018 •

edited

Loading

ycombinator commented Jun 26, 2018 •

edited

Loading

exekias commented Jun 26, 2018 •

edited

Loading

ycombinator Jun 27, 2018 •

edited

Loading

ycombinator commented Jun 28, 2018 •

edited

Loading

ruflin Jun 29, 2018 •

edited

Loading

ycombinator commented Jul 3, 2018 •

edited

Loading