Add experimental Docker json-file prospector #5402

exekias · 2017-10-18T07:13:29Z

This pull request adds an experimental docker prospector. Docker supports different ways of outputting logs, this prospector implements json-file.

By offering the prospector we abstract the format from the user, so there is no need to manually configure JSON decoding, also we can get timestamp from the JSON file, something that's not possible as of now from existing settings.

prospectors:
   - type: docker
     containers.ids:
       - c3ec7a0bd9640151a768663b7e78c115d5b1a7f87fba572666bacd8065893d41

This would read all logs:

prospectors:
   - type: docker
     containers.ids:
       - '*'

This also abstract specifics of the chosen provider, for this case this would create a file prospector for /var/lib/docker/containers/c3ec7a0bd9640151a768663b7e78c115d5b1a7f87fba572666bacd8065893d41/*.log.

In the future, we can extend this to support other logging drivers/formats, like CRI format, or a plugin implementation. I would add a conf key like mode: json-file once that happens.

TODO:

Abstract file paths by adding a container_id parameter.
Extract @timestamp from JSON
Add tests

ruflin

I'm going forth and back on this one if we should have config options to support json in json or a prospector type. The reason I'm leaning in the direction of the prospector type as you did here is that it allows us the change the implementation in the future and add more magic without affecting the user config. +1 on this :-)

@exekias Could you add some more details to the PR description on what this prospector does. An example on what the input log data looks like and what it makes out of it would be great.

ruflin · 2017-10-20T14:48:25Z

filebeat/harvester/util.go

-	LogType:   {},
-	RedisType: {},
-	UdpType:   {},
+	StdinType:  {},


@kvch Not related to this PR, but as we register the prospectors now I'm curious if we still need this?

ValidType is still used in log prospector for config validation and opening the right input.
But there is no other check/use of it. The check now happens when a factory is retrieved from registry using GetFactory. So it is not required, but this part needs further refactoring to make ValidType log specific. For the sake of consistency and to make refactoring easier, it is acceptable to add docker to the list.

I pushed #5537 to remove this part.

ruflin · 2017-11-07T22:41:25Z

Lets move this forward to play around with it but mark it experimental for now. Can you add an experimental log message and add it to the CHANGELOG?

kwojcicki · 2017-11-08T21:34:57Z

I would like to have filebeat only harvest from currently active docker containers. Was wondering if this would be an appropriate thing to add to this docker prospector or maybe it should be added somewhere else?

houndci-bot · 2017-11-14T12:09:20Z

filebeat/harvester/util.go

+	LogType    = "log"
+	StdinType  = "stdin"
+	RedisType  = "redis"
+	UdpType    = "udp"


const UdpType should be UDPType

houndci-bot · 2017-11-14T12:09:20Z

filebeat/harvester/reader/message.go

@@ -42,3 +42,17 @@ func (msg *Message) AddFields(fields common.MapStr) {
 	}
 	msg.Fields.Update(fields)
 }
+
+func (msg *Message) RemoveFields(fields ...string) {


exported method Message.RemoveFields should have comment or be unexported
receiver name msg should be consistent with previous receiver name m for Message

houndci-bot · 2017-11-14T12:16:44Z

filebeat/harvester/reader/message.go

@@ -42,3 +43,18 @@ func (msg *Message) AddFields(fields common.MapStr) {
 	}
 	msg.Fields.Update(fields)
 }
+
+// RemoveFields removes given fields from the message
+func (msg *Message) RemoveFields(fields ...string) {


receiver name msg should be consistent with previous receiver name m for Message

exekias · 2017-11-14T12:17:44Z

This should be ready for a second look @ruflin, I implemented timestamp parsing, put message in the right place and added some tests

exekias · 2017-11-14T14:39:06Z

jenkins, retest this please

ruflin

LGTM. Left some minor comments.

ruflin · 2017-11-15T02:53:54Z

filebeat/harvester/reader/docker_json.go

+	}
+
+	message.AddFields(common.MapStr{
+		"stream": line.Stream,


Could you give some details on what values stream will have? Should we add this to a fields.yml?

I see below this is stdin for example. If we know this field will exist, probably worth adding as keyword to fields.yml

Just pushed it, thanks!

ruflin · 2017-11-15T02:54:32Z

filebeat/harvester/reader/docker_json.go

+	// Parse timestamp
+	ts, err := time.Parse(time.RFC3339, line.Timestamp)
+	if err != nil {
+		return message, errors.Wrap(err, "parsing docker timestamp")


Perhaps worth adding the given timestamp to the error. Probably already part of err?

parse error looks pretty verbose: https://golang.org/src/time/format.go?s=23626:23672#L669

ruflin · 2017-11-15T02:56:43Z

filebeat/harvester/util.go

-	LogType:   {},
-	RedisType: {},
-	UdpType:   {},
+	StdinType:  {},


I pushed #5537 to remove this part.

ruflin · 2017-11-15T02:58:20Z

filebeat/prospector/docker/config.go

+}
+
+type config struct {
+	ContainersIDs  []string `config:"containers_ids"`


Could we make this container.ids and container.path or containers.ids and containers.path?

ruflin · 2017-11-15T03:00:56Z

filebeat/prospector/log/config.go

@@ -83,6 +83,9 @@ type config struct {
 	MaxBytes     int                     `config:"max_bytes" validate:"min=0,nonzero"`
 	Multiline    *reader.MultilineConfig `config:"multiline"`
 	JSON         *reader.JSONConfig      `config:"json"`
+
+	// Hidden on purpose, used by the docker prospector:
+	DockerJSON bool `config:"docker-json"`


Kind of hacky but don't have a better solution.

@urso recommended something like this to avoid repeating code or a major refactor, works for me 😇

ruflin · 2017-11-15T03:03:27Z

filebeat/prospector/log/harvester.go

@@ -515,6 +517,11 @@ func (h *Harvester) newLogFileReader() (reader.Reader, error) {
 		return nil, err
 	}

+	if h.config.DockerJSON {


It's interesting that form an implementation point of view it just comes down to adding a reader in the harvester (simplified). Seems like the log harvester is pretty generic and perhaps should be extracted. Prospectors then could configure "readers" on the harvester. (just thinking out loud for later and @urso was also talking about similar stuff in the past).

exekias · 2017-11-15T15:18:05Z

rebased after master changes

ruflin · 2017-11-16T02:53:00Z

filebeat/_meta/fields.common.yml

@@ -23,6 +23,12 @@
      description: >
        The content of the line read from the log file.

+    - name: stream


Could we call this log.stream or something similar? log.source? Source is already overloaded.

@weltenwort has some ideas for a better name?

ruflin · 2017-11-16T02:53:26Z

Merged this one as the field naming discussion we can still do in a follow up PR.

tsg · 2017-11-21T10:28:48Z

@exekias Please remember to add docs for this one before the 6.1 GA date.

This adds docs for elastic#5402

This adds docs for #5402

This adds docs for elastic#5402 (cherry picked from commit c37cc31)

This adds docs for #5402 (cherry picked from commit c37cc31)

dedemorton · 2018-01-22T19:11:03Z

Removing needs_docs label because it looks like docs were added in #5752

This adds docs for elastic#5402 (cherry picked from commit b9ce598)

exekias added discuss Issue needs further discussion. enhancement Filebeat Filebeat in progress Pull request is currently in progress. labels Oct 18, 2017

exekias force-pushed the docker-prospector branch from 57d4f33 to c7379f4 Compare October 18, 2017 08:04

ruflin reviewed Oct 20, 2017

View reviewed changes

exekias changed the title ~~[WIP] Add experimental Docker json-file prospector~~ Add experimental Docker json-file prospector Oct 30, 2017

exekias force-pushed the docker-prospector branch from c7379f4 to 643cdd6 Compare November 14, 2017 12:09

houndci-bot reviewed Nov 14, 2017

View reviewed changes

exekias force-pushed the docker-prospector branch 2 times, most recently from 0d6a2ca to cdf12cb Compare November 14, 2017 12:16

houndci-bot reviewed Nov 14, 2017

View reviewed changes

exekias added review and removed discuss Issue needs further discussion. in progress Pull request is currently in progress. labels Nov 14, 2017

exekias force-pushed the docker-prospector branch from cdf12cb to 3a05f29 Compare November 14, 2017 12:18

ruflin reviewed Nov 15, 2017

View reviewed changes

exekias force-pushed the docker-prospector branch from 3a05f29 to 13a4c02 Compare November 15, 2017 09:49

Add experimental Docker json-file prospector

98641f7

exekias force-pushed the docker-prospector branch from 13a4c02 to 98641f7 Compare November 15, 2017 14:28

exekias added the needs_docs label Nov 15, 2017

ruflin reviewed Nov 16, 2017

View reviewed changes

ruflin merged commit 4b3f9e3 into elastic:master Nov 16, 2017

exekias mentioned this pull request Nov 17, 2017

Add support for CRI logs format #5630

Closed

exekias mentioned this pull request Nov 29, 2017

Add docs for docker prospector #5752

Merged

exekias added a commit to exekias/beats that referenced this pull request Nov 29, 2017

Add docs for docker prospector

2ce363c

This adds docs for elastic#5402

ruflin pushed a commit that referenced this pull request Nov 29, 2017

Add docs for docker prospector (#5752)

c37cc31

This adds docs for #5402

exekias added a commit to exekias/beats that referenced this pull request Nov 30, 2017

Add docs for docker prospector (elastic#5752)

090bace

This adds docs for elastic#5402 (cherry picked from commit c37cc31)

exekias mentioned this pull request Nov 30, 2017

Cherry-pick #5752 to 6.1: Add docs for docker prospector #5774

Merged

kvch pushed a commit that referenced this pull request Nov 30, 2017

Add docs for docker prospector (#5752) (#5774)

4e751dd

This adds docs for #5402 (cherry picked from commit c37cc31)

exekias mentioned this pull request Dec 12, 2017

Getting Logs from Docker Containers #918

Closed

exekias mentioned this pull request Dec 20, 2017

Update Filebeat Kubernetes manifests to use docker prospector #5934

Closed

dedemorton removed the needs_docs label Jan 22, 2018

leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023

Add docs for docker prospector (elastic#5752) (elastic#5774)

81e98d0

This adds docs for elastic#5402 (cherry picked from commit b9ce598)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add experimental Docker json-file prospector #5402

Add experimental Docker json-file prospector #5402

exekias commented Oct 18, 2017 •

edited

Loading

ruflin left a comment

ruflin Oct 20, 2017

kvch Oct 24, 2017

ruflin Nov 15, 2017

ruflin commented Nov 7, 2017

kwojcicki commented Nov 8, 2017

houndci-bot Nov 14, 2017

houndci-bot Nov 14, 2017

houndci-bot Nov 14, 2017

exekias commented Nov 14, 2017

exekias commented Nov 14, 2017

ruflin left a comment

ruflin Nov 15, 2017

ruflin Nov 15, 2017

exekias Nov 15, 2017

ruflin Nov 15, 2017

exekias Nov 15, 2017

ruflin Nov 15, 2017

ruflin Nov 15, 2017

ruflin Nov 15, 2017

exekias Nov 15, 2017 •

edited

Loading

ruflin Nov 15, 2017

exekias commented Nov 15, 2017

ruflin Nov 16, 2017

ruflin commented Nov 16, 2017

tsg commented Nov 21, 2017

dedemorton commented Jan 22, 2018

Add experimental Docker json-file prospector #5402

Add experimental Docker json-file prospector #5402

Conversation

exekias commented Oct 18, 2017 • 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

ruflin commented Nov 7, 2017

kwojcicki commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias commented Nov 14, 2017

exekias commented Nov 14, 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

exekias Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias commented Nov 15, 2017

Choose a reason for hiding this comment

ruflin commented Nov 16, 2017

tsg commented Nov 21, 2017

dedemorton commented Jan 22, 2018

exekias commented Oct 18, 2017 •

edited

Loading

exekias Nov 15, 2017 •

edited

Loading