Add extra fields based on label and env for gelf/fluentd/json-file/journald log drivers #15975

dqminh · 2015-09-01T12:31:35Z

This adds container env and labels to log context, and use the data in gelf and fluentd log driver to expose more logging data.

Usage:

gelf: --log-opt gelf-labels=label1,label2 --log-opt gelf-env=env1,env2
fluentd: --log-opt fluentd-labels=label1,label2 --log-opt fluentd-env=env1,env2

The above instructions add label1, label2, env1, env2 key-value pair to log.

cc @LK4D4 @tagomoris @mariussturm

cpuguy83 · 2015-09-01T12:39:21Z

I think any changes here would need to depend on #15384

mariussturm · 2015-09-01T15:34:09Z

@dqminh extra fields in a GELF message have to start with a _. In the current PR you can pick any name, right?

dqminh · 2015-09-01T15:39:29Z

@mariussturm ah yes, i forgot about that specification ( seems like the log servers still happily accept it though ). I think user can pick names, but we can force that the fields we sent is prefixed with _. wdyt ?

mariussturm · 2015-09-01T15:42:17Z

Yes, I would force the prefix. Thanks!

tagomoris · 2015-09-01T15:54:46Z

LGTM 👍

mariussturm · 2015-09-01T16:23:20Z

Another thing, since the fluentd-labels/env and the gelf-labels/env behave in the same way, there is no need for different option names. I think it's more important to point out that the label/env options hook into the Docker metadata system. Maybe a generic name like --log-opt append-labels would be clearer for the user?

dqminh · 2015-09-01T16:33:21Z

@mariussturm however not all log drivers can support this though. I think journald can with the same changes, but json file and syslog cannot. Not sure if we can apply a global log opt here.

mariussturm · 2015-09-01T17:01:24Z

It's the same with gelf-tag and fluentd-tag option, both will be deprecated in favour of tag: #15384

dqminh · 2015-09-01T17:04:03Z

@mariussturm tag can be applied universally though, this is not applicable to syslog / json-file afaik.

dqminh · 2015-09-03T15:37:05Z

@mariussturm added support for _ prefix in additional field names.

dqminh · 2015-09-10T09:23:57Z

Obligatory ping @mariussturm @cpuguy83 😄

Not sure which order we want to merge this vs #15384 . IMO this is much more simpler and useful than #15384 for ingesters want to consume structured log with injected metadata ( i.e. not applicable to syslog )

mariussturm · 2015-09-10T09:45:40Z

The GELF part LGTM. But as I said parameter name feels inconsistent to me. Especially when you combine it with the tag option: --log-opt tag vs. --log-opt gelf-labels
As a user: why one with protocol prefix and the other without?
But this is something @calavera or @cpuguy83 has to decide.

calavera · 2015-09-10T17:10:20Z

I'd say that if an option is used by more than one driver, it should not have a prefix. I agree with @mariussturm that it feels inconsistent. I think this should be --log-opt labels= and --log-opt env=.

dqminh · 2015-09-10T17:31:07Z

Hmm i dont mind changing it. I jus think that it can be confusing when not all drivers can support this, for example syslog and json-file, also journald but that can be fixed.
Do you think that is only a doc issue now ?

calavera · 2015-09-10T20:43:31Z

@dqminh maybe it would be even easier to add these options to all drivers rather than just two of them 😸

dqminh · 2015-09-10T21:00:32Z

@calavera we can probably extend json-file, but syslog is problematic though unless we want RFC5424. Feels like that a big change.

calavera · 2015-09-17T15:59:11Z

@dqminh let's go with json-file only then. We can always iterate on syslog if we need to.

calavera · 2015-09-18T21:51:28Z

I added this PR to the 1.9 milestone. I think it would be a nice feature to have in systems like Amazon ECS #15153.

@dqminh feel free to reach me if you need help with this.

calavera · 2015-09-24T16:59:55Z

@dqminh is there any progress on this?

dqminh · 2015-09-25T00:12:05Z

@calavera updated to labels/env log options and add it to journald and json-file too.

calavera · 2015-09-25T18:02:02Z

daemon/logger/jsonfilelog/jsonfilelog.go

