Merge pull request #14482 from ywwg/owilliams/group-quote

promql: support quoting in grouping label lists

promql/parser/generated_parser.y

@@ -23,6 +23,8 @@ import (
         "github.com/prometheus/prometheus/model/value"
         "github.com/prometheus/prometheus/model/histogram"
         "github.com/prometheus/prometheus/promql/parser/posrange"
+
+        "github.com/prometheus/common/model"
 )
 
 %}
@@ -360,10 +362,18 @@ grouping_label_list:
 
 grouping_label  : maybe_label
                         {
-                        if !isLabel($1.Val) {
+                        if !model.LabelName($1.Val).IsValid() {
+                                yylex.(*parser).unexpected("grouping opts", "label")
+                        }
+                        $$ = $1
+                        }
+                | STRING {
+                        if !model.LabelName(yylex.(*parser).unquoteString($1.Val)).IsValid() {
                                 yylex.(*parser).unexpected("grouping opts", "label")
                         }
                         $$ = $1
+                        $$.Pos++
+                        $$.Val = yylex.(*parser).unquoteString($$.Val)
                         }
                 | error
                         { yylex.(*parser).unexpected("grouping opts", "label"); $$ = Item{} }

promql/parser/lex.go

@@ -1059,16 +1059,3 @@ func isDigit(r rune) bool {
 func isAlpha(r rune) bool {
 	return r == '_' || ('a' <= r && r <= 'z') || ('A' <= r && r <= 'Z')
 }
-
-// isLabel reports whether the string can be used as label.
-func isLabel(s string) bool {
-	if len(s) == 0 || !isAlpha(rune(s[0])) {
-		return false
-	}
-	for _, c := range s[1:] {
-		if !isAlphaNumeric(c) {
-			return false
-		}
-	}
-	return true
-}

promql/parser/parse_test.go

@@ -2397,6 +2397,51 @@ var testExpr = []struct {
 			},
 		},
 	},
+	{
+		input: `sum by ("foo")({"some.metric"})`,
+		expected: &AggregateExpr{
+			Op: SUM,
+			Expr: &VectorSelector{
+				LabelMatchers: []*labels.Matcher{
+					MustLabelMatcher(labels.MatchEqual, model.MetricNameLabel, "some.metric"),
+				},
+				PosRange: posrange.PositionRange{
+					Start: 15,
+					End:   30,
+				},
+			},
+			Grouping: []string{"foo"},
+			PosRange: posrange.PositionRange{
+				Start: 0,
+				End:   31,
+			},
+		},
+	},
+	{
+		input:  `sum by ("foo)(some_metric{})`,
+		fail:   true,
+		errMsg: "unterminated quoted string",
+	},
+	{
+		input: `sum by ("foo", bar, 'baz')({"some.metric"})`,
+		expected: &AggregateExpr{
+			Op: SUM,
+			Expr: &VectorSelector{
+				LabelMatchers: []*labels.Matcher{
+					MustLabelMatcher(labels.MatchEqual, model.MetricNameLabel, "some.metric"),
+				},
+				PosRange: posrange.PositionRange{
+					Start: 27,
+					End:   42,
+				},
+			},
+			Grouping: []string{"foo", "bar", "baz"},
+			PosRange: posrange.PositionRange{
+				Start: 0,
+				End:   43,
+			},
+		},
+	},
 	{
 		input: "avg by (foo)(some_metric)",
 		expected: &AggregateExpr{
@@ -3844,6 +3889,7 @@ func readable(s string) string {
 }
 
 func TestParseExpressions(t *testing.T) {
+	model.NameValidationScheme = model.UTF8Validation
 	for _, test := range testExpr {
 		t.Run(readable(test.input), func(t *testing.T) {
 			expr, err := ParseExpr(test.input)

promql/parser/printer.go

@@ -77,14 +77,24 @@ func (node *AggregateExpr) getAggOpStr() string {
 
 	switch {
 	case node.Without:
-		aggrString += fmt.Sprintf(" without (%s) ", strings.Join(node.Grouping, ", "))
+		aggrString += fmt.Sprintf(" without (%s) ", joinLabels(node.Grouping))
 	case len(node.Grouping) > 0:
-		aggrString += fmt.Sprintf(" by (%s) ", strings.Join(node.Grouping, ", "))
+		aggrString += fmt.Sprintf(" by (%s) ", joinLabels(node.Grouping))
 	}
 
 	return aggrString
 }
 
+func joinLabels(ss []string) string {
+	for i, s := range ss {
+		// If the label is already quoted, don't quote it again.
+		if s[0] != '"' && s[0] != '\'' && s[0] != '`' && !model.IsValidLegacyMetricName(model.LabelValue(s)) {
+			ss[i] = fmt.Sprintf("\"%s\"", s)
+		}
+	}
+	return strings.Join(ss, ", ")
+}
+
 func (node *BinaryExpr) String() string {
 	returnBool := ""
 	if node.ReturnBool {

promql/parser/printer_test.go

@@ -16,6 +16,7 @@ package parser
 import (
 	"testing"
 
+	"github.com/prometheus/common/model"
 	"github.com/stretchr/testify/require"
 
 	"github.com/prometheus/prometheus/model/labels"
@@ -44,6 +45,14 @@ func TestExprString(t *testing.T) {
 			in:  `sum without(instance) (task:errors:rate10s{job="s"})`,
 			out: `sum without (instance) (task:errors:rate10s{job="s"})`,
 		},
+		{
+			in:  `sum by("foo.bar") (task:errors:rate10s{job="s"})`,
+			out: `sum by ("foo.bar") (task:errors:rate10s{job="s"})`,
+		},
+		{
+			in:  `sum without("foo.bar") (task:errors:rate10s{job="s"})`,
+			out: `sum without ("foo.bar") (task:errors:rate10s{job="s"})`,
+		},
 		{
 			in: `topk(5, task:errors:rate10s{job="s"})`,
 		},
@@ -157,6 +166,8 @@ func TestExprString(t *testing.T) {
 		},
 	}
 
+	model.NameValidationScheme = model.UTF8Validation
+
 	for _, test := range inputs {
 		expr, err := ParseExpr(test.in)
 		require.NoError(t, err)