Merge pull request #2515 from grafana/hotfix/0.38.0/thresholds-url-grouping-parsing

Fix the parsing of threshold name tags containing tokens

Author: oleiade

metrics/metric.go

@@ -133,7 +133,7 @@ var ErrMetricNameParsing = errors.New("parsing metric name failed")
 // of "key:value" strings. On failure, it returns an error containing the `ErrMetricNameParsing` in its chain.
 func ParseMetricName(name string) (string, []string, error) {
 	openingTokenPos := strings.IndexByte(name, '{')
-	closingTokenPos := strings.IndexByte(name, '}')
+	closingTokenPos := strings.LastIndexByte(name, '}')
 	containsOpeningToken := openingTokenPos != -1
 	containsClosingToken := closingTokenPos != -1
 
@@ -147,37 +147,44 @@ func ParseMetricName(name string) (string, []string, error) {
 	// its counterpart, the expression is malformed.
 	if (containsOpeningToken && !containsClosingToken) ||
 		(!containsOpeningToken && containsClosingToken) {
-		return "", nil, fmt.Errorf("%w; reason: unmatched opening/close curly brace", ErrMetricNameParsing)
+		return "", nil, fmt.Errorf(
+			"%w, metric %q has unmatched opening/close curly brace",
+			ErrMetricNameParsing, name,
+		)
 	}
 
+	// If the closing brace token appears before the opening one,
+	// the expression is malformed
 	if closingTokenPos < openingTokenPos {
-		return "", nil, fmt.Errorf("%w; reason: closing curly brace appears before opening one", ErrMetricNameParsing)
+		return "", nil, fmt.Errorf("%w, metric %q closing curly brace appears before opening one", ErrMetricNameParsing, name)
 	}
 
-	parserFn := func(c rune) bool {
-		return c == '{' || c == '}'
+	// If the last character is not a closing brace token,
+	// the expression is malformed.
+	if closingTokenPos != (len(name) - 1) {
+		err := fmt.Errorf(
+			"%w, metric %q lacks a closing curly brace in its last position",
+			ErrMetricNameParsing,
+			name,
+		)
+		return "", nil, err
 	}
 
-	// Split the metric_name{tag_key:tag_value,...} expression
-	// into two "metric_name" and "tag_key:tag_value,..." strings.
-	parts := strings.FieldsFunc(name, parserFn)
-	if len(parts) == 0 || len(parts) > 2 {
-		return "", nil, ErrMetricNameParsing
-	}
-
-	// Split the tag key values
-	tags := strings.Split(parts[1], ",")
+	// We already know the position of the opening and closing curly brace
+	// tokens. Thus, we extract the string in between them, and split its
+	// content to obtain the tags key values.
+	tags := strings.Split(name[openingTokenPos+1:closingTokenPos], ",")
 
-	// For each tag definition, ensure
+	// For each tag definition, ensure it is correctly formed
 	for i, t := range tags {
 		keyValue := strings.SplitN(t, ":", 2)
 
 		if len(keyValue) != 2 || keyValue[1] == "" {
-			return "", nil, fmt.Errorf("%w; reason: malformed tag expression %q", ErrMetricNameParsing, t)
+			return "", nil, fmt.Errorf("%w, metric %q tag expression is malformed", ErrMetricNameParsing, t)
 		}
 
 		tags[i] = strings.TrimSpace(t)
 	}
 
-	return parts[0], tags, nil
+	return name[0:openingTokenPos], tags, nil
 }

metrics/metric_test.go

@@ -111,6 +111,20 @@ func TestParseMetricName(t *testing.T) {
 			wantTags:             []string{"group:::mygroup"},
 			wantErr:              false,
 		},
+		{
+			name:                 "metric name with valid name and repeated curly braces tokens in tags definition",
+			metricNameExpression: "http_req_duration{name:http://${}.com}",
+			wantMetricName:       "http_req_duration",
+			wantTags:             []string{"name:http://${}.com"},
+			wantErr:              false,
+		},
+		{
+			name:                 "metric name with valid name and repeated curly braces and colon tokens in tags definition",
+			metricNameExpression: "http_req_duration{name:http://${}.com,url:ssh://github.com:grafana/k6}",
+			wantMetricName:       "http_req_duration",
+			wantTags:             []string{"name:http://${}.com", "url:ssh://github.com:grafana/k6"},
+			wantErr:              false,
+		},
 		{
 			name:                 "metric name with tag definition missing `:value`",
 			metricNameExpression: "test_metric{easyas}",
@@ -146,6 +160,11 @@ func TestParseMetricName(t *testing.T) {
 			metricNameExpression: "test_metric}abc{bar",
 			wantErr:              true,
 		},
+		{
+			name:                 "metric name with valid name and trailing characters after closing curly brace in tags definition",
+			metricNameExpression: "test_metric{foo:ba}r",
+			wantErr:              true,
+		},
 	}
 	for _, tt := range tests {
 		tt := tt

metrics/thresholds.go

@@ -260,8 +260,8 @@ var ErrInvalidThreshold = errors.New("invalid threshold")
 func (ts *Thresholds) Validate(metricName string, r *Registry) error {
 	parsedMetricName, _, err := ParseMetricName(metricName)
 	if err != nil {
-		err := fmt.Errorf("unable to validate threshold expressions: %w", ErrMetricNameParsing)
-		return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
+		parseErr := fmt.Errorf("unable to validate threshold expressions; reason: %w", err)
+		return errext.WithExitCodeIfNone(parseErr, exitcodes.InvalidConfig)
 	}
 
 	// Obtain the metric the thresholds apply to from the registry.