- Updates based on code review

influxdata · Mar 25, 2017 · cdd504d · cdd504d
1 parent 44f3c63
commit cdd504d
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 78 deletions.
diff --git a/plugins/inputs/docker/README.md b/plugins/inputs/docker/README.md
@@ -148,6 +148,7 @@ based on the availability of per-cpu stats on your system.
     - engine_host
 - docker_metadata
     - unit=bytes
+    - engine_host
 
 #### Docker Container tags
 - Tags on all containers:

diff --git a/plugins/inputs/docker/docker.go b/plugins/inputs/docker/docker.go
@@ -112,7 +112,6 @@ var sampleConfig = `
   ## Note that an empty array for both will include all labels as tags
   docker_label_include = []
   docker_label_exclude = []
-
 `
 
 // Description returns input description
@@ -150,11 +149,19 @@ func (d *Docker) Gather(acc telegraf.Accumulator) error {
 
 	// Create label filters
 	if len(d.LabelInclude) != 0 {
-		d.LabelFilter.labelInclude, _ = filter.Compile(d.LabelInclude)
+		var err error
+		d.LabelFilter.labelInclude, err = filter.Compile(d.LabelInclude)
+		if err != nil {
+			return err
+		}
 	}
 
 	if len(d.LabelExclude) != 0 {
-		d.LabelFilter.labelExclude, _ = filter.Compile(d.LabelExclude)
+		var err error
+		d.LabelFilter.labelExclude, err = filter.Compile(d.LabelExclude)
+		if err != nil {
+			return err
+		}
 	}
 
 	// Get daemon info
@@ -631,7 +638,6 @@ func init() {
 		return &Docker{
 			PerDevice: true,
 			Timeout:   internal.Duration{Duration: time.Second * 5},
-			//AddLabels: true,
 		}
 	})
 }
diff --git a/plugins/inputs/docker/docker_test.go b/plugins/inputs/docker/docker_test.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/docker/docker/api/types"
 	"github.com/stretchr/testify/require"
