Skip to content

fix(grafana): refuse agg(A op B) shapes with no honest ES|QL rendering - #433

Merged
shmsr merged 1 commit into
elastic:mainfrom
giorgi-imerlishvili-elastic:fix/377-promql-count-set-operator
Sep 2, 2026
Merged

fix(grafana): refuse agg(A op B) shapes with no honest ES|QL rendering#433
shmsr merged 1 commit into
elastic:mainfrom
giorgi-imerlishvili-elastic:fix/377-promql-count-set-operator

Conversation

@giorgi-imerlishvili-elastic

@giorgi-imerlishvili-elastic giorgi-imerlishvili-elastic commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes #377.

Is the issue valid?

Yes for two of its three claims; the third is not a bug.

  • Valid — wrong number. count(A and B >= threshold) translated to COUNT(A) and shipped as migrated_with_warnings. On the reporter's k8s volumes dashboard the "Running PVCs Above % Used Warning Threshold" tile rendered 13 when the index holds only 8 PVCs and the correct answer is 2.
  • Valid — silently dropped operator. The and and the >= 80/100 threshold were both discarded with no warning naming them.
  • Not a bug — the missing WHERE @timestamp. The report inferred that the absent time predicate makes the panel scan the whole index. Against the raw _query API that reproduces (a Sept-1 window returned Aug-20 rows, since BUCKET() only sizes buckets). But Kibana does not send the query bare: the internal/search/esql_async body carries a sibling "filter": {"range": {"@timestamp": {gte, lte}}} built from the time picker, and the panel correctly shows N/A on an empty window. This is the deliberate design from 117fd4e and is unchanged here.

Root cause

_ast_aggregate_fragment rewrites three shapes of agg(A op B):

  • the linear sum(A ± B) push-down,
  • scalar hoisting (max(A * 8)),
  • the histogram mean idiom.

Every other shape fell through and returned a bare unknown fragment. The generic fragment_extract / stats_expression fallback then rebuilt agg(<first metric leaf>) from the fragment's summary fields — which discards the operator and every operand but the first, producing a well-formed query with no relation to the source semantics.

The underlying reason no generic rewrite exists: PromQL evaluates A op B per matching series pair, matching on the operands' full label set, before the aggregation reduces the survivors. ES|QL has no equivalent stage.

Code paths checked

  • observability_migration/adapters/source/grafana/promql.py_ast_aggregate_fragment, _push_outer_agg, _make_binary_fragment, _append_not_feasible_reason
  • colocated_binary_agg_plan / colocated_binary_agg_unblock — the per-document renderer that legitimately handles count(A + B)
  • fragment_guardrails_rule — turns accumulated not_feasible_reasons into feasibility = "not_feasible"
  • fragment_extract / stats_expression — the fallback that produced the wrong query
  • The QUERY_PREPROCESSORSCLASSIFIERSTRANSLATORSPOSTPROCESSORSVALIDATORS ordering, to confirm the refusal lands before the fallback and can still be cleared by the co-located renderer

The fix

A closing guard in _ast_aggregate_fragment refuses any agg(A op B) that no rewrite above could express, so it degrades to a "Migration Required" placeholder carrying the original PromQL and a reason naming both the operator and the aggregation.

The guard is an allowlist of operators it hands on, not a list of operators it refuses. The first revision was a deny-list and it omitted atan2, which reproduced the exact mistranslation the guard exists to prevent. An unenumerated operator now fails closed.

or is the single deferred operator: it carries established Grafana idioms — the same-metric range-window fallback (max_over_time(M[$interval]) or max_over_time(M[5m])) and the live-absent operand drop — that need a resolver parse time does not have.

Side effects considered

A 107-case matrix of agg(binary_expr) shapes was run against the unmodified base and this branch:

count
flipped feasiblenot_feasible 40
unchanged 67
unintended changes 0

Every existing safe rewrite still applies: sum(A ± B) push-down, scalar hoisting, the histogram mean idiom, and the co-located count(A + B) renderer. A full artifact diff of a real migration (base vs branch) shows exactly one panel changed.

