Addressed comments.

Signed-off-by: bwplotka <bwplotka@gmail.com>

model/relabel/relabel.go

@@ -29,8 +29,8 @@ import (
 )
 
 var (
+	// relabelTargetLegacy allows targeting labels with legacy Prometheus character set, plus ${<var>} variables for dynamic characters from source the metrics.
 	relabelTargetLegacy = regexp.MustCompile(`^(?:(?:[a-zA-Z_]|\$(?:\{\w+\}|\w+))+\w*)+$`)
-	relabelTargetUTF8   = regexp.MustCompile(`^(?:(?:[^\${}]|\$(?:\{[^\${}]+\}|[^\${}]+))+[^\${}]*)+$`) // All UTF-8 chars, but ${}.
 
 	DefaultRelabelConfig = Config{
 		Action:      Replace,
@@ -129,11 +129,18 @@ func (c *Config) Validate() error {
 		return fmt.Errorf("%q is invalid 'target_label' for %s action", c.TargetLabel, c.Action)
 	}
 
-	relabelTargetRe := relabelTargetUTF8
+	isValidLabelNameWithRegexVarFn := func(value string) bool {
-	if model.NameValidationScheme == model.LegacyValidation {
+		// UTF-8 allows ${} characters, so standard validation allow $variables by default.
-		relabelTargetRe = relabelTargetLegacy
+		// TODO(bwplotka): Relabelling users cannot put $ and ${<...>} characters in metric names or values.
+		// Design escaping mechanism to allow that, once valid use case appears.
+		return model.LabelName(value).IsValid()
 	}
-	if c.Action == Replace && varInRegexTemplate(c.TargetLabel) && !relabelTargetRe.MatchString(c.TargetLabel) {
+	if model.NameValidationScheme == model.LegacyValidation {
+		isValidLabelNameWithRegexVarFn = func(value string) bool {
+			return relabelTargetLegacy.MatchString(value)
+		}
+	}
+	if c.Action == Replace && varInRegexTemplate(c.TargetLabel) && !isValidLabelNameWithRegexVarFn(c.TargetLabel) {
 		return fmt.Errorf("%q is invalid 'target_label' for %s action", c.TargetLabel, c.Action)
 	}
 	if (c.Action == Lowercase || c.Action == Uppercase || c.Action == KeepEqual || c.Action == DropEqual) && !model.LabelName(c.TargetLabel).IsValid() {
@@ -142,7 +149,7 @@ func (c *Config) Validate() error {
 	if (c.Action == Lowercase || c.Action == Uppercase || c.Action == KeepEqual || c.Action == DropEqual) && c.Replacement != DefaultRelabelConfig.Replacement {
 		return fmt.Errorf("'replacement' can not be set for %s action", c.Action)
 	}
-	if c.Action == LabelMap && !relabelTargetRe.MatchString(c.Replacement) {
+	if c.Action == LabelMap && !isValidLabelNameWithRegexVarFn(c.Replacement) {
 		return fmt.Errorf("%q is invalid 'replacement' for %s action", c.Replacement, c.Action)
 	}
 	if c.Action == HashMod && !model.LabelName(c.TargetLabel).IsValid() {

model/relabel/relabel_test.go

@@ -651,7 +651,7 @@ func TestRelabel(t *testing.T) {
 					Regex:        MustNewRegexp("line1.*line2"),
 					TargetLabel:  "utf-8.label${1}",
 					Separator:    ";",
-					Replacement:  `match${1}`,
+					Replacement:  "match${1}",
 					Action:       Replace,
 				},
 			},
@@ -662,6 +662,38 @@ func TestRelabel(t *testing.T) {
 				"utf-8.label": "match",
 			}),
 		},
+		{ // Replace targetLabel with UTF-8 characters and various $var styles.
+			input: labels.FromMap(map[string]string{
+				"a": "line1\nline2",
+				"b": "bar",
+				"c": "baz",
+			}),
+			relabel: []*Config{
+				{
+					SourceLabels: model.LabelNames{"a"},
+					Regex:        MustNewRegexp("(line1).*(?<second>line2)"),
+					TargetLabel:  "label1_${1}",
+					Separator:    ";",
+					Replacement:  "val_${second}",
+					Action:       Replace,
+				},
+				{
+					SourceLabels: model.LabelNames{"a"},
+					Regex:        MustNewRegexp("(line1).*(?<second>line2)"),
+					TargetLabel:  "label2_$1",
+					Separator:    ";",
+					Replacement:  "val_$second",
+					Action:       Replace,
+				},
+			},
+			output: labels.FromMap(map[string]string{
+				"a":            "line1\nline2",
+				"b":            "bar",
+				"c":            "baz",
+				"label1_line1": "val_line2",
+				"label2_line1": "val_line2",
+			}),
+		},
 	}
 
 	for _, test := range tests {
@@ -758,65 +790,31 @@ func TestRelabelValidate(t *testing.T) {
 	}
 }
 
-func TestTargetLabelValidity(t *testing.T) {
+func TestTargetLabelLegacyValidity(t *testing.T) {
-	t.Run("legacy", func(t *testing.T) {
+	for _, test := range []struct {
-		for _, test := range []struct {
+		str   string
-			str   string
+		valid bool
-			valid bool
+	}{
-		}{
+		{"-label", false},
-			{"-label", false},
+		{"label", true},
-			{"label", true},
+		{"label${1}", true},
-			{"label${1}", true},
+		{"${1}label", true},
-			{"${1}label", true},
+		{"${1}", true},
-			{"${1}", true},
+		{"${1}label", true},
-			{"${1}label", true},
+		{"${", false},
-			{"${", false},
+		{"$", false},
-			{"$", false},
+		{"${}", false},
-			{"${}", false},
+		{"foo${", false},
-			{"foo${", false},
+		{"$1", true},
-			{"$1", true},
+		{"asd$2asd", true},
-			{"asd$2asd", true},
+		{"-foo${1}bar-", false},
-			{"-foo${1}bar-", false},
+		{"bar.foo${1}bar", false},
-			{"bar.foo${1}bar", false},
+		{"_${1}_", true},
-			{"_${1}_", true},
+		{"foo${bar}foo", true},
-			{"foo${bar}foo", true},
+	} {
-		} {
+		require.Equal(t, test.valid, relabelTargetLegacy.Match([]byte(test.str)),
-			t.Run(test.str, func(t *testing.T) {
+			"Expected %q to be %v", test.str, test.valid)
-				require.Equal(t, test.valid, relabelTargetLegacy.Match([]byte(test.str)),
+	}
-					"Expected %q to be %v", test.str, test.valid)
-			})
-		}
-	})
-	t.Run("utf-8", func(t *testing.T) {
-		for _, test := range []struct {
-			str   string
-			valid bool
-		}{
-			{"-label", true},
-			{"label", true},
-			{"label${1}", true},
-			{"${1}label", true},
-			{"${1}", true},
-			{"${1}label", true},
-			{"$1", true},
-			{"asd$2asd", true},
-			{"-foo${1}bar-", true},
-			{"bar.foo${1}bar", true},
-			{"_${1}_", true},
-			{"foo${bar}foo", true},
-
-			// Those can be ambiguous. Currently, we assume user error.
-			{"${", false},
-			{"$", false},
-			{"${}", false},
-			{"foo${", false},
-		} {
-			t.Run(test.str, func(t *testing.T) {
-				require.Equal(t, test.valid, relabelTargetUTF8.Match([]byte(test.str)),
-					"Expected %q to be %v", test.str, test.valid)
-			})
-		}
-	})
 }
 
 func BenchmarkRelabel(b *testing.B) {

model/rulefmt/rulefmt_test.go

@@ -60,7 +60,7 @@ func TestParseFileFailure(t *testing.T) {
 		},
 		{
 			filename: "invalid_record_name.bad.yaml",
-			errMsg:   "potential issue in the recording rule name; should it be in expr?: strawberry{flavor=\"sweet\"}",
+			errMsg:   "braces present in the recording rule name; should it be in expr?: strawberry{flavor=\"sweet\"}",
 		},
 		{
 			filename: "bad_field.bad.yaml",