Skip to content

fix(grafana): refuse agg(A or B) instead of dropping an operand - #436

Merged
shmsr merged 2 commits into
elastic:mainfrom
giorgi-imerlishvili-elastic:fix/434-agg-over-or-drops-operand
Sep 7, 2026
Merged

fix(grafana): refuse agg(A or B) instead of dropping an operand#436
shmsr merged 2 commits into
elastic:mainfrom
giorgi-imerlishvili-elastic:fix/434-agg-over-or-drops-operand

Conversation

@giorgi-imerlishvili-elastic

Copy link
Copy Markdown
Collaborator

Fixes #434.

Was the issue valid?

Yes — and the symptom is worse than reported. The issue predicted the panel
would be labelled migrated_with_warnings; on the base it is labelled plain
migrated with zero warnings, and renders as a normal stat panel. An
operator sees a confident green number that counts half of what was asked for.

Reproduced on the unmodified base (442b6b7), count(node_a or node_b):

FROM metrics-* | STATS node_a = COUNT(node_a) BY time_bucket = BUCKET(@timestamp, 75, ?_tstart, ?_tend) | SORT time_bucket ASC

node_b is absent.

Root cause

_ast_aggregate_fragment deliberately defers the or operator — unlike every
other binary operator, which #433 taught it to refuse — because or carries two
legitimate reductions that need a resolver parse time does not have:

  1. the Grafana same-metric range-window fallback, max_over_time(M[$interval]) or max_over_time(M[5m])
  2. the live-absent operand drop

Nothing downstream then claimed the deferred fragment. The generic
fragment_extract / stats_expression fallback rebuilt agg(<first metric leaf>) from the fragment's summary fields and shipped it clean.

The telling part: the bare chain node_a or node_b keeps both operands
(COALESCE / unified WHERE ... OR). So the aggregation wrapper was inverting
the verdict on the identical expression.