Refusing or was tried and reverted — it broke test_clamp_wrapper_uses_real_output_field_when_panel_drops_unmigrated_target, which is how the boundary was located.

Tests

  • Full suite: 6097 passed, 3 skipped, 473 subtests passed.
  • make lint — clean. make typecheck — clean (10 source files).
  • Fidelity ratchet, coverage matrices, panel matrices — pass. No baseline was refreshed.
  • New: tests/test_issue377_agg_over_binary_operator.py, including a sweep asserting no operator silently drops an operand.
  • New snapshots: agg_over_and_operator_not_feasible.txt, agg_over_series_comparison_not_feasible.txt.
  • Hooks are not installed locally (.git/hooks has only samples), so all five configured checks were run manually: gitleaks v8.30.1 via Docker (no leaks, 894 commits), source headers (492 files), ruff, pytest smoke, no-local-paths. Nothing was bypassed; --no-verify was not used.

Kibana visual verification

Chrome DevTools MCP against https://127.0.0.1:5601, dashboard obs-migrate-k8s-storage-volumes-cluster, view mode, hard-reloaded to clear stale edit state.

  1. Base build uploaded first: the tile rendered 13 — the bug, live.
  2. Time-range probe: moving to a window with no data turned the tile to N/A, and the esql_async request body showed Kibana's injected @timestamp range filter. This is what refutes the issue's third claim.
  3. Branch build uploaded: the same tile now renders Migration Required, the original PromQL, and the reason string.
  4. Neighbouring panels ("Unbound PVCs" and the other 12) were unchanged, and the default state did not regress.

Pre-existing failures

None attributable to this branch. One transient local failure — test_grafana_create_alert_rules_without_api_key_fails_the_run — was caused by an exported KIBANA_API_KEY in the verifying shell; that test asserts behavior without a key. Clearing the env restored a full green run. Not a code issue.

Known gap, not fixed here

count(A or B) still drops the right operand (COUNT(node_a)). It is older and wider than this fix, needs a resolver at a later pipeline stage, and is tracked in #434. It is documented in the new test's docstring so it stays visible rather than silently sitting inside the deferred branch.

`count(A and B >= threshold)` translated to `COUNT(A)`: the set operator and
the threshold were both discarded, and the panel shipped as
`migrated_with_warnings`. On a real k8s volumes dashboard that rendered as 13
PVCs over their warning threshold when only 8 PVCs existed at all and the
correct answer was 2. A plausible wrong number is the worst outcome a
migration can produce, because nobody re-derives it by hand.

`_ast_aggregate_fragment` rewrote three shapes of `agg(A op B)` -- the linear
`sum(A +/- B)` push-down, scalar hoisting, and the histogram mean idiom -- and
let every other shape fall through. The generic
`fragment_extract`/`stats_expression` fallback then rebuilt
`agg(<first metric leaf>)` from the fragment's summary fields, silently
dropping the operator and the remaining operands.

Add a closing guard that refuses whatever no rewrite could express, so these
degrade to a "Migration Required" placeholder carrying the original PromQL and
a reason naming the operator and the aggregation, instead of a fabricated
value. The guard is an allowlist of operators it hands on rather than a list
of operators it refuses: a deny-list omitted `atan2` and reproduced the exact
mistranslation it exists to stop, so an unenumerated operator now fails
closed.

`or` is the one deferred operator. It carries established Grafana idioms --
the same-metric range-window fallback and the live-absent operand drop -- that
need a resolver parse time does not have.

Refs elastic#377
@shmsr
shmsr merged commit 442b6b7 into elastic:main Sep 2, 2026
13 checks passed
@shmsr shmsr added source:grafana Grafana source migration program:grafana-engine Grafana translator and engine correctness bug Something isn't working labels Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working program:grafana-engine Grafana translator and engine correctness source:grafana Grafana source migration

Projects

None yet

2 participants