Keep different registry entry per container stream #7281

exekias · 2018-06-06T23:44:30Z

This PR adds a metadata map to prospectors state. Adding metadata to states allows having a different state per unique metadata for the same file.

This change is used by the docker prospector to have different states per stream, as some users configure a container prospector per stream (stdout & stderr).

Probably fixes #7045

exekias · 2018-06-06T23:47:51Z

Hound wanted me to rename Id field to ID, but that would spawn a broader change, I don't think it's worth it, as this field wasn't added on this PR.

houndci-bot · 2018-06-07T09:38:05Z

filebeat/input/file/state.go

-	Timestamp   time.Time     `json:"timestamp"`
-	TTL         time.Duration `json:"ttl"`
-	Type        string        `json:"type"`
+	Id          string            `json:"-"` // local unique id to make comparison more efficient


struct field Id should be ID

urso · 2018-06-07T13:29:31Z

filebeat/input/file/state.go

 	}
 }

 // ID returns a unique id for the state as a string
 func (s *State) ID() string {
 	// Generate id on first request. This is needed as id is not set when converting back from json
 	if s.Id == "" {
-		s.Id = s.FileStateOS.String()
+		h, _ := hashstructure.Hash(s.Meta, nil)
+		s.Id = s.FileStateOS.String() + fmt.Sprintf("-%d", h)


ouch... we optimized ID generation by removing Sprintf and reduce allocations. The ID generation used to be one of the worst offenders in CPU profiles. This change makes it worse again. How about:

if s.Meta == nil { s.Id = s.FileStateOS.String() } else { hashValue, _ := hashstructure.Hash(s.Meta, nil) var hashBuf [17]byte hash, _ := strconv.AppendUint(h[:0], hashValue, 16) hash = append(hash, '-') fileID := s.FileStateOS.String() var b strings.Builder b.Grow(len(hash) + len(fileID)) b.Write(hash) b.WriteString(fileID) s.Id = b.String() }

This still does execute an extra alloc, but only in the docker case and should be less expensive.

sounds like a good idea, pushing this change

urso · 2018-06-07T13:33:35Z

filebeat/input/file/state.go

@@ -49,5 +56,5 @@ func (s *State) IsEqual(c *State) bool {

 // IsEmpty returns true if the state is empty
 func (s *State) IsEmpty() bool {
-	return *s == State{}
+	return reflect.DeepEqual(*s, State{})


we need DeepEqual?

How about tmp := *s; tmp.Meta = nil; return tmp == State{} && len(s.Meta) == 0

The compiler still says it cannot compare maps, are your concerns about performance? I could do manual comparison for all fields, or use 2 structs like:

type State struct { Common Meta map[string]string `json:"meta"` }

then:

if len(s.Meta) > 0 { return false } return *s.Common == Common{}

a little performance, but I'd also prefer to keep out reflect from most packages.

I wonder if checking for FileStateOS or the timestamp only would be enough. This method is used to determine if we have a valid/usable state or not.

I don't have enough experience with all the possible situations with the registry so I'll trust you here on what to do 😇

I updated this to use s.FileStateOS == file.StateOS{} && s.Timestamp.IsZero() ..., also pinging @ruflin for awareness

exekias · 2018-06-08T09:11:33Z

This should be ready for another review, thanks for all the help @urso!

exekias · 2018-06-08T10:11:11Z

jenkins, retest this please

This allows us to tag a source file with different metadata, to keep independent read streams.

Configs using multiple streams won't share state, which was causing issues.

exekias · 2018-06-08T15:12:39Z

jenkins, retest this please

urso · 2018-06-08T15:28:58Z

filebeat/tests/system/test_registrar.py

+
+        self.render_config_template(
+            type='docker',
+            input_raw='''


btw. check out the textwrap package. This allows you to indent multiline strings within the code more properly.

urso · 2018-06-08T15:31:04Z

LGTM and filebeat CI is green. Waiting for review from @ruflin

ruflin

Left a minor question.

The change LGTM. The part I didn't fully understand yet is why in same log file data from stderr and stdout end up. Is that if 2 different services are sending data out of a single container?

ruflin · 2018-06-11T07:42:25Z

filebeat/input/file/state.go

@@ -49,5 +72,8 @@ func (s *State) IsEqual(c *State) bool {

 // IsEmpty returns true if the state is empty
 func (s *State) IsEmpty() bool {
-	return *s == State{}
+	return s.FileStateOS == file.StateOS{} &&


Can you share some details here on why not only add s.Meta but also the other chnages?

As I added a map to the State struct, a simple comparison is no longer possible (Go doesn't implement map compare), I had to update the code to do an individual comparison on the struct fields

exekias · 2018-06-12T09:59:20Z

Docker json-file format stores both stderr and stdout in the same file. They use a JSON object per line, with the following schema:

{"log":"example line\n","stream":"stdout","time":"2018-04-13T08:38:08.395077018Z"}
{"log":"example line\n","stream":"stderr","time":"2018-04-13T08:38:08.395077018Z"}

Our docker input can parse that and demultiplex the 2 different streams, as some times you want to do different processing on them (for instance, pass them to 2 different filesets).

Some containers use this to send different log per stream, for instance nginx: https://github.com/nginxinc/docker-nginx/blob/f603bb3632ea6df0bc9da2179c18eb322c286298/mainline/stretch/Dockerfile#L92-L93

This PR adds a metadata map to prospectors state. Adding metadata to states allows having a different state per unique metadata for the same file. This change is used by the `docker` prospector to have different states per stream, as some users configure a container prospector per stream (`stdout` & `stderr`). Probably fixes elastic#7045 (cherry picked from commit d37c114)

… stream (#7300) Cherry-pick of PR #7281 to 6.3 branch. Original message: This PR adds a metadata map to prospectors state. Adding metadata to states allows having a different state per unique metadata for the same file. This change is used by the `docker` prospector to have different states per stream, as some users configure a container prospector per stream (`stdout` & `stderr`). Probably fixes #7045

…ntainer stream (elastic#7300) Cherry-pick of PR elastic#7281 to 6.3 branch. Original message: This PR adds a metadata map to prospectors state. Adding metadata to states allows having a different state per unique metadata for the same file. This change is used by the `docker` prospector to have different states per stream, as some users configure a container prospector per stream (`stdout` & `stderr`). Probably fixes elastic#7045

exekias added enhancement in progress Pull request is currently in progress. Filebeat Filebeat labels Jun 6, 2018

exekias mentioned this pull request Jun 6, 2018

filebeat docker input stores wrong offset #7045

Closed

exekias added bug needs_backport PR is waiting to be backported to other branches. v6.3.0 and removed enhancement labels Jun 6, 2018

elastic deleted a comment from houndci-bot Jun 6, 2018

exekias added review and removed in progress Pull request is currently in progress. labels Jun 7, 2018

houndci-bot reviewed Jun 7, 2018

View reviewed changes

exekias changed the title ~~Filebeat registry meta~~ Keep different registry entry per container stream Jun 7, 2018

exekias force-pushed the filebeat-registry-meta branch 4 times, most recently from 36773a7 to 6d23c58 Compare June 7, 2018 11:13

urso reviewed Jun 7, 2018

View reviewed changes

exekias force-pushed the filebeat-registry-meta branch 3 times, most recently from e23da83 to 3c9e268 Compare June 7, 2018 14:42

exekias force-pushed the filebeat-registry-meta branch 2 times, most recently from 447d970 to 966866d Compare June 8, 2018 09:35

Carlos Pérez-Aradros Herce added 3 commits June 8, 2018 13:18

Add metadata map to prospector state

9d7b607

This allows us to tag a source file with different metadata, to keep independent read streams.

Include container stream in prospector meta

3193611

Configs using multiple streams won't share state, which was causing issues.

Update CHANGELOG.asciidoc

8cfe2d7

Carlos Pérez-Aradros Herce added 4 commits June 8, 2018 13:18

Add tests

b1e4c94

Apply performance improvements

46c9c45

Do not add metadata for stream: all

a2785d2

Do not use reflect to check empty state

f66d075

exekias force-pushed the filebeat-registry-meta branch from 966866d to f66d075 Compare June 8, 2018 11:18

urso requested a review from ruflin June 8, 2018 15:22

urso reviewed Jun 8, 2018

View reviewed changes

ruflin reviewed Jun 11, 2018

View reviewed changes

ruflin merged commit d37c114 into elastic:master Jun 12, 2018

exekias mentioned this pull request Jun 12, 2018

Cherry-pick #7281 to 6.3: Keep different registry entry per container stream #7300

Merged

exekias added v6.3.0 and removed needs_backport PR is waiting to be backported to other branches. labels Jun 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep different registry entry per container stream #7281

Keep different registry entry per container stream #7281

exekias commented Jun 6, 2018

exekias commented Jun 6, 2018 •

edited

Loading

houndci-bot Jun 7, 2018

urso Jun 7, 2018

exekias Jun 7, 2018

urso Jun 7, 2018

urso Jun 7, 2018

exekias Jun 7, 2018

urso Jun 7, 2018

exekias Jun 7, 2018

exekias Jun 8, 2018 •

edited

Loading

exekias commented Jun 8, 2018

exekias commented Jun 8, 2018

exekias commented Jun 8, 2018

urso Jun 8, 2018

urso commented Jun 8, 2018

ruflin left a comment

ruflin Jun 11, 2018

exekias Jun 12, 2018

exekias commented Jun 12, 2018 •

edited

Loading

Keep different registry entry per container stream #7281

Keep different registry entry per container stream #7281

Conversation

exekias commented Jun 6, 2018

exekias commented Jun 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

exekias commented Jun 8, 2018

exekias commented Jun 8, 2018

exekias commented Jun 8, 2018

Choose a reason for hiding this comment

urso commented Jun 8, 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

exekias commented Jun 12, 2018 • edited Loading

exekias commented Jun 6, 2018 •

edited

Loading

exekias Jun 8, 2018 •

edited

Loading

exekias commented Jun 12, 2018 •

edited

Loading