Related code paths checked

  • _ast_aggregate_fragment and its _AGG_OVER_BINARY_DEFERRED_OPS closing guard
  • _agg_over_binary_not_feasible_reason (the fix(grafana): refuse agg(A op B) shapes with no honest ES|QL rendering #433 refusal text)
  • _reduce_or_operands, _collapse_same_metric_range_fallback_groups, _flatten_or_operands
  • colocated_binary_agg_plan and colocated_binary_agg_unblock
  • the bare-or path's own disclosure warnings, for wording parity
  • fragment_guardrails (priority 1), which turns a reason into the refusal

The fix

Refuse at the stage that has the resolver, not at parse time. Parse-time
refusal was tried in #433 and broke a legitimate reduction.

  • New classifier agg_over_or_operand_drop_rule (priority 0, ahead of
    fragment_guardrails) marks the fragment not_feasible when no reduction can
    elect a single operand.
  • When a reduction does succeed but removed an operand, it discloses the
    removal — the same warnings the bare path already emits for the same drops.
    Silently succeeding was half the bug.
  • Both the classifier and colocated_binary_agg_plan now consume one shared
    agg_over_or_reduction helper, so they cannot drift into disagreeing about
    what is renderable.

It also closes a second silent drop the issue did not name: same metric with
differing label matchers, count(node_a{job="x"} or node_a{job="y"}), which
previously collapsed to COUNT(node_a) and discarded the second series set.

Side effects considered

Behaviour across the four or shapes, base vs branch:

Expression Base Branch
count(node_a or node_b) COUNT(node_a), clean not_feasible
count(node_a{job="x"} or node_a{job="y"}) COUNT(node_a), clean not_feasible
avg(max_over_time(node_a[$i]) or max_over_time(node_a[5m])) correct query identical query + disclosure
count(node_a or node_a) COUNT(node_a) COUNT(node_a)

The range-window fallback — the reduction #433 was protecting — emits
byte-identical ES|QL; only the disclosure is added. Offline (no resolver) the
rule refuses rather than guessing. Zero fidelity change on the benchmark corpus:
the refused shapes do not occur in it.

Known minor: on the degenerate count(node_a or node_a) the disclosure is
worded as an alternate-window drop when nothing was lost. The query is correct
and it errs toward over-disclosure. The obvious fix (dedupe textually identical
operands) is not safe — after macro substitution a real fallback
max_over_time(M[$interval]) or max_over_time(M[5m]) becomes two identical
operands, so deduping would suppress a legitimate disclosure.

Tests

Base Branch
Passed 6200 6229
Skipped 53 53
Subtests 473 496
  • tests/test_issue434_agg_over_or_operand_drop.py — 29 new tests (refusal,
    disclosure, by() grouping, nested chains, quantile, wrapper nesting, panel
    types, wrapped-vs-bare parity, and each surviving reduction).
  • test_issue377_agg_over_binary_operator.py — the or exclusion is removed
    from the operator sweep, as the issue asked.
  • test_migrate.py::test_clamp_wrapper_uses_real_output_field_when_panel_drops_unmigrated_target
    now seeds field caps so its or reduction is resolver-backed rather than
    accidental.

make lint and make typecheck clean (mypy: no issues in 10 source files).
Manually ran the configured pre-commit checks (gitleaks is not installed
locally, so the staged diff was scanned for credential patterns instead);
nothing was committed with --no-verify.

No pre-existing base failures. A full run on 442b6b7 is green at
6200 passed / 53 skipped.

Kibana visual verification

Chrome DevTools MCP against a local stack, both dashboards opened in view
mode
.

BEFORE — the ES|QL errors are themselves the proof. The two agg(A or B)
panels fail on node_a only (line 2:24: Unknown column [node_a]), while
the bare node_a or node_b control fails on both
(Found 2 problems ... [node_a] ... [node_b]). Same operator, and the bare form
keeps both operands while the aggregated form kept one.

AFTER — those two panels are "Migration Required" tiles carrying the
original PromQL and a reason naming both operands. All four control panels are
textually identical to BEFORE.

Network confirms it quantitatively: BEFORE issues 6 esql_async queries,
AFTER issues 4. The two refused panels emit no query at all; the other four
are untouched. All requests HTTP 200. The three console errors on AFTER are
field/data gaps (node_a/node_b are not seeded in that stack) and appear
identically on BEFORE.

`count(node_a or node_b)` emitted `STATS node_a = COUNT(node_a)` and
reported the panel as cleanly migrated with no warning, so an operator
saw a confident number that counted half of what was asked for.

`_ast_aggregate_fragment` defers the `or` operator (unlike every other
binary operator, which it refuses) because `or` carries two legitimate
reductions that need a resolver parse time does not have: the Grafana
same-metric range-window fallback, and the live-absent operand drop.
Nothing downstream then claimed the fragment, so the generic
fragment_extract/stats_expression fallback rebuilt `agg(<first metric
leaf>)` from the fragment's summary fields and shipped it clean. The
bare chain keeps both operands via COALESCE / unified `WHERE ... OR`,
so the aggregation wrapper was inverting the verdict on the identical
expression.

Refuse at the stage that has the resolver instead. A new priority-0
classifier marks the fragment not_feasible when no reduction can elect
a single operand, and discloses the removal when a reduction does
succeed but dropped one -- the same warnings the bare path emits. Both
the classifier and colocated_binary_agg_plan now share one reduction
helper so they cannot drift into disagreeing about what is renderable.

This also closes a second silent drop the issue did not name: same
metric with differing label matchers, `count(node_a{job="x"} or
node_a{job="y"})`, previously collapsed to `COUNT(node_a)` and
discarded the second series set without saying so.

Fixes elastic#434
The classifier only inspected the top fragment's inner child, so
sum(count(A or B)), label_replace(sum(A or B), ...), and avg(A or B) > 5
still rebuilt a single-operand aggregation. Walk every nested fragment
child and only disclose a range-window drop when the operands actually
are that fallback.
@shmsr
shmsr merged commit f12fee5 into elastic:main Sep 7, 2026
13 checks passed
shmsr added a commit to giorgi-imerlishvili-elastic/observability-migration-platform that referenced this pull request Sep 7, 2026
Keep the sparse-document agg(A op B) ES|QL path from this PR and the
agg(A or B) classification rewrite from elastic#436; the two bullets overlap
in docs/sources/grafana.md.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

count(A or B) silently drops the right-hand operand

2 participants