From 04be47ca2f5f8351a1de4b6959905bc8e41df80c Mon Sep 17 00:00:00 2001 From: George MacRorie Date: Wed, 17 Jul 2019 15:51:38 +0200 Subject: [PATCH 1/3] Add failing test for internal templating wildcard match bug --- internal/templating/engine_test.go | 44 ++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/internal/templating/engine_test.go b/internal/templating/engine_test.go index b7dd23f384e67..933a7a22edba0 100644 --- a/internal/templating/engine_test.go +++ b/internal/templating/engine_test.go @@ -3,6 +3,7 @@ package templating import ( "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -20,3 +21,46 @@ func TestEngineAlternateSeparator(t *testing.T) { }, tags) require.Equal(t, "", field) } + +func TestEngineWithWildcardTemplate(t *testing.T) { + var ( + defaultTmpl, err = NewDefaultTemplateWithPattern("measurement*") + templates = []string{ + "taskmanagerTask.alarm-detector.Assign.alarmDefinitionId metricsType.process.nodeId.x.alarmDefinitionId.measurement.field rule=1", + "taskmanagerTask.*.*.*.* metricsType.process.nodeId.measurement rule=2", + } + + lineOne = "taskmanagerTask.alarm-detector.Assign.alarmDefinitionId.timeout_errors.duration.p75" + lineTwo = "taskmanagerTask.alarm-detector.Assign.numRecordsInPerSecond.m5_rate" + ) + require.NoError(t, err) + + engine, err := NewEngine(".", defaultTmpl, templates) + require.NoError(t, err) + + measurement, tags, field, err := engine.Apply(lineOne) + require.NoError(t, err) + + assert.Equal(t, "duration", measurement) + assert.Equal(t, "p75", field) + assert.Equal(t, map[string]string{ + "metricsType": "taskmanagerTask", + "process": "alarm-detector", + "nodeId": "Assign", + "x": "alarmDefinitionId", + "alarmDefinitionId": "timeout_errors", + "rule": "1", + }, tags) + + measurement, tags, field, err = engine.Apply(lineTwo) + require.NoError(t, err) + + assert.Equal(t, "numRecordsInPerSecond", measurement) + assert.Equal(t, "", field) + assert.Equal(t, map[string]string{ + "metricsType": "taskmanagerTask", + "process": "alarm-detector", + "nodeId": "Assign", + "rule": "2", + }, tags) +} From 7dfd47ed2b489d8bd64f915a875d49c444beb007 Mon Sep 17 00:00:00 2001 From: George MacRorie Date: Wed, 17 Jul 2019 15:54:19 +0200 Subject: [PATCH 2/3] Ensure internal templating explores wildcard nodes in case of multiple match --- internal/templating/node.go | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/internal/templating/node.go b/internal/templating/node.go index 83ab1a40cda4e..844b314948d53 100644 --- a/internal/templating/node.go +++ b/internal/templating/node.go @@ -55,32 +55,42 @@ func (n *node) search(line string) *Template { // recursiveSearch performs the actual recursive search func (n *node) recursiveSearch(lineParts []string) *Template { - // Nothing to search + // nothing to search if len(lineParts) == 0 || len(n.children) == 0 { return n.template } - // If last element is a wildcard, don't include it in this search since it's sorted - // to the end but lexicographically it would not always be and sort.Search assumes - // the slice is sorted. - length := len(n.children) - if n.children[length-1].value == "*" { + var ( + hasWildcard bool + length = len(n.children) + ) + + // search children excluding wildcard match has been + // artifically sorted to the end of the children set + if hasWildcard = n.children[length-1].value == "*"; hasWildcard { length-- } - // Find the index of child with an exact match i := sort.Search(length, func(i int) bool { return n.children[i].value >= lineParts[0] }) - // Found an exact match, so search that child sub-tree - if i < len(n.children) && n.children[i].value == lineParts[0] { - return n.children[i].recursiveSearch(lineParts[1:]) + // given an exact match is found within children set + if i < length && n.children[i].value == lineParts[0] { + // decend into the matching node + if tmpl := n.children[i].recursiveSearch(lineParts[1:]); tmpl != nil { + // given a template is found return it + return tmpl + } } - // Not an exact match, see if we have a wildcard child to search - if n.children[len(n.children)-1].value == "*" { - return n.children[len(n.children)-1].recursiveSearch(lineParts[1:]) + + // if last child is a wildcard + if hasWildcard { + // descend the wildcard node + return n.children[length].recursiveSearch(lineParts[1:]) } + + // fallback to returning template at this node return n.template } From 04148e401d0e801aa9dc40cee785518fd849644d Mon Sep 17 00:00:00 2001 From: George MacRorie Date: Thu, 18 Jul 2019 11:00:07 +0200 Subject: [PATCH 3/3] Convert internal templating engine test into table test --- internal/templating/engine_test.go | 65 +++++++++++++++++------------- internal/templating/node.go | 10 +++-- 2 files changed, 44 insertions(+), 31 deletions(-) diff --git a/internal/templating/engine_test.go b/internal/templating/engine_test.go index 933a7a22edba0..0dfcb89d8d823 100644 --- a/internal/templating/engine_test.go +++ b/internal/templating/engine_test.go @@ -29,38 +29,49 @@ func TestEngineWithWildcardTemplate(t *testing.T) { "taskmanagerTask.alarm-detector.Assign.alarmDefinitionId metricsType.process.nodeId.x.alarmDefinitionId.measurement.field rule=1", "taskmanagerTask.*.*.*.* metricsType.process.nodeId.measurement rule=2", } - - lineOne = "taskmanagerTask.alarm-detector.Assign.alarmDefinitionId.timeout_errors.duration.p75" - lineTwo = "taskmanagerTask.alarm-detector.Assign.numRecordsInPerSecond.m5_rate" ) require.NoError(t, err) engine, err := NewEngine(".", defaultTmpl, templates) require.NoError(t, err) - measurement, tags, field, err := engine.Apply(lineOne) - require.NoError(t, err) - - assert.Equal(t, "duration", measurement) - assert.Equal(t, "p75", field) - assert.Equal(t, map[string]string{ - "metricsType": "taskmanagerTask", - "process": "alarm-detector", - "nodeId": "Assign", - "x": "alarmDefinitionId", - "alarmDefinitionId": "timeout_errors", - "rule": "1", - }, tags) + for _, testCase := range []struct { + line string + measurement string + field string + tags map[string]string + }{ + { + line: "taskmanagerTask.alarm-detector.Assign.alarmDefinitionId.timeout_errors.duration.p75", + measurement: "duration", + field: "p75", + tags: map[string]string{ + "metricsType": "taskmanagerTask", + "process": "alarm-detector", + "nodeId": "Assign", + "x": "alarmDefinitionId", + "alarmDefinitionId": "timeout_errors", + "rule": "1", + }, + }, + { + line: "taskmanagerTask.alarm-detector.Assign.numRecordsInPerSecond.m5_rate", + measurement: "numRecordsInPerSecond", + tags: map[string]string{ + "metricsType": "taskmanagerTask", + "process": "alarm-detector", + "nodeId": "Assign", + "rule": "2", + }, + }, + } { + t.Run(testCase.line, func(t *testing.T) { + measurement, tags, field, err := engine.Apply(testCase.line) + require.NoError(t, err) - measurement, tags, field, err = engine.Apply(lineTwo) - require.NoError(t, err) - - assert.Equal(t, "numRecordsInPerSecond", measurement) - assert.Equal(t, "", field) - assert.Equal(t, map[string]string{ - "metricsType": "taskmanagerTask", - "process": "alarm-detector", - "nodeId": "Assign", - "rule": "2", - }, tags) + assert.Equal(t, testCase.measurement, measurement) + assert.Equal(t, testCase.field, field) + assert.Equal(t, testCase.tags, tags) + }) + } } diff --git a/internal/templating/node.go b/internal/templating/node.go index 844b314948d53..53d028fd0fb8f 100644 --- a/internal/templating/node.go +++ b/internal/templating/node.go @@ -65,8 +65,10 @@ func (n *node) recursiveSearch(lineParts []string) *Template { length = len(n.children) ) - // search children excluding wildcard match has been - // artifically sorted to the end of the children set + // exclude last child from search if it is a wildcard. sort.Search expects + // a lexicographically sorted set of children and we have artificially sorted + // wildcards to the end of the child set + // wildcards will be searched seperately if no exact match is found if hasWildcard = n.children[length-1].value == "*"; hasWildcard { length-- } @@ -84,9 +86,9 @@ func (n *node) recursiveSearch(lineParts []string) *Template { } } - // if last child is a wildcard + // given no template is found and the last child is a wildcard if hasWildcard { - // descend the wildcard node + // also search the wildcard child node return n.children[length].recursiveSearch(lineParts[1:]) }