+	"fmt"
 )
 
 func TestDockerGatherContainerStats(t *testing.T) {
@@ -244,77 +245,60 @@ func testStats() *types.StatsJSON {
 	return stats
 }
 
-func TestDockerGatherLabels(t *testing.T) {
-	var acc testutil.Accumulator
-	d := Docker{
-		client:  nil,
-		testing: true,
-	}
-
-	// Default case should include both labels
-	err := d.Gather(&acc)
-	require.NoError(t, err)
-
-	acc.AssertContainsTaggedFields(t,
-		"docker_container_cpu",
-		map[string]interface{}{
-			"usage_total":  uint64(1231652),
-			"container_id": "b7dfbb9478a6ae55e237d4d74f8bbb753f0817192b5081334dc78476296e2173",
-		},
-		map[string]string{
-			"container_name":    "etcd2",
-			"container_image":   "quay.io:4443/coreos/etcd",
-			"cpu":               "cpu3",
-			"container_version": "v2.2.2",
-			"engine_host":       "absol",
-			"label1":            "test_value_1",
-			"label2":            "test_value_2",
-		},
-	)
+var gatherLabelsTests = []struct {
+	include     []string
+	exclude     []string
+	expected    []string
+	notexpected []string
+}{
+	{[]string{},          []string{},         []string{"label1","label2"}, []string{}},
+	{[]string{"*"},       []string{},         []string{"label1","label2"}, []string{}},
+	{[]string{"lab*"},    []string{},         []string{"label1","label2"}, []string{}},
+	{[]string{"label1"},  []string{},         []string{"label1"},          []string{"label2"}},
+	{[]string{"label1*"}, []string{},         []string{"label1"},          []string{"label2"}},
+	{[]string{},          []string{"*"},      []string{},                  []string{"label1","label2"}},
+	{[]string{},          []string{"lab*"},   []string{},                  []string{"label1","label2"}},
+	{[]string{},          []string{"label1"}, []string{"label2"},          []string{"label1"}},
+	{[]string{"*"},       []string{"*"},      []string{},                  []string{"label1","label2"}},
 
-	// Include should only include label1
-	d.LabelInclude = append(d.LabelInclude, "label1")
-	err1 := d.Gather(&acc)
-	require.NoError(t, err1)
-
-	acc.AssertContainsTaggedFields(t,
-		"docker_container_cpu",
-		map[string]interface{}{
-			"usage_total":  uint64(1231652),
-			"container_id": "b7dfbb9478a6ae55e237d4d74f8bbb753f0817192b5081334dc78476296e2173",
-		},
-		map[string]string{
-			"container_name":    "etcd2",
-			"container_image":   "quay.io:4443/coreos/etcd",
-			"cpu":               "cpu3",
-			"container_version": "v2.2.2",
-			"engine_host":       "absol",
-			"label1":            "test_value_1",
-		},
-	)
-
-	// Exclude should only include label2.
-	d.LabelInclude[0] = "*"
-	d.LabelExclude = append(d.LabelExclude, "label1")
-	err2 := d.Gather(&acc)
-	require.NoError(t, err2)
-
-	acc.AssertContainsTaggedFields(t,
-		"docker_container_cpu",
-		map[string]interface{}{
-			"usage_total":  uint64(1231652),
-			"container_id": "b7dfbb9478a6ae55e237d4d74f8bbb753f0817192b5081334dc78476296e2173",
-		},
-		map[string]string{
-			"container_name":    "etcd2",
-			"container_image":   "quay.io:4443/coreos/etcd",
-			"cpu":               "cpu3",
-			"container_version": "v2.2.2",
-			"engine_host":       "absol",
-			"label2":            "test_value_2",
-		},
-	)
+}
 
+func TestDockerGatherLabels(t *testing.T) {
+	for _, tt := range gatherLabelsTests {
+		var acc testutil.Accumulator
+		d := Docker{
+			client:  nil,
+			testing: true,
+		}
+
+		for _, label := range tt.include {
+			d.LabelInclude = append(d.LabelInclude, label)
+		}
+		for _, label := range tt.exclude {
+			d.LabelExclude = append(d.LabelExclude, label)
+		}
+
+		err := d.Gather(&acc)
+		require.NoError(t, err)
+
+		var expectedErrors []string
+		for _, label := range tt.expected {
+			if !acc.HasTag("docker_container_cpu", label) {
+				expectedErrors = append(expectedErrors, fmt.Sprintf("%s ", label))
+			}
+		}
+
+		var notexpectedErrors []string
+		for _, label := range tt.notexpected {
+			if acc.HasTag("docker_container_cpu", label) {
+				notexpectedErrors = append(notexpectedErrors, fmt.Sprintf("%s ", label))
+			}
+		}
+
+		if len(expectedErrors) > 0 || len(notexpectedErrors) > 0 {
+			t.Errorf("Failed test for: Include: %s  Exclude:  %s", tt.include, tt.exclude)
+		}
+	}
 }
 
 func TestDockerGatherInfo(t *testing.T) {
@@ -324,11 +308,6 @@ func TestDockerGatherInfo(t *testing.T) {
 		testing: true,
 	}
 
-	/* In order to not modify code here, we set this to false since the container structure now has
-	   labels and the default is true.
-	*/
-
-	d.LabelExclude = append(d.LabelExclude, "*")
 	err := d.Gather(&acc)
 	require.NoError(t, err)
 
@@ -372,6 +351,8 @@ func TestDockerGatherInfo(t *testing.T) {
 			"cpu":               "cpu3",
 			"container_version": "v2.2.2",
 			"engine_host":       "absol",
+			"label1":            "test_value_1",
+			"label2":            "test_value_2",
 		},
 	)
 	acc.AssertContainsTaggedFields(t,
@@ -418,6 +399,8 @@ func TestDockerGatherInfo(t *testing.T) {
 			"container_name":    "etcd2",
 			"container_image":   "quay.io:4443/coreos/etcd",
 			"container_version": "v2.2.2",
+			"label1":            "test_value_1",
+			"label2":            "test_value_2",
 		},
 	)