Add `add_docker_metadata` processor #4352

exekias · 2017-05-18T15:48:14Z

This processor will enrich events matching the desired configuration with Docker info:

docker.container.id
docker.container.image
docker.container.name
docker.container.labels

This PR adds docker/client dependency and cleans up common vendors with current go-dockerclient from metricbeat/module/docker.

I'm also using a patch for docker/client (exekias/moby@83d94aa) to ensure the client works in netbsd, will check with upstream if I can push that to master.

Important files to check here:
vendor.json:

Everything under add_docker_metadata (78f9943)

andrewkroh · 2017-05-18T16:09:45Z

I think we should consider using the top level vendor dir and removing the metricbeat/module/docker/vendor contents. We would have to check that nothing breaks in the docker module, but I'm guessing this would lead to smaller binaries. I don't know if cgo is used anywhere in these deps, but I have run into problems duplicate symbols when the same lib is included twice due to it being vendored under multiple paths.

andrewkroh · 2017-05-18T16:13:11Z

libbeat/processors/add_docker_metadata/add_docker_metadata.go

+
+	watcher, err := watcherConstructor(config.Host, config.TLS)
+	if err != nil {
+		panic(err)


Why not return an error?

wow, I can believe I left that there, sorry

andrewkroh · 2017-05-18T16:17:43Z

libbeat/processors/add_docker_metadata/add_docker_metadata.go

+}
+
+func (d *addDockerMetadata) String() string {
+	return "add_docker_metadata"


I think it is customary to include config info in the string representation (assuming no secrets are included). I don't believe there is a standard format.

andrewkroh · 2017-05-18T21:55:47Z

jenkins, test it

andrewkroh · 2017-05-18T22:38:24Z

jenkins, test it

ruflin

That is an addition we will really need in the future and become quite powerful. It especially is becoming interesting with docker logs and having the id in the pat (how are we going to extract it ;-) ) and then attach the info to it.

You used in this PR a new push API which I think we don't use yet. This makes a lot of sense in this context but would also be great to have in the docker module (or filebeat?)

I suggest two split this PR up in 3 parts:

Move docker dependencies to the top vendor directory. Be aware that we use our own fork of the docker project (see vendor.json on docker module)
Add processor
Decide where the add event as metricset or prospector

ruflin · 2017-05-19T06:54:27Z

filebeat/filebeat.full.yml

+#processors:
+#- add_docker_metadata:
+#    match_fields: ["system.process.cgroup.id"]
+#  #host: "unix:///var/run/docker.sock"


It seems like indentation would be off if uncommented, as this is commented twice

ruflin · 2017-05-19T06:57:03Z

libbeat/processors/add_docker_metadata/add_docker_metadata.go

+			meta.Put("container.labels", labels)
+		}
+
+		meta.Put("container.id", container.ID)


I assume this matches the "fields" with have in metricbeat for the docker containers?

yes, they do

ruflin · 2017-05-19T06:59:05Z

libbeat/processors/add_docker_metadata/add_docker_metadata.go

