Repository: NetApp/harvest
Pull request #336 (2021-07-22 14:03:52)

fix: false negatives by prefix matcher (second PR for release branch)
Same PR as #333, but for the release branch.


Patch:
diff --git a/cmd/tools/grafana/grafana.go b/cmd/tools/grafana/grafana.go
index dbe89351..f34fc47f 100644
--- a/cmd/tools/grafana/grafana.go
+++ b/cmd/tools/grafana/grafana.go
@@ -274,13 +274,8 @@ func addGlobalPrefix(db map[string]interface{}, prefix string) {
 		p, t                                       interface{}
 		queryString, definition, expr              string
 		ok, has                                    bool
-		regex                                      *regexp.Regexp
 	)
 
-	// regex we will use to match metric names
-	// used in addPrefixToMatricNames, but better to initialize it once
-	regex = regexp.MustCompile(`([a-zA-Z_+]+){.+?}`)
-
 	// make sure prefix ends with _
 	if !strings.HasSuffix(prefix, "_") {
 		prefix += "_"
@@ -312,7 +307,7 @@ func addGlobalPrefix(db map[string]interface{}, prefix string) {
 
 			if _, has = target["expr"]; has {
 				if expr, ok = target["expr"].(string); ok {
-					target["expr"] = addPrefixToMetricNames(expr, prefix, regex)
+					target["expr"] = addPrefixToMetricNames(expr, prefix)
 				}
 			}
 		}
@@ -330,32 +325,39 @@ func addGlobalPrefix(db map[string]interface{}, prefix string) {
 	for _, t = range templates {
 		if template, ok = t.(map[string]interface{}); ok {
 			if definition, ok = template["definition"].(string); ok {
-				template["definition"] = addPrefixToMetricNames(definition, prefix, regex)
+				template["definition"] = addPrefixToMetricNames(definition, prefix)
 			}
 			if query, ok = template["query"].(map[string]interface{}); ok {
 				if queryString, ok = query["query"].(string); ok {
-					query["query"] = addPrefixToMetricNames(queryString, prefix, regex)
+					query["query"] = addPrefixToMetricNames(queryString, prefix)
 				}
 			}
 		}
 	}
 }
 
-// addPrefixToMetricNames adds prefix to metric names in expr. Note that
-// this function will only work with the Prometheus-dashboards of Harvest.
-// It will recognize a number of ways in which metrics are used as queries.
+// addPrefixToMetricNames adds prefix to metric names in expr or leaves it
+// unchanged if no metric names are identified.
+// Note that this function will only work with the Prometheus-dashboards of Harvest.
+// It will use a number of patterns in which metrics might be used in queries.
 // (E.g. a single metric, multiple metrics used in addition, etc -- for examples
 // see the test). If we change queries of our dashboards, we have to review
 // this function as well (or come up with a better solution).
-func addPrefixToMetricNames(expr, prefix string, regex *regexp.Regexp) string {
+func addPrefixToMetricNames(expr, prefix string) string {
 	var (
 		match    [][]string
 		submatch []string
+		isMatch  bool
+		regex    *regexp.Regexp
+		err      error
 	)
 
 	// variable queries
 	if strings.HasPrefix(expr, "label_values(") {
-		if strings.Contains(expr, ", ") {
+		if isMatch, err = regexp.MatchString(`^label_values\s?\(([a-zA-Z_])+(\s?{.+?})?,\s?[a-zA-Z_]+\)$`, expr); err != nil {
+			fmt.Printf("Regex error: %v\n", err)
+			return expr
+		} else if isMatch {
 			return strings.Replace(expr, "label_values(", "label_values("+prefix, 1)
 		} else {
 			// no metric name
@@ -364,6 +366,7 @@ func addPrefixToMetricNames(expr, prefix string, regex *regexp.Regexp) string {
 	}
 
 	// everything else is for graph queries
+	regex = regexp.MustCompile(`([a-zA-Z_+]+)\s?{.+?}`)
 	match = regex.FindAllStringSubmatch(expr, -1)
 
 	for _, m := range match {
diff --git a/grafana/prometheus/harvest_dashboard_metadata.json b/grafana/prometheus/harvest_dashboard_metadata.json
index f3964e33..9593d355 100644
--- a/grafana/prometheus/harvest_dashboard_metadata.json
+++ b/grafana/prometheus/harvest_dashboard_metadata.json
@@ -136,7 +136,7 @@
       "pluginVersion": "7.5.4",
       "targets": [
         {
-          "expr": "count (count by (datacenter) (poller_status))",
+          "expr": "count (count by (datacenter) (poller_status{hostname=~\"$Hostname\"}))",
           "format": "time_series",
           "hide": false,
           "instant": false,
@@ -4392,4 +4392,4 @@
   "title": "Harvest Metadata",
   "uid": "BeIE5HGGzsfdfdd",
   "version": 3
-}
\ No newline at end of file
+}


Test patch:
diff --git a/cmd/tools/grafana/grafana_test.go b/cmd/tools/grafana/grafana_test.go
index 6e239d8e..56403c5f 100644
--- a/cmd/tools/grafana/grafana_test.go
+++ b/cmd/tools/grafana/grafana_test.go
@@ -1,7 +1,6 @@
 package grafana
 
 import (
-	"regexp"
 	"testing"
 )
 
@@ -55,17 +54,14 @@ func TestHttpsAddr(t *testing.T) {
 func TestAddPrefixToMetricNames(t *testing.T) {
 
 	var (
-		examples, expected [7]string
+		examples, expected []string
 		prefix, result     string
-		regex              *regexp.Regexp
 		i                  int
 	)
 
-	regex = regexp.MustCompile(`([a-zA-Z_+]+){.+?}`)
-
 	prefix = "xx_"
 
-	examples = [7]string{
+	examples = []string{
 		`sum(volume_read_data{datacenter=\"$Datacenter\",cluster=~\"$Cluster\"}) by (cluster) + sum(volume_write_data{datacenter=\"$Datacenter\",cluster=~\"$Cluster\"}) by(cluster)`,
 		`sum(topk($TopResources, volume_total_ops{datacenter=\"$Datacenter\",cluster=\"$Cluster\",svm=~\"$SVM\",volume=~\"$Volume\"}))`,
 		`volume_size_used_percent{datacenter=\"$Datacenter\",cluster=\"$Cluster\",svm=~\"$SVM\",volume=~\"$Volume\"}`,
@@ -73,9 +69,12 @@ func TestAddPrefixToMetricNames(t *testing.T) {
 		`label_values(metadata_component_status{type="collector",poller=~"$Poller"}, name)`,
 		`label_values(poller_status, datacenter)`,
 		`label_values(datacenter)`,
+		`label_values(node_uptime{datacenter="$Datacenter"},cluster)`,
+		`label_values (node_uptime{datacenter="$Datacenter"},cluster)`,
+		`label_values(node_uptime {datacenter="$Datacenter"},cluster)`,
 	}
 
-	expected = [7]string{
+	expected = []string{
 		`sum(xx_volume_read_data{datacenter=\"$Datacenter\",cluster=~\"$Cluster\"}) by (cluster) + sum(xx_volume_write_data{datacenter=\"$Datacenter\",cluster=~\"$Cluster\"}) by(cluster)`,
 		`sum(topk($TopResources, xx_volume_total_ops{datacenter=\"$Datacenter\",cluster=\"$Cluster\",svm=~\"$SVM\",volume=~\"$Volume\"}))`,
 		`xx_volume_size_used_percent{datacenter=\"$Datacenter\",cluster=\"$Cluster\",svm=~\"$SVM\",volume=~\"$Volume\"}`,
@@ -83,10 +82,13 @@ func TestAddPrefixToMetricNames(t *testing.T) {
 		`label_values(xx_metadata_component_status{type="collector",poller=~"$Poller"}, name)`,
 		`label_values(xx_poller_status, datacenter)`,
 		`label_values(datacenter)`, // no metric name
+		`label_values(xx_node_uptime{datacenter="$Datacenter"},cluster)`,
+		`label_values (xx_node_uptime{datacenter="$Datacenter"},cluster)`,
+		`label_values(xx_node_uptime {datacenter="$Datacenter"},cluster)`,
 	}
 
 	for i = range examples {
-		result = addPrefixToMetricNames(examples[i], prefix, regex)
+		result = addPrefixToMetricNames(examples[i], prefix)
 		if result != expected[i] {
 			t.Errorf("\nExpected: [%s]\n     Got: [%s]", expected[i], result)
 		}