From c33a6cfa56e8802773f1d4be1fcac7364f544799 Mon Sep 17 00:00:00 2001 From: Aum Date: Fri, 9 Oct 2026 10:41:40 +0000 Subject: [PATCH] fix(promql): keep metric name for comparison operators (#9332) * fix(promql): keep metric name for comparison operators `collect_metric_names` dropped the response `__name__` for every binary operator except `and` / `or` / `unless`, so comparisons such as `node_load1 > 5` came back without the metric name. Grafana rendered them as `{instance="..."}` and alert templates reading `$labels.__name__` got nothing. Follow Prometheus' `shouldDropMetricName` / `resultMetric`: arithmetic operators drop the name, comparisons keep the left-hand name, except when the comparison returns a bool or is a one-to-one `on(...)` match. A `group_left` / `group_right` modifier does not reduce the labels, so the name is kept there. Both the HTTP PromQL API and the gRPC gateway share `retrieve_metric_name_and_result_type`, so one fix covers both. Signed-off-by: Aumnertic * docs: document binary_keeps_metric_name Signed-off-by: Aumnertic * fix(promql): handle on(__name__), scalar lhs and group_right Signed-off-by: Aumnertic * chore(promql): correct result-side docs and drop unreachable String arm Signed-off-by: Aumnertic * fix(promql): drop metric name when explicitly ignored Handle ignoring(__name__) for one-to-one comparisons and cover comparison, group_left, group_right and unless behavior. Signed-off-by: Aumnertic --------- Signed-off-by: Aumnertic --- src/servers/src/http/prometheus.rs | 218 +++++++++++++++++++++++++++-- 1 file changed, 207 insertions(+), 11 deletions(-) diff --git a/src/servers/src/http/prometheus.rs b/src/servers/src/http/prometheus.rs index 420a0c8f4b4..da89d2a7b6c 100644 --- a/src/servers/src/http/prometheus.rs +++ b/src/servers/src/http/prometheus.rs @@ -49,11 +49,11 @@ use futures::future::join_all; use futures::{StreamExt, TryStreamExt}; use itertools::Itertools; use promql_parser::label::{METRIC_NAME, MatchOp, Matcher, Matchers}; -use promql_parser::parser::token::{self}; +use promql_parser::parser::token::TokenType; use promql_parser::parser::value::ValueType; use promql_parser::parser::{ - AggregateExpr, BinaryExpr, Call, Expr as PromqlExpr, LabelModifier, MatrixSelector, ParenExpr, - SubqueryExpr, UnaryExpr, VectorSelector, + AggregateExpr, BinModifier, BinaryExpr, Call, Expr as PromqlExpr, LabelModifier, + MatrixSelector, ParenExpr, SubqueryExpr, UnaryExpr, VectorMatchCardinality, VectorSelector, }; use query::parser::{DEFAULT_LOOKBACK_STRING, PromQuery, QueryLanguageParser, QueryStatement}; use serde::de::{self, MapAccess, Visitor}; @@ -1373,6 +1373,72 @@ fn promql_expr_to_metric_name(expr: &PromqlExpr) -> Option { } } +/// Whether the metric name survives a binary operation; see +/// [`binary_metric_name_side`] for the operand that supplies it. +/// +/// Follows Prometheus' `shouldDropMetricName` and `resultMetric`: set operators +/// and comparisons keep the name, while arithmetic operators drop it. A +/// comparison also drops it when it returns a bool or is a one-to-one +/// `on(...)` match that does not keep `__name__` or an `ignoring(...)` match +/// that excludes it. Prometheus keeps only the `on(...)` labels or deletes the +/// `ignoring(...)` labels for one-to-one matches. +fn binary_keeps_metric_name(op: &TokenType, modifier: Option<&BinModifier>) -> bool { + if op.is_set_operator() { + return true; + } + + if !op.is_comparison_operator() { + return false; + } + + let Some(m) = modifier else { + return true; + }; + + if m.return_bool { + return false; + } + + if m.card == VectorMatchCardinality::OneToOne { + match &m.matching { + Some(LabelModifier::Include(labels)) + if !labels.labels.contains(&METRIC_NAME.to_string()) => + { + return false; + } + Some(LabelModifier::Exclude(labels)) + if labels.labels.contains(&METRIC_NAME.to_string()) => + { + return false; + } + _ => {} + } + } + + true +} + +/// The operand whose labels supply the result metric name of a binary +/// operation. +/// +/// Prometheus builds the result from the side whose labels it carries, i.e. the +/// "many" side of the match: `group_right` makes the right-hand operand that +/// side, and a scalar left-hand operand (for example `0.5 < a`) leaves the +/// vector on the right. Every other case takes the left-hand operand. +fn binary_metric_name_side<'a>( + lhs: &'a PromqlExpr, + rhs: &'a PromqlExpr, + modifier: Option<&BinModifier>, +) -> &'a PromqlExpr { + let group_right = + modifier.is_some_and(|m| matches!(m.card, VectorMatchCardinality::OneToMany(_))); + if group_right || matches!(lhs.value_type(), ValueType::Scalar) { + rhs + } else { + lhs + } +} + /// Recursively collect all metric names from a PromQL expression fn collect_metric_names(expr: &PromqlExpr, metric_names: &mut HashSet) { match expr { @@ -1395,14 +1461,16 @@ fn collect_metric_names(expr: &PromqlExpr, metric_names: &mut HashSet) { collect_metric_names(expr, metric_names) } PromqlExpr::Unary(UnaryExpr { .. }) => metric_names.clear(), - PromqlExpr::Binary(BinaryExpr { lhs, op, .. }) => { - if matches!( - op.id(), - token::T_LAND // INTERSECT - | token::T_LOR // UNION - | token::T_LUNLESS // EXCEPT - ) { - collect_metric_names(lhs, metric_names) + PromqlExpr::Binary(BinaryExpr { + lhs, + op, + rhs, + modifier, + .. + }) => { + if binary_keeps_metric_name(op, modifier.as_ref()) { + let side = binary_metric_name_side(lhs, rhs, modifier.as_ref()); + collect_metric_names(side, metric_names) } else { metric_names.clear() } @@ -3211,6 +3279,134 @@ mod tests { expected_type: ValueType::Vector, should_error: false, }, + // Arithmetic operators always drop the metric name. + TestCase { + name: "arithmetic between metrics", + promql: "a + b", + expected_metric: None, + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "arithmetic with scalar", + promql: "a * 2", + expected_metric: None, + expected_type: ValueType::Vector, + should_error: false, + }, + // Comparisons keep the metric name from the side whose labels the + // result carries, i.e. the "many" side of the match: normally the + // left operand, but `group_right` takes it from the right operand and + // a scalar left operand leaves the vector on the right. The name is + // dropped when the comparison returns a bool or is a one-to-one + // `on(...)` match that does not keep `__name__` or an `ignoring(...)` + // match that excludes it. A `group_left`/`group_right` modifier does + // not reduce the labels, so the name is kept. + TestCase { + name: "bool comparison between metrics", + promql: "a > bool b", + expected_metric: None, + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "bool comparison with scalar", + promql: "a > bool 0.5", + expected_metric: None, + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "comparison with scalar", + promql: "a > 0.5", + expected_metric: Some("a"), + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "comparison with scalar on the left", + promql: "0.5 < a", + expected_metric: Some("a"), + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "equality comparison with scalar", + promql: "up == 0", + expected_metric: Some("up"), + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "comparison between metrics", + promql: "a > b", + expected_metric: Some("a"), + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "comparison with ignoring", + promql: "a > ignoring(x) b", + expected_metric: Some("a"), + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "comparison with ignoring __name__", + promql: "a > ignoring(__name__) b", + expected_metric: None, + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "comparison with ignoring __name__ and group_left", + promql: "a > ignoring(__name__) group_left b", + expected_metric: Some("a"), + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "comparison with ignoring __name__ and group_right", + promql: "a > ignoring(__name__) group_right b", + expected_metric: Some("b"), + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "unless with ignoring __name__", + promql: "a unless ignoring(__name__) b", + expected_metric: Some("a"), + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "comparison with on", + promql: "a > on(x) b", + expected_metric: None, + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "comparison with on keeping __name__", + promql: "a > on(__name__) b", + expected_metric: Some("a"), + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "comparison with on and group_left", + promql: "a > on(x) group_left b", + expected_metric: Some("a"), + expected_type: ValueType::Vector, + should_error: false, + }, + TestCase { + name: "comparison with on and group_right", + promql: "a > on(x) group_right b", + expected_metric: Some("b"), + expected_type: ValueType::Vector, + should_error: false, + }, // Subquery cases TestCase { name: "basic subquery",