+		value, err := event.GetValue(field)
+		if err == nil {
+			if strValue, ok := value.(string); ok {
+				cid = strValue


fields is a list but in the end you only have one cid?

there is no way to match several fields into several containers, that should not happen. Fields is a list cause you may have different fields you want to match, not all of them present in all events

I kind of assume, the different events will come from different modules or prospectors. Means in this case each prospector should define it's own processor. So there would not be a need to have an array of fields I think.

ruflin · 2017-05-19T06:59:44Z

libbeat/processors/add_docker_metadata/add_docker_metadata.go

+	var cid string
+	for _, field := range d.fields {
+		value, err := event.GetValue(field)
+		if err == nil {


I recommend using here the default pattern:

if err != nil { continue } ...

Makes it easier to read in my opinion.

ruflin · 2017-05-19T07:02:35Z

libbeat/processors/add_docker_metadata/watcher.go

+	for _, c := range containers {
+		w.containers[c.ID] = &Container{
+			ID:     c.ID,
+			Name:   c.Names[0][1:], // Strip '/' from container names


Are we doing that also in the container metricset?

This is an spurious char given by this API call & version, doesn't seem to be the case for our other calls

ruflin · 2017-05-19T07:04:23Z

libbeat/processors/add_docker_metadata/watcher.go

+	WATCH:
+		for {
+			select {
+			case event := <-events:


So this is actually a docker event listener instead of polling the data. Very similar to what we have now in kubernetes? Should we also have this in the docker module as a metricset?

It could make sense yes

ruflin · 2017-05-19T07:06:17Z

filebeat/filebeat.full.yml

+#
+#processors:
+#- add_docker_metadata:
+#    match_fields: ["system.process.cgroup.id"]


I was first confused by this and had to read the code to understand what it does. It takes this field and checks if the value of this field matches to any container id. If yes, it will add the container meta data to the event.

In the code, it can currently only match 1 field I think, but here it is plural and you use an array. How is this going to work exactly?

BTW I'm not too sure about the example used here. In case someone uses cgroups, doesn't that kind of mean he doesn't want to use docker?

It checks all the fields until one of them matches

As for the example, you may want to have docker info on top of the cgroup id, so you can filter by, for instance, docker image

So the config option is kind of field_that_matches_container_id: .... What is the use case of having multiple fields defined?

For the cgroup docker part: I see that this is what it's doing, but why would someone then not just use the docker module instead as he will rely on the docker api.

We have several metrics matching cgroup id in system (maybe something we have to look into): system.process.cgroup.cpu.id, system.process.cgroup.memory.id, system.process.cgroup.blkio.id

But I assume per config you only want to match to one?

I would like to match all of them, perhaps in the future it would be better to merge those fields into a global system.cgroup.id, with different meanings depending on the metricset, but as for now I would need to match any possible field there.

what do you think @andrewkroh, does this make any sense?

exekias · 2017-05-19T09:47:18Z

Ok I'll play with vendors and open a PR, then rebase this once that is merged

exekias · 2017-05-29T17:33:32Z

Updated vendor folders, all details in PR description

tsg · 2017-05-31T10:10:23Z

Merging as the tests passed before the rebase, which only fixed the changelog.

exekias added libbeat review labels May 18, 2017

exekias force-pushed the add-docker-metadata branch 3 times, most recently from 6df9110 to c9cdf74 Compare May 18, 2017 16:04

andrewkroh reviewed May 18, 2017

View reviewed changes

ruflin reviewed May 19, 2017

View reviewed changes

exekias mentioned this pull request May 19, 2017

Move docker module vendor files to root folder #4362

Closed

monicasarbu added the in progress Pull request is currently in progress. label May 29, 2017

exekias force-pushed the add-docker-metadata branch 12 times, most recently from 46aaef3 to bfebcef Compare May 29, 2017 16:58

exekias removed the in progress Pull request is currently in progress. label May 29, 2017

exekias added 3 commits May 31, 2017 12:04

Add add_docker_metadata processor

f25e205

Add docker/client dependency

8eb3b9e

Remove duplicated docker vendored libraries

f17843b

exekias force-pushed the add-docker-metadata branch from bfebcef to f17843b Compare May 31, 2017 10:05

tsg merged commit 30ff394 into elastic:master May 31, 2017

monicasarbu added the :Processors label Jun 6, 2017

andrewkroh mentioned this pull request Jun 13, 2017

Modify add_docker_metadata processor to lookup ContainerId from source path in case of filebeat #4496

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `add_docker_metadata` processor #4352

Add `add_docker_metadata` processor #4352

exekias commented May 18, 2017 •

edited

Loading

andrewkroh commented May 18, 2017

andrewkroh May 18, 2017

exekias May 19, 2017

andrewkroh May 18, 2017

andrewkroh commented May 18, 2017

andrewkroh commented May 18, 2017

ruflin left a comment

ruflin May 19, 2017

ruflin May 19, 2017

exekias May 19, 2017

ruflin May 19, 2017

exekias May 19, 2017

ruflin May 19, 2017

ruflin May 19, 2017

ruflin May 19, 2017

exekias May 19, 2017

ruflin May 19, 2017

exekias May 29, 2017

ruflin May 19, 2017

ruflin May 19, 2017

exekias May 19, 2017

exekias May 19, 2017

ruflin May 19, 2017

exekias May 29, 2017

ruflin May 29, 2017

exekias May 29, 2017

exekias commented May 19, 2017

exekias commented May 29, 2017

tsg commented May 31, 2017

Add add_docker_metadata processor #4352

Add add_docker_metadata processor #4352

Conversation

exekias commented May 18, 2017 • edited Loading

andrewkroh commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh commented May 18, 2017

andrewkroh commented May 18, 2017

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

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

exekias commented May 19, 2017

exekias commented May 29, 2017

tsg commented May 31, 2017

Add `add_docker_metadata` processor #4352

Add `add_docker_metadata` processor #4352

exekias commented May 18, 2017 •

edited

Loading