From 39e11f50b257066cdc4d72d6df9582f3c3824242 Mon Sep 17 00:00:00 2001 From: Julius Volz Date: Wed, 3 Dec 2025 14:14:35 +0100 Subject: [PATCH] Fix serialization for empty `ignoring()` in combination with `group_x()` Currently both the backend and frontend printers/formatters/serializers incorrectly transform the following expression: ``` up * ignoring() group_left(__name__) node_boot_time_seconds ``` ...into: ``` up * node_boot_time_seconds ``` ...which yields a different result (including the metric name in the result vs. no metric name). We need to keep empty `ignoring()` modifiers if there is a grouping modifier present. Signed-off-by: Julius Volz --- promql/parser/printer.go | 12 +++--- promql/parser/printer_test.go | 19 ++++++++ web/ui/mantine-ui/src/promql/format.tsx | 23 +++++----- web/ui/mantine-ui/src/promql/serialize.ts | 13 +++--- .../src/promql/serializeAndFormat.test.ts | 43 ++++++++++++++++++- 5 files changed, 85 insertions(+), 25 deletions(-) diff --git a/promql/parser/printer.go b/promql/parser/printer.go index a562b88044..c315bface7 100644 --- a/promql/parser/printer.go +++ b/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" diff --git a/promql/parser/printer_test.go b/promql/parser/printer_test.go index aadfd5688a..b28da988da 100644 --- a/promql/parser/printer_test.go +++ b/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`, }, diff --git a/web/ui/mantine-ui/src/promql/format.tsx b/web/ui/mantine-ui/src/promql/format.tsx index f4b883f678..75b1965b35 100644 --- a/web/ui/mantine-ui/src/promql/format.tsx +++ b/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) { + if (vm !== null) { + if ( + vm.labels.length > 0 || + vm.on || + vm.card === vectorMatchCardinality.manyToOne || + vm.card === vectorMatchCardinality.oneToMany + ) { matching = ( <> {" "} - on - ( - {labelNameList(vm.labels)} - ) - - ); - } else { - matching = ( - <> - {" "} - ignoring + + {vm.on ? "on" : "ignoring"} + ( {labelNameList(vm.labels)} ) diff --git a/web/ui/mantine-ui/src/promql/serialize.ts b/web/ui/mantine-ui/src/promql/serialize.ts index bbccede708..584e1ae9ff 100644 --- a/web/ui/mantine-ui/src/promql/serialize.ts +++ b/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 ( diff --git a/web/ui/mantine-ui/src/promql/serializeAndFormat.test.ts b/web/ui/mantine-ui/src/promql/serializeAndFormat.test.ts index 62b10cd781..a3734d311f 100644 --- a/web/ui/mantine-ui/src/promql/serializeAndFormat.test.ts +++ b/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__) …`, }, {