diff --git a/docs/sources/grafana.md b/docs/sources/grafana.md index 7a3d3d3c..e771c876 100644 --- a/docs/sources/grafana.md +++ b/docs/sources/grafana.md @@ -869,6 +869,7 @@ is available at `examples/cue/grafana-rule-pack.cue`. - `label_join(v, dst, separator, src1, src2, ...)` translates to a post-`STATS` `| EVAL dst = CONCAT(src1, "separator", src2, ...)` when all source labels appear in the inner expression's `by()` clause. If any source label is absent from the `by()` clause, the panel stays `not_feasible` (the column would not exist in the `STATS` output and `CONCAT` cannot reference it). - `label_replace(v, dst, replacement, src, regex)` follows the same rule when the replacement references the source label (a `$1`-style capture): the post-`STATS` `| EVAL`/`| GROK` can only read `src` if the inner expression's result still carries it, so forms that aggregate it away stay `not_feasible` rather than emitting a query that fails on an unknown column. This includes nested aggregations, whose grouping comes from the source's own `by()` clauses (issue #382) and whose outer `STATS` drops the inner grouping regardless — `label_replace(max(sum by (namespace) (m)), "ns", "$1", "namespace", "(.*)")` is not feasible even though `namespace` appears in an inner `by()`. Forms that read no source column are unaffected: a literal replacement emits `EVAL dst = "literal"`, and an identity copy (`dst` resolving to the same target field as `src`) emits nothing at all. - Histogram mean idioms `sum(increase|rate(m_sum) / increase|rate(m_count))` approximate as a ratio of aggregates (`sum(m_sum)/sum(m_count)`) with an explicit warning; unrelated per-element ratios stay `not_feasible`. +- An aggregation **wrapping** a binary operator (`agg(A op B)`) is only feasible when a rewrite can prove the operator survives, because PromQL evaluates `A op B` per matching series pair — matching on the operands' full label set — before the aggregation reduces the survivors, and ES|QL has no equivalent stage. The rewrites are: `sum(A ± B)` push-down (`sum` is linear), scalar hoisting (`max(A * 8)`, `avg(A / 100)`), the histogram mean idiom above, and the co-located per-document renderer (`count(A + B)` → `COUNT((A + B))`, for `+ - * /` over operands that share a label set). Everything else stays `not_feasible` with a warning naming the operator and the aggregation: set operators `and` / `unless` (`count(A and B >= threshold)` — issue #377), comparisons between two series (`count(A < B)`; compare against a scalar threshold instead), and arithmetic the renderer does not cover (`%`, `^`, `atan2`, or operands that carry their own `by()` reduction). The refusal is an allowlist of operators that are handed on rather than a list of operators that are refused, so an operator nobody enumerated fails closed instead of silently reaching the fallback. Previously these fell through to the generic single-metric fallback, which rebuilt `agg()` and shipped a plausible but wrong number as `migrated_with_warnings`. `agg(A or B)` is deliberately excluded from this refusal: it keeps its established reductions (the same-metric range-window fallback `max_over_time(M[$interval]) or max_over_time(M[5m])`, and the live-absent operand drop). - Multi-target XY panels fuse when series share a compatible ES|QL shape. Summary panels (`stat` / `singlestat` / `gauge` / `bargauge` / table) use the same compatibility group and approximate multi-series stats as a summary table when needed. Grouping mismatches where a bare aggregation (no `by()`, e.g. `min`/`avg`/`max`/`sum`) shares its metric field with a grouped sibling target — the disk-graphs "IO Wait per core" Min/Avg/Max-over-CPUs shape (issue #355) — render as two separate ES|QL layers instead of unioning the `BY` fields: one layer keeps the grouped target's own dimensions, the other re-aggregates the bare target across every group (time-bucket only), matching what Grafana always draws as a single cross-series line. When the bare target instead broadcasts an *unrelated* metric alongside a grouped breakdown (e.g. QoS `by (qos_class)` + an ungrouped fleet-wide total) there is no shared per-series computation to split out, so that target's `BY` fields still union with the grouped target's, but the warning now names the changed semantics (which series is affected, the fields it is now grouped by) instead of only describing the mechanism; an unrelated bare target sharing the panel does not disable the split for the pair that *does* share a metric. A lone bare target with no Min/Avg/Max siblings still gets its own summary layer, aliased to its own `legendFormat` text (not the raw metric field name); the same split applies within a data stream even when another fused target lands on a different index partition. Divergent label filters on otherwise identical measures CASE-inline into the shared `STATS` (including window-less `LAST_OVER_TIME`, used by Express-style status-class counters). `legendFormat` `{{label}}` placeholders on `rate`/`irate`/`increase` (and other TS paths covered by issue #99) are display hints — they become series aliases, not `BY` dimensions — so overlays like Redis in/out rates can share one panel. When fused targets carry *different* multi-placeholder `legendFormat` templates (issue #354), the composite `EVAL legend` expression is suppressed so the `STATS` column name carries series identity instead of mislabelling every series with the first target's literal text; targets whose templates agree, or that use only a single placeholder, are unaffected. Targets that remain incompatible (Windows vs Linux metrics, complex `or`/`label_replace` trees) still keep the largest compatible group and warn; Windows-specific drop wording only applies when every dropped target is a `windows_*` metric. - Grafana **stat/gauge tiles are one-per-series** (a status grid). Translation preserves the source PromQL expression's grouping and cardinality rather than inferring `BY` dimensions from `legendFormat` text alone. An ungrouped `up` (or similar) that Grafana would still fan out per scrape target is migrated **with a warning**; outer aggregations such as `sum(rate(...))` stay scalars even when the legend contains `{{job}}`. The same rule governs **nested aggregations** on every panel type: the grouping of `max(sum(m))` comes from the source's own `by()` clauses and nothing else — legend placeholders, legacy table column patterns, and dashboard-wide label inference are all display hints. In a nested aggregation the inner grouping additionally decides what the *outer* aggregation reduces over, so a hint-derived inner `BY` made `max(sum(m))` report the largest single label value instead of the collapsed total, and a hint-derived outer `BY` turned the scalar `count(count by (cpu) (m))` into a constant `1` per CPU (issue #382). - Composite/multi-target series (a fused native-PROMQL `value` column, the general ES|QL translator's scalar-expression `computed_value` column, or a curated-pack override that folds several source metrics into one `value` column with a `series_group` breakdown) never surface that internal column name to the operator. An unambiguous static `legendFormat` shared by every visible target takes priority; mixed or templated legends fall back to the panel title as the metric's label. A single such metric with a breakdown uses that label as the Y-axis title when Grafana left the axis unnamed and no uniform unit title (`%`, `Bytes`, …) can be inferred (issue #351). An explicit Grafana axis label still wins; opaque aliases such as `percentage` are ignored so a unit title can apply. A panel with no usable title/legend text falls back to the prior hidden-title behavior. diff --git a/observability_migration/adapters/source/grafana/promql.py b/observability_migration/adapters/source/grafana/promql.py index 7ddc0cf9..ec020bb3 100644 --- a/observability_migration/adapters/source/grafana/promql.py +++ b/observability_migration/adapters/source/grafana/promql.py @@ -2925,6 +2925,65 @@ def _join_rhs_not_plain_selector_reason(right_frag): ) +_COMPARISON_OPERATORS = frozenset({"==", "!=", ">", "<", ">=", "<="}) + +# Operators the closing guard below hands on instead of refusing. Only ``or`` +# qualifies: unlike ``and``/``unless`` it has established reductions the later +# stages own -- the Grafana same-metric range-window fallback +# (``max_over_time(M[$interval]) or max_over_time(M[5m])``) and the live-absent +# operand drop in ``colocated_binary_agg_plan`` -- both of which need a resolver +# that parse time does not have. +# +# This is an allowlist rather than a list of refused operators so an operator +# nobody enumerated fails closed. A deny-list silently omitted ``atan2``, which +# reintroduced exactly the mistranslation the guard exists to stop. +_AGG_OVER_BINARY_DEFERRED_OPS = frozenset({"or"}) + + +def _agg_over_binary_not_feasible_reason(outer_agg, op): + """Explain why ``agg(A B)`` has no honest ES|QL rendering. + + PromQL evaluates the inner operator per matching series pair *before* the + aggregation reduces them, and matching is on the operands' full label set. + ES|QL has no equivalent stage, so unless a dedicated rewrite applies the + only faithful answer is to refuse. + """ + agg = outer_agg or "aggregation" + op = (op or "").strip() + if not op: + return ( + f"Aggregating over an unrecognised PromQL binary expression ({agg}(A op B)) " + "cannot be expressed accurately in ES|QL; the operands must be matched on " + "their full label set before the aggregation reduces them" + ) + if op.lower() in _SET_OPERATORS: + return ( + f"PromQL set operator '{op.lower()}' inside an aggregation " + f"({agg}(A {op.lower()} B)) has no honest ES|QL translation; it selects which " + "series survive by matching the operands on their full label set before " + f"{agg}() reduces them, so the operand cannot be dropped without changing " + "the result; marked not_feasible" + ) + if op in _COMPARISON_OPERATORS: + return ( + f"PromQL comparison '{op}' between two time-series inside an aggregation " + f"({agg}(A {op} B)) filters one series by another and has no honest ES|QL " + "translation; compare against a scalar threshold instead of a second " + "series; marked not_feasible" + ) + if op in {"*", "/"}: + return ( + f"Aggregating over a per-element {op} between two time-series " + f"({agg}(A {op} B)) cannot be expressed accurately in ES|QL; " + "rewrite as a ratio of aggregates if the series are label-aligned" + ) + return ( + f"Aggregating over a per-element {op} between two time-series " + f"({agg}(A {op} B)) cannot be expressed accurately in ES|QL; the operands " + "must be matched on their full label set before the aggregation reduces them" + ) + + def _ast_aggregate_fragment(node, expr): child = _ast_from_node(node.expr, _ast_node_expr(node.expr)) frag = _copy_fragment_summary(_new_fragment(expr), child) @@ -3131,8 +3190,8 @@ def _ast_aggregate_fragment(node, expr): # Handle aggregation over a binary expression between two time-series. # SUM is linear so sum(A ± B) = sum(A) ± sum(B); push the aggregation # down to each operand and return a binary_expr the pipeline can handle. - # Division and multiplication are not linear: sum(A/B) ≠ sum(A)/sum(B), - # so those patterns are marked not_feasible rather than silently dropped. + # Every other shape either has a dedicated rewrite below or is refused by + # the closing guard, because no rewrite means the operator is dropped. if child.family == "binary_expr": inner_left = child.extra.get("left_frag") inner_right = child.extra.get("right_frag") @@ -3204,13 +3263,19 @@ def _ast_aggregate_fragment(node, expr): new_binary.group_mode = frag.group_mode new_binary.extra["approximated_agg_over_summary_ratio"] = True return new_binary - # Two true time-series operands — multiplication/division is not - # linearisable: agg(A op B) ≠ agg(A) op agg(B). + # Nothing above could rewrite ``agg(A op B)`` honestly. Refuse instead + # of returning a bare ``unknown`` fragment: the generic + # ``fragment_extract``/``stats_expression`` fallback rebuilds + # ``agg()`` from the fragment's summary fields, which + # discards the operator and every other operand and ships a plausible + # but wrong number (issue #377). ``colocated_binary_agg_unblock`` clears + # this reason again for the arithmetic the co-located renderer can + # express exactly, so the refusal only sticks where nothing else can. + op_lower = (child.binary_op or "").strip().lower() + if op_lower not in _AGG_OVER_BINARY_DEFERRED_OPS and not frag.extra.get("not_feasible_reasons"): _append_not_feasible_reason( frag, - f"Aggregating over a per-element {child.binary_op} between two time-series " - f"({frag.outer_agg}(A {child.binary_op} B)) cannot be expressed accurately in ES|QL; " - "rewrite as a ratio of aggregates if the series are label-aligned", + _agg_over_binary_not_feasible_reason(frag.outer_agg, child.binary_op), ) return frag diff --git a/tests/snapshots/promql_to_esql/agg_over_and_operator_not_feasible.txt b/tests/snapshots/promql_to_esql/agg_over_and_operator_not_feasible.txt new file mode 100644 index 00000000..1d29a4a9 --- /dev/null +++ b/tests/snapshots/promql_to_esql/agg_over_and_operator_not_feasible.txt @@ -0,0 +1,5 @@ +source: count((max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_used_bytes)) and ((max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_used_bytes)) / (max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_capacity_bytes))) >= (80 / 100)) or vector(0) +feasibility: not_feasible +warning: PromQL set operator 'and' inside an aggregation (count(A and B)) has no honest ES|QL translation; it selects which series survive by matching the operands on their full label set before count() reduces them, so the operand cannot be dropped without changing the result; marked not_feasible +--- + diff --git a/tests/snapshots/promql_to_esql/agg_over_series_comparison_not_feasible.txt b/tests/snapshots/promql_to_esql/agg_over_series_comparison_not_feasible.txt new file mode 100644 index 00000000..42b3baca --- /dev/null +++ b/tests/snapshots/promql_to_esql/agg_over_series_comparison_not_feasible.txt @@ -0,0 +1,5 @@ +source: count(node_filesystem_avail_bytes < node_filesystem_size_bytes) +feasibility: not_feasible +warning: PromQL comparison '<' between two time-series inside an aggregation (count(A < B)) filters one series by another and has no honest ES|QL translation; compare against a scalar threshold instead of a second series; marked not_feasible +--- + diff --git a/tests/test_issue377_agg_over_binary_operator.py b/tests/test_issue377_agg_over_binary_operator.py new file mode 100644 index 00000000..884af2c8 --- /dev/null +++ b/tests/test_issue377_agg_over_binary_operator.py @@ -0,0 +1,342 @@ +# Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one or more contributor license agreements. +# SPDX-License-Identifier: Elastic-2.0 + +"""Tests for issue #377 — an aggregation wrapping a PromQL binary operator. + +PromQL evaluates ``A B`` per matching series pair, matching on the +operands' full label set, *before* the enclosing aggregation reduces the +survivors. ES|QL has no equivalent stage, so ``agg(A B)`` is only +translatable when a specific rewrite proves the operator can be preserved +(``sum(A ± B)`` push-down, scalar hoisting, the co-located per-document +renderer). + +Before this fix the parser recognised those rewrites and then fell through +*silently* for every other shape: the fragment kept ``family="unknown"`` with +``outer_agg`` set, and the generic ``fragment_extract``/``stats_expression`` +fallback rebuilt ``agg()`` from the fragment's summary +fields. The operator and every other operand were discarded, and the panel +shipped as ``migrated_with_warnings`` with a plausible but wrong number. + +The reproduction is grafana.com dashboard 11454 ("K8s / Storage / Volumes / +Cluster"), panel 12 ``Running PVCs Above % Used Warning Threshold``: a +``count( and )`` singlestat that rendered a drifting count +of *all* PVCs while its sibling graph panel 41 — the same expression without the +``count(...)`` wrapper — was already correctly refused as ``not_feasible``. + +Covered here: + +1. The reproduction refuses honestly, emits no ES|QL, and reaches the operator + in its message. +2. The refusal is a property of the nesting, not of the panel type, and the + wrapped and bare forms of the same expression now agree. +3. ``and``/``unless``, two-series comparisons, and arithmetic with no renderer + all refuse rather than dropping an operand. +4. The rewrites that *can* preserve the operator still translate, byte for byte + — including ``or``, which keeps its established range-window fallback. +""" + +from __future__ import annotations + +import unittest + +from observability_migration.adapters.source.grafana import panels, rules, schema +from observability_migration.adapters.source.grafana.promql import ( + _agg_over_binary_not_feasible_reason, +) +from observability_migration.adapters.source.grafana.translate import ( + translate_promql_to_esql, +) + +INDEX = "metrics-*" + +# Issue #377 reproduction: dashboard 11454 panel 12, with the +# ``$pvc_percent_used_warning_threshold`` textbox variable already inlined as +# ``80`` by the issue #378 pre-pass. +PVC_COUNT_EXPR = ( + "count (\n" + " (max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_used_bytes ))\n" + " and\n" + " (\n" + " (max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_used_bytes ))\n" + " / (max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_capacity_bytes ))\n" + " ) >= (80 / 100)\n" + ")\n" + "or vector(0)" +) + +# Panel 41 of the same dashboard: the identical construct rendered over time, +# i.e. the inner expression without the ``count(...) or vector(0)`` wrapper. +PVC_BARE_EXPR = ( + "(max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_used_bytes ))\n" + "and\n" + "(\n" + " (max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_used_bytes ))\n" + " / (max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_capacity_bytes ))\n" + ") >= (80 / 100)" +) + + +def _translate(expr: str, panel_type: str = "stat"): + rule_pack = rules.RulePackConfig() + return translate_promql_to_esql( + expr, + datasource_index=INDEX, + esql_index=INDEX, + panel_type=panel_type, + rule_pack=rule_pack, + resolver=schema.SchemaResolver(rule_pack), + translation_hints={"summary_mode": panel_type in {"stat", "singlestat"}}, + ) + + +def _translate_panel(expr: str, panel_type: str = "stat"): + """Migrate a one-panel dashboard the way a real run does.""" + rule_pack = rules.RulePackConfig() + dashboard = { + "uid": "u-377", + "title": "issue 377", + "panels": [ + { + "id": 12, + "type": panel_type, + "title": "Running PVCs Above % Used Warning Threshold", + "gridPos": {"x": 0, "y": 0, "w": 12, "h": 8}, + "targets": [ + {"refId": "A", "expr": expr, "datasource": {"type": "prometheus"}} + ], + } + ], + } + result = panels.translate_dashboard( + dashboard, + datasource_index=INDEX, + esql_index=INDEX, + rule_pack=rule_pack, + resolver=schema.SchemaResolver(rule_pack), + ) + return result.panel_results[0], result + + +class TestIssue377Reproduction(unittest.TestCase): + """grafana.com 11454 panel 12 must refuse instead of shipping a wrong count.""" + + def test_count_over_and_comparison_is_not_feasible(self): + translated = _translate(PVC_COUNT_EXPR) + + self.assertEqual(translated.feasibility, "not_feasible") + self.assertEqual(translated.confidence, 0.0) + self.assertFalse(translated.esql_query) + + def test_refusal_names_the_operator_and_the_aggregation(self): + translated = _translate(PVC_COUNT_EXPR) + + joined = " ".join(translated.warnings) + self.assertIn("set operator 'and' inside an aggregation", joined) + self.assertIn("count(A and B)", joined) + self.assertIn("not_feasible", joined) + + def test_no_bare_count_over_the_first_metric_is_emitted(self): + """The exact symptom: COUNT(kubelet_volume_stats_used_bytes), threshold gone.""" + translated = _translate(PVC_COUNT_EXPR) + + self.assertNotIn("COUNT(", translated.esql_query or "") + self.assertNotIn("kubelet_volume_stats_used_bytes", translated.esql_query or "") + + def test_panel_status_is_not_feasible_not_migrated_with_warnings(self): + panel_result, result = _translate_panel(PVC_COUNT_EXPR) + + self.assertEqual(panel_result.status, "not_feasible") + self.assertEqual(result.not_feasible, 1) + self.assertEqual(result.migrated_with_warnings, 0) + + def test_wrapped_and_bare_forms_of_the_same_expression_agree(self): + """Panel 12 and its sibling panel 41 reached opposite verdicts before.""" + wrapped = _translate(PVC_COUNT_EXPR, panel_type="stat") + bare = _translate(PVC_BARE_EXPR, panel_type="timeseries") + + self.assertEqual(wrapped.feasibility, "not_feasible") + self.assertEqual(bare.feasibility, "not_feasible") + + def test_refusal_does_not_depend_on_panel_type(self): + for panel_type in ("stat", "singlestat", "gauge", "timeseries", "graph", "table"): + with self.subTest(panel_type=panel_type): + self.assertEqual( + _translate(PVC_COUNT_EXPR, panel_type=panel_type).feasibility, + "not_feasible", + ) + + +class TestAggOverBinaryRefusals(unittest.TestCase): + """Every ``agg(A op B)`` shape with no honest rewrite must refuse.""" + + def test_set_operators_between_distinct_series(self): + for agg in ("count", "sum", "max", "min", "avg"): + for op in ("and", "unless"): + with self.subTest(agg=agg, op=op): + translated = _translate(f"{agg}(node_up {op} node_ready)") + self.assertEqual(translated.feasibility, "not_feasible") + self.assertFalse(translated.esql_query) + + def test_set_operators_between_the_same_metric(self): + """Differing matchers are still a per-series intersection, not a filter.""" + for op in ("and", "unless"): + with self.subTest(op=op): + translated = _translate( + f'sum(node_up{{job="a"}} {op} node_up{{job="b"}})' + ) + self.assertEqual(translated.feasibility, "not_feasible") + + def test_comparison_between_two_series(self): + for op in (">", ">=", "<", "<=", "==", "!="): + with self.subTest(op=op): + translated = _translate( + f"count(node_filesystem_avail_bytes {op} node_filesystem_size_bytes)" + ) + self.assertEqual(translated.feasibility, "not_feasible") + self.assertFalse(translated.esql_query) + + def test_arithmetic_with_no_renderer(self): + # %, ^ and atan2 are outside the co-located renderer's allowlist, and + # grouped operands are outside it too (each carries its own reduction). + for expr in ( + "count(node_a % node_b)", + "max(node_a ^ 5)", + "count(node_a atan2 node_b)", + "count(max by (ns) (node_a) + max by (ns) (node_b))", + ): + with self.subTest(expr=expr): + translated = _translate(expr) + self.assertEqual(translated.feasibility, "not_feasible") + self.assertFalse(translated.esql_query) + + def test_no_binary_operator_silently_drops_an_operand(self): + """The invariant the guard exists to hold, swept over every operator. + + Each operator must either refuse, or emit a query that still reads the + right-hand operand. Shipping ``COUNT(node_a)`` for ``count(node_a op + node_b)`` is the bug. The guard is an allowlist of operators it hands on + (only ``or``) rather than a list of operators it refuses, because a + deny-list omitted ``atan2`` and let exactly that through unwarned. + + ``or`` is the one operator the guard hands on, so it is not swept here: + it still drops the right operand for a plain cross-metric union. That + gap is older and wider than this fix (it needs a resolver the parser + does not have) and is tracked separately. + """ + for op in ( + "+", "-", "*", "/", "%", "^", "atan2", + "==", "!=", ">", "<", ">=", "<=", + "and", "unless", + ): + with self.subTest(op=op): + translated = _translate(f"count(node_a {op} node_b)") + if translated.feasibility == "not_feasible": + self.assertFalse(translated.esql_query) + self.assertTrue(translated.warnings) + else: + self.assertIn("node_b", translated.esql_query) + + def test_scalar_comparison_on_the_left_is_not_silently_dropped(self): + translated = _translate("max(5 > node_load1)") + + self.assertEqual(translated.feasibility, "not_feasible") + self.assertNotIn("MAX(", translated.esql_query or "") + + +class TestAggOverBinaryStillTranslatable(unittest.TestCase): + """The rewrites that preserve the operator must be untouched by the guard.""" + + def test_scalar_comparison_filter_still_counts_series(self): + """count(m > k) has a real translation and must not be caught.""" + translated = _translate("count(kubelet_volume_stats_used_bytes > 0.8)") + + self.assertEqual(translated.feasibility, "feasible") + self.assertIn("WHERE kubelet_volume_stats_used_bytes > 0.8", translated.esql_query) + self.assertIn("COUNT(*)", translated.esql_query) + + def test_colocated_arithmetic_still_evaluates_per_document(self): + for expr in ( + "count(node_a + node_b)", + "max(node_a - node_b)", + "sum(node_a + 5)", + "max(5 - node_a)", + ): + with self.subTest(expr=expr): + translated = _translate(expr) + self.assertEqual(translated.feasibility, "feasible") + self.assertIn("computed_value", translated.esql_query) + + def test_sum_over_addition_still_pushes_down(self): + translated = _translate("sum(node_a + node_b)") + + self.assertEqual(translated.feasibility, "feasible") + self.assertIn("SUM(node_a)", translated.esql_query) + self.assertIn("SUM(node_b)", translated.esql_query) + + def test_scalar_scaling_still_hoists(self): + translated = _translate("max(node_a * 8)", panel_type="timeseries") + + self.assertEqual(translated.feasibility, "feasible") + self.assertIn("MAX(node_a)", translated.esql_query) + self.assertIn("* 8", translated.esql_query) + + def test_or_keeps_its_range_window_fallback(self): + """``or`` is deliberately outside the guard; this idiom must keep working.""" + translated = _translate( + "avg by (service_name) (" + "max_over_time(mysql_max_used_connections[$interval])" + " or max_over_time(mysql_max_used_connections[5m]))", + panel_type="timeseries", + ) + + self.assertEqual(translated.feasibility, "feasible") + self.assertIn("AVG(MAX_OVER_TIME(", translated.esql_query) + + def test_histogram_summary_ratio_still_approximates(self): + translated = _translate( + "sum(increase(rpc_duration_sum[5m]) / increase(rpc_duration_count[5m]))", + panel_type="timeseries", + ) + + self.assertEqual(translated.feasibility, "feasible") + self.assertTrue(translated.esql_query) + + +class TestAggOverBinaryReason(unittest.TestCase): + """The refusal message has to tell an operator what to do next.""" + + def test_set_operator_reason(self): + reason = _agg_over_binary_not_feasible_reason("count", "and") + + self.assertIn("set operator 'and' inside an aggregation", reason) + self.assertIn("count(A and B)", reason) + + def test_comparison_reason_suggests_a_scalar_threshold(self): + reason = _agg_over_binary_not_feasible_reason("sum", ">=") + + self.assertIn("sum(A >= B)", reason) + self.assertIn("scalar threshold", reason) + + def test_ratio_reason_is_unchanged_for_multiplication_and_division(self): + """Existing snapshots pin this wording; keep it byte-identical.""" + for op in ("*", "/"): + with self.subTest(op=op): + self.assertEqual( + _agg_over_binary_not_feasible_reason("sum", op), + f"Aggregating over a per-element {op} between two time-series " + f"(sum(A {op} B)) cannot be expressed accurately in ES|QL; " + "rewrite as a ratio of aggregates if the series are label-aligned", + ) + + def test_reason_tolerates_a_missing_outer_agg(self): + self.assertIn("aggregation(A and B)", _agg_over_binary_not_feasible_reason("", "and")) + + def test_reason_tolerates_an_unrecognised_operator(self): + reason = _agg_over_binary_not_feasible_reason("count", "") + + self.assertIn("unrecognised PromQL binary expression", reason) + self.assertIn("count(A op B)", reason) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_promql_esql_snapshots.py b/tests/test_promql_esql_snapshots.py index dd60aa98..a648419b 100644 --- a/tests/test_promql_esql_snapshots.py +++ b/tests/test_promql_esql_snapshots.py @@ -449,6 +449,26 @@ "http_requests_total and http_other_total", "timeseries", ), + # An aggregation *wrapping* a set operator has to refuse for the same reason + # the bare operator above does (issue #377). The reproduction is grafana.com + # dashboard 11454 panel 12, whose sibling graph panel already refused. + ( + "agg_over_and_operator_not_feasible", + ( + "count((max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_used_bytes))" + " and ((max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_used_bytes))" + " / (max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_capacity_bytes)))" + " >= (80 / 100)) or vector(0)" + ), + "stat", + ), + # A comparison between two *series* under an aggregation reaches the same + # closing guard: it filters one series by another, which ES|QL cannot express. + ( + "agg_over_series_comparison_not_feasible", + "count(node_filesystem_avail_bytes < node_filesystem_size_bytes)", + "timeseries", + ), # --- explicit hard blockers --------------------------------------------- ( "histogram_quantile_bare_rate_feasible",