Add Docker container environment variables as tags. Only whitelisted #2580 #2581

rsingh2411 · 2017-03-28T10:13:57Z

Add Docker container environment variables as tags. Only whitelisted #2580
PR for the above issue.

Required for all PRs:

CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
Sign CLA (if not already signed)

danielnelson

Looks good, just a few minor changes needed.

danielnelson · 2017-03-31T21:28:41Z

plugins/inputs/docker/docker.go

@@ -98,6 +111,8 @@ var sampleConfig = `
  perdevice = true
  ## Whether to report for each container total blkio and network stats or not
  total = false
+  ## Which environment variables should we use as a tag
+  gather_environment = ["ENV", "DC"]


Comment this line so its not on by default, change the names to something a little more standard so its extra clear these are environment variables.

danielnelson · 2017-03-31T21:30:24Z

plugins/inputs/docker/docker.go

+	if len(d.GatherEnvironment) > 0 {
+		info, err := inspectWrapper(d.client, ctx, container.ID)
+		if err != nil {
+			return fmt.Errorf("Error getting docker container inspect: %s", err.Error())


The wording here is confusing, I suggest "Error inspecting docker container: %s"

danielnelson · 2017-03-31T22:25:21Z

plugins/inputs/docker/docker.go

+
+		for _, envvar := range info.Config.Env {
+			kvs := strings.SplitN(envvar, "=", 2)
+			if !sliceContains(kvs[0], d.GatherEnvironment) {


Don't use this function since it is private to the libraries docker package, its just a coincidence that we are also in a docker package. I think this loop would be better if we ranged over d.GatherEnvironment and split only if the prefix matches.

danielnelson · 2017-03-31T22:34:30Z

plugins/inputs/docker/docker.go

+			if !sliceContains(kvs[0], d.GatherEnvironment) {
+				continue
+			}
+			tags[kvs[0]] = kvs[1]


Lets be extra careful and check the len of kvs, since SplitN could return a slice of length 1.

danielnelson · 2017-03-31T22:41:23Z

plugins/inputs/docker/docker_test.go

-		testing: true,
+		client:            nil,
+		testing:           true,
+		GatherEnvironment: []string{"ENVVAR1", "ENVVAR2"},


Can you add tests for envvars that are not found in the container, envvars with zero length values, multiple '=' signs, and any other edge cases you can think of.

I have added mentioned negative edge cases.

danielnelson · 2017-03-31T22:44:55Z

Please update the docker README.md too.

rsingh2411 · 2017-04-03T20:41:24Z

Review changes are incorporated. Please have a look

rsingh2411 · 2017-04-03T21:42:31Z

There were some conflicts due to Docker: optionally add labels as tags (#2425) ,
apologies for redundant commits. Tests are passing successfully

danielnelson · 2017-04-04T01:09:19Z

plugins/inputs/docker/README.md

@@ -30,12 +30,13 @@ for the stat structure can be found
  perdevice = true
  ## Whether to report for each container total blkio and network stats or not
  total = false
+  ## Which environment variables should we use as a tag
+  gather_environment = ["JAVA_HOME", "HEAP_SIZE"]


Maybe we should rename this option to tag_env?

danielnelson · 2017-04-04T01:12:18Z

plugins/inputs/docker/docker.go

+		}
+		for _, envvar := range info.Config.Env {
+			for _, prefix := range d.GatherEnvironment {
+				if strings.Count(envvar, "=") > 1 {


I think we should't skip environment variables like: FOO=BAR=BAZ, also add a testcase for this.

Done, added the testcase

danielnelson · 2017-04-04T01:13:55Z

plugins/inputs/docker/docker.go

+				if strings.Count(envvar, "=") > 1 {
+					continue
+				}
+				if strings.HasPrefix(envvar, prefix) {


I think items with a common prefix will get picked up. Try adding a test case for if there are environment variables FOO and FOOBAR, and specify FOO in the config.

Added an testcase for this as well

rsingh2411 · 2017-05-10T13:14:00Z

I have incorporated review changes. Testing failed due to Test killed with quit: ran too long
coudn't re run as didn't have privelages

danielnelson · 2017-05-10T23:51:53Z

plugins/inputs/docker/docker_test.go

-		testing: true,
+		client:         nil,
+		testing:        true,
+		TagEnvironment: []string{"ENVVAR1", "ENVVAR2", "ENVVAR3", "ENVVAR4", "ENVVAR5", "ENVVAR6", "ENVVAR7", "ENVVAR8", "ENVVAR9"},


Please line wrap this after each element, not a hard rule but try to stick to about 80 chars per line.

Will be done.

danielnelson · 2017-05-10T23:56:34Z

plugins/inputs/docker/docker_test.go

+			"ENVVAR2":           "dolorsitamet",
+			"ENVVAR3":           "ubuntu:10.04",
+			"ENVVAR6":           " ",
+			"ENVVAR7":           "ENVVAR9",


Value should be ENVVAR8=ENVVAR9

Apologies, didn't get this comment
say ENVVAR7=ENVVAR8=ENVVAR9
then tags would be
ENVVAR7=ENVVAR9
ENVVAR8=ENVVAR9
please correct me if my understanding is wrong.

The tagkey should be ENVVAR7 and the tagvalue should be ENVVAR8=ENVVAR9

danielnelson · 2017-05-10T23:56:49Z

plugins/inputs/docker/docker_test.go

+			"ENVVAR3":           "ubuntu:10.04",
+			"ENVVAR6":           " ",
+			"ENVVAR7":           "ENVVAR9",
+			"ENVVAR8":           "ENVVAR9",


There shouldn't be an ENVVAR8

Apologies, didn't get this comment
say ENVVAR7=ENVVAR8=ENVVAR9
then tags would be
ENVVAR7=ENVVAR9
ENVVAR8=ENVVAR9
please correct me if my understanding is wrong.

danielnelson · 2017-05-12T23:49:16Z

@rsingh2411 Can you add an entry to the 1.4 feature section of the CHANGELOG?

@nhaugo Did you want to review?

…nfluxdata#2580

…nfluxdata#2580 (influxdata#2581)

…2580 (#2581)

rsingh2411 force-pushed the master branch from ded088f to cd0702e Compare March 28, 2017 10:19

nhaugo requested review from nhaugo and danielnelson March 30, 2017 20:45

nhaugo added this to the 1.3.0 milestone Mar 30, 2017

danielnelson suggested changes Mar 31, 2017

View reviewed changes

rsingh2411 force-pushed the master branch from cd0702e to 92ff944 Compare April 3, 2017 19:20

danielnelson suggested changes Apr 4, 2017

View reviewed changes

danielnelson modified the milestones: 1.3.0, 1.4.0 Apr 20, 2017

rsingh2411 force-pushed the master branch from fe479cc to 2098453 Compare May 10, 2017 13:10

danielnelson suggested changes May 10, 2017

View reviewed changes

rsingh2411 force-pushed the master branch from 2098453 to 9f3ed5e Compare May 12, 2017 14:12

danielnelson approved these changes May 12, 2017

View reviewed changes

Add Docker container environment variables as tags. Only whitelisted i…

718a812

…nfluxdata#2580

rsingh2411 force-pushed the master branch from 9f3ed5e to 718a812 Compare May 14, 2017 06:32

danielnelson merged commit 93e638d into influxdata:master May 18, 2017

vlamug pushed a commit to vlamug/telegraf that referenced this pull request May 30, 2017

Add Docker container environment variables as tags. Only whitelisted i…

118e461

…nfluxdata#2580 (influxdata#2581)

jeichorn pushed a commit to jeichorn/telegraf that referenced this pull request Jul 24, 2017

Add Docker container environment variables as tags. Only whitelisted i…

4fa8dc3

…nfluxdata#2580 (influxdata#2581)

maxunt pushed a commit that referenced this pull request Jun 26, 2018

Add Docker container environment variables as tags. Only whitelisted #…

f28f166

…2580 (#2581)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Docker container environment variables as tags. Only whitelisted #2580 #2581

Add Docker container environment variables as tags. Only whitelisted #2580 #2581

rsingh2411 commented Mar 28, 2017 •

edited

Loading

danielnelson left a comment

danielnelson Mar 31, 2017

rsingh2411 Apr 3, 2017

danielnelson Mar 31, 2017

rsingh2411 Apr 3, 2017

danielnelson Mar 31, 2017

rsingh2411 Apr 3, 2017

danielnelson Mar 31, 2017

rsingh2411 Apr 3, 2017

danielnelson Mar 31, 2017

rsingh2411 Apr 3, 2017

danielnelson commented Mar 31, 2017

rsingh2411 commented Apr 3, 2017

rsingh2411 commented Apr 3, 2017 •

edited

Loading

danielnelson Apr 4, 2017

rsingh2411 May 10, 2017

danielnelson Apr 4, 2017

rsingh2411 May 10, 2017

danielnelson Apr 4, 2017

rsingh2411 May 10, 2017

rsingh2411 commented May 10, 2017 •

edited

Loading

danielnelson May 10, 2017

rsingh2411 May 11, 2017

rsingh2411 May 12, 2017

danielnelson May 10, 2017

rsingh2411 May 11, 2017

danielnelson May 11, 2017

rsingh2411 May 12, 2017

danielnelson May 10, 2017

rsingh2411 May 11, 2017

rsingh2411 May 12, 2017

danielnelson commented May 12, 2017

Add Docker container environment variables as tags. Only whitelisted #2580 #2581

Add Docker container environment variables as tags. Only whitelisted #2580 #2581

Conversation

rsingh2411 commented Mar 28, 2017 • edited Loading

Required for all PRs:

danielnelson 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

danielnelson commented Mar 31, 2017

rsingh2411 commented Apr 3, 2017

rsingh2411 commented Apr 3, 2017 • 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

rsingh2411 commented May 10, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnelson commented May 12, 2017

rsingh2411 commented Mar 28, 2017 •

edited

Loading

rsingh2411 commented Apr 3, 2017 •

edited

Loading

rsingh2411 commented May 10, 2017 •

edited

Loading