Merge pull request #17643 from prometheus/fix-empty-ignoring-serialization

Fix serialization for empty `ignoring()` in combination with `group_x()`

promql/parser/printer.go

@@ -147,12 +147,14 @@ func (node *BinaryExpr) ShortString() string {
 func (node *BinaryExpr) getMatchingStr() string {
 	matching := ""
 	vm := node.VectorMatching
-	if vm != nil && (len(vm.MatchingLabels) > 0 || vm.On) {
-		vmTag := "ignoring"
-		if vm.On {
-			vmTag = "on"
+	if vm != nil {
+		if len(vm.MatchingLabels) > 0 || vm.On || vm.Card == CardManyToOne || vm.Card == CardOneToMany {
+			vmTag := "ignoring"
+			if vm.On {
+				vmTag = "on"
+			}
+			matching = fmt.Sprintf(" %s (%s)", vmTag, strings.Join(vm.MatchingLabels, ", "))
 		}
-		matching = fmt.Sprintf(" %s (%s)", vmTag, strings.Join(vm.MatchingLabels, ", "))
 
 		if vm.Card == CardManyToOne || vm.Card == CardOneToMany {
 			vmCard := "right"

promql/parser/printer_test.go

@@ -94,6 +94,25 @@ func TestExprString(t *testing.T) {
 			in:  `a - ignoring() c`,
 			out: `a - c`,
 		},
+		{
+			// This is a bit of an odd case, but valid. If the user specifies ignoring() with
+			// no labels, it means that both label sets have to be exactly the same on both
+			// sides (except for the metric name). This is the same behavior as specifying
+			// no matching modifier at all, but if the user wants to include the metric name
+			// from either side in the output via group_x(__name__), they have to specify
+			// ignoring() explicitly to be able to do so, since the grammar does not allow
+			// grouping modifiers without either ignoring(...) or on(...). So we need to
+			// preserve the empty ignoring() clause in this case.
+			//
+			//   a - group_left(__name__) c             <--- Parse error
+			//   a - ignoring() group_left(__name__) c  <--- Valid
+			in:  `a - ignoring() group_left(__metric__) c`,
+			out: `a - ignoring () group_left (__metric__) c`,
+		},
+		{
+			in:  `a - ignoring() group_left c`,
+			out: `a - ignoring () group_left () c`,
+		},
 		{
 			in: `up > bool 0`,
 		},

web/ui/mantine-ui/src/promql/format.tsx

@@ -266,22 +266,19 @@ const formatNodeInternal = (
       let matching = <></>;
       let grouping = <></>;
       const vm = node.matching;
-      if (vm !== null && (vm.labels.length > 0 || vm.on)) {
-        if (vm.on) {
-          matching = (
-            <>
-              {" "}
-              <span className="promql-keyword">on</span>
-              <span className="promql-paren">(</span>
-              {labelNameList(vm.labels)}
-              <span className="promql-paren">)</span>
-            </>
-          );
-        } else {
+      if (vm !== null) {
+        if (
+          vm.labels.length > 0 ||
+          vm.on ||
+          vm.card === vectorMatchCardinality.manyToOne ||
+          vm.card === vectorMatchCardinality.oneToMany
+        ) {
           matching = (
             <>
               {" "}
-              <span className="promql-keyword">ignoring</span>
+              <span className="promql-keyword">
+                {vm.on ? "on" : "ignoring"}
+              </span>
               <span className="promql-paren">(</span>
               {labelNameList(vm.labels)}
               <span className="promql-paren">)</span>

web/ui/mantine-ui/src/promql/serialize.ts

@@ -136,11 +136,14 @@ const serializeNode = (
       let matching = "";
       let grouping = "";
       const vm = node.matching;
-      if (vm !== null && (vm.labels.length > 0 || vm.on)) {
-        if (vm.on) {
-          matching = ` on(${labelNameList(vm.labels)})`;
-        } else {
-          matching = ` ignoring(${labelNameList(vm.labels)})`;
+      if (vm !== null) {
+        if (
+          vm.labels.length > 0 ||
+          vm.on ||
+          vm.card === vectorMatchCardinality.manyToOne ||
+          vm.card === vectorMatchCardinality.oneToMany
+        ) {
+          matching = ` ${vm.on ? "on" : "ignoring"}(${labelNameList(vm.labels)})`;
         }
 
         if (

web/ui/mantine-ui/src/promql/serializeAndFormat.test.ts

@@ -192,8 +192,7 @@ describe("serializeNode and formatNode", () => {
           anchored: false,
           smoothed: false,
         },
-        output:
-          '{label1="value1"}',
+        output: '{label1="value1"}',
       },
 
       // Anchored and smoothed modifiers.
@@ -722,6 +721,46 @@ describe("serializeNode and formatNode", () => {
         output: "… + ignoring(label1, label2) …",
         prettyOutput: `  …
 + ignoring(label1, label2)
+  …`,
+      },
+      {
+        // Empty ignoring() without group modifiers can be stripped away.
+        node: {
+          type: nodeType.binaryExpr,
+          op: binaryOperatorType.add,
+          lhs: { type: nodeType.placeholder, children: [] },
+          rhs: { type: nodeType.placeholder, children: [] },
+          matching: {
+            card: vectorMatchCardinality.oneToOne,
+            labels: [],
+            on: false,
+            include: [],
+          },
+          bool: false,
+        },
+        output: "… + …",
+        prettyOutput: `  …
++
+  …`,
+      },
+      {
+        // Empty ignoring() with group modifiers may not be stripped away.
+        node: {
+          type: nodeType.binaryExpr,
+          op: binaryOperatorType.add,
+          lhs: { type: nodeType.placeholder, children: [] },
+          rhs: { type: nodeType.placeholder, children: [] },
+          matching: {
+            card: vectorMatchCardinality.manyToOne,
+            labels: [],
+            on: false,
+            include: ["__name__"],
+          },
+          bool: false,
+        },
+        output: "… + ignoring() group_left(__name__) …",
+        prettyOutput: `  …
++ ignoring() group_left(__name__)
   …`,
       },
       {