+	var extra []byte
+	if attrs := ctx.ExtraAttributes(nil); len(attrs) > 0 {
+		var err error
+		extra, err = json.Marshal(extra)


shouldn't this be extra, err = json.Marshal(attrs) ?

calavera · 2015-09-25T18:03:11Z

Is there a way we can have tests and documentation for this? I just found an issue that would have been quickly detected with some unit tests.

dqminh · 2015-09-30T12:28:14Z

@calavera fixed and added some tests last week ( sorry forgot to ping :( )

This doesnt have any documentation yet, but I dont have much cycle on this for this week, so feel free to take over if you want. Otherwise let me know and i will try to find some time for it.

calavera · 2015-09-30T22:43:10Z

code LGTM, but I'm not going to be able to carry it in, because I'm going to be offline for a couple of weeks.

vdemeester · 2015-10-01T08:27:36Z

daemon/logger/context.go

+				envMapping[kv[0]] = kv[1]
+			}
+		}
+		for _, l := range strings.Split(env, ",") {


Maybe could factorize this in a function as it's the same is few lines upwards.

vdemeester · 2015-10-01T08:35:27Z

Except from the small comments, code LGTM too.
@calavera @dqminh I could carry it, but wondering if the commits should be squashed or not ?

cpuguy83 · 2015-10-01T13:20:23Z

Yes, @dqminh Do you have time to squash commits before someone carries?

Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

this allows gelf logger to collect extra metadata from containers with `--log-opt labels=label1,label2 --log-opt env=env1,env2` Additional log field will be prefixed with `_` as per gelf protocol https://www.graylog.org/resources/gelf/ Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

this allows fluentd logger to collect extra metadata from containers with `--log-opt labels=label1,label2 --log-opt env=env1,env2` Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

this allows journald logger to collect extra metadata from containers with `--log-opt labels=label1,label2 --log-opt env=env1,env2` Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

this allows jsonfile logger to collect extra metadata from containers with `--log-opt labels=label1,label2 --log-opt env=env1,env2`. Extra attributes are saved into `attrs` attributes for each log data. Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

dqminh · 2015-10-04T21:12:44Z

Squashed and rebased.

icecrime · 2015-10-10T01:02:08Z

Moving to docs review, thanks @dqminh!

thaJeztah · 2015-10-12T18:00:54Z

@dqminh I don't see any docs yet; can you add a description of the new options to
https://github.com/docker/docker/blob/master/docs/reference/logging/overview.md, also, IIUC, env and label can now be used in --log-opt tag, so they need to be added to the table, and here; https://github.com/docker/docker/blob/master/docs/reference/logging/log_tags.md

jessfraz · 2015-10-12T20:50:04Z

@dqminh if you get this done by today we can try and get it in 1.9 before code freeze

vdemeester · 2015-10-12T20:50:55Z

@jfrazelle @dqminh I'm gonna carry it for the docs ; to make sure it makes it 😉.

jessfraz · 2015-10-12T20:51:31Z

thanks @vdemeester

jessfraz · 2015-10-12T22:47:25Z

closing because #16961 is updated thanks @dqminh

Carry #15975 - Add extra fields based on label and env for gelf/fluentd/json-file/journald log drivers

GordonTheTurtle added the status/0-triage label Sep 1, 2015

cpuguy83 added status/1-design-review and removed status/0-triage labels Sep 1, 2015

dqminh force-pushed the log-driver-extra-fields branch from 8a0b7da to 0aecddc Compare September 1, 2015 14:18

dqminh force-pushed the log-driver-extra-fields branch from 23d9a10 to 470da99 Compare September 2, 2015 14:26

calavera added status/2-code-review impact/changelog and removed status/1-design-review labels Sep 16, 2015

calavera mentioned this pull request Sep 18, 2015

Allow access to full logging context in fluentd-tag. #15153

Closed

calavera added this to the 1.9.0 milestone Sep 18, 2015

runcom mentioned this pull request Sep 21, 2015

Proposal: Add custom fields to journald logging #16446

Closed

dqminh changed the title ~~Add extra fields based on label and env for gelf/fluentd log drivers~~ Add extra fields based on label and env for gelf/fluentd/json-file/journald log drivers Sep 24, 2015

dqminh force-pushed the log-driver-extra-fields branch 2 times, most recently from ba30b64 to 40470d1 Compare September 25, 2015 00:03

calavera reviewed Sep 25, 2015
View reviewed changes

dqminh force-pushed the log-driver-extra-fields branch from 40470d1 to 4b8ab62 Compare September 26, 2015 00:43

vdemeester reviewed Oct 1, 2015
View reviewed changes

dqminh added 5 commits October 4, 2015 21:12

add env and labels to log context

835956f

Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

add labels/env log option for fluentd

ce1a1b0

this allows fluentd logger to collect extra metadata from containers with `--log-opt labels=label1,label2 --log-opt env=env1,env2` Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

add labels/env log option for journald

15fad1e

this allows journald logger to collect extra metadata from containers with `--log-opt labels=label1,label2 --log-opt env=env1,env2` Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

dqminh force-pushed the log-driver-extra-fields branch from 4b8ab62 to 056dfb0 Compare October 4, 2015 21:12

icecrime added status/3-docs-review and removed status/2-code-review labels Oct 10, 2015

vdemeester mentioned this pull request Oct 12, 2015

Carry #15975 - Add extra fields based on label and env for gelf/fluentd/json-file/journald log drivers #16961

Merged

jessfraz closed this Oct 12, 2015

thaJeztah added a commit that referenced this pull request Oct 13, 2015

Merge pull request #16961 from vdemeester/pr-15975-carry-for-docs

3856c5e

Carry #15975 - Add extra fields based on label and env for gelf/fluentd/json-file/journald log drivers

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extra fields based on label and env for gelf/fluentd/json-file/journald log drivers #15975

Add extra fields based on label and env for gelf/fluentd/json-file/journald log drivers #15975

dqminh commented Sep 1, 2015

cpuguy83 commented Sep 1, 2015

mariussturm commented Sep 1, 2015

dqminh commented Sep 1, 2015

mariussturm commented Sep 1, 2015

tagomoris commented Sep 1, 2015

mariussturm commented Sep 1, 2015

dqminh commented Sep 1, 2015

mariussturm commented Sep 1, 2015

dqminh commented Sep 1, 2015

dqminh commented Sep 3, 2015

dqminh commented Sep 10, 2015

mariussturm commented Sep 10, 2015

calavera commented Sep 10, 2015

dqminh commented Sep 10, 2015

calavera commented Sep 10, 2015

dqminh commented Sep 10, 2015

calavera commented Sep 17, 2015

calavera commented Sep 18, 2015

calavera commented Sep 24, 2015

dqminh commented Sep 25, 2015

calavera Sep 25, 2015

calavera commented Sep 25, 2015

dqminh commented Sep 30, 2015

calavera commented Sep 30, 2015

vdemeester Oct 1, 2015

vdemeester commented Oct 1, 2015

cpuguy83 commented Oct 1, 2015

dqminh commented Oct 4, 2015

icecrime commented Oct 10, 2015

thaJeztah commented Oct 12, 2015

jessfraz commented Oct 12, 2015

vdemeester commented Oct 12, 2015

jessfraz commented Oct 12, 2015

jessfraz commented Oct 12, 2015

Add extra fields based on label and env for gelf/fluentd/json-file/journald log drivers #15975

Add extra fields based on label and env for gelf/fluentd/json-file/journald log drivers #15975

Conversation

dqminh commented Sep 1, 2015

cpuguy83 commented Sep 1, 2015

mariussturm commented Sep 1, 2015

dqminh commented Sep 1, 2015

mariussturm commented Sep 1, 2015

tagomoris commented Sep 1, 2015

mariussturm commented Sep 1, 2015

dqminh commented Sep 1, 2015

mariussturm commented Sep 1, 2015

dqminh commented Sep 1, 2015

dqminh commented Sep 3, 2015

dqminh commented Sep 10, 2015

mariussturm commented Sep 10, 2015

calavera commented Sep 10, 2015

dqminh commented Sep 10, 2015

calavera commented Sep 10, 2015

dqminh commented Sep 10, 2015

calavera commented Sep 17, 2015

calavera commented Sep 18, 2015

calavera commented Sep 24, 2015

dqminh commented Sep 25, 2015

calavera Sep 25, 2015

Choose a reason for hiding this comment

calavera commented Sep 25, 2015

dqminh commented Sep 30, 2015

calavera commented Sep 30, 2015

vdemeester Oct 1, 2015

Choose a reason for hiding this comment

vdemeester commented Oct 1, 2015

cpuguy83 commented Oct 1, 2015

dqminh commented Oct 4, 2015

icecrime commented Oct 10, 2015

thaJeztah commented Oct 12, 2015

jessfraz commented Oct 12, 2015

vdemeester commented Oct 12, 2015

jessfraz commented Oct 12, 2015

jessfraz commented Oct 12, 2015