Skip to content

fix: strip PromQL comments before they are read as query structure - #444

Merged
shmsr merged 1 commit into
elastic:mainfrom
giorgi-imerlishvili-elastic:fix/443-promql-comment-truncation
Sep 22, 2026
Merged

shmsr merged 1 commit into
elastic:mainfrom
giorgi-imerlishvili-elastic:fix/443-promql-comment-truncation

Conversation

@giorgi-imerlishvili-elastic

Copy link
Copy Markdown
Collaborator

Fixes #443.

Is the issue valid?

Yes, with one correction to its impact claim. The issue describes the emitted
query being silently truncated. On Elasticsearch 9.6.0-SNAPSHOT there is no
silent wrong answer for the reported expression: the ES|QL lexer rejects the
stray # outright (no viable alternative at input ...), so the affected panel
hard-fails in Kibana with "Couldn't parse Elasticsearch ES|QL query" while the
run still scores it migrated with no warning. That is the second impact shape
the issue lists, not the first.

Root cause

A PromQL # comment runs to the end of its line. Nothing stripped comments, and
_clean_promql_for_native_with_state ends with re.sub(r"\s+", " ", expr) —
so the newline that terminates the comment was destroyed before the comment
was ever recognised, letting it swallow the rest of the expression.

Decisive check against live Elasticsearch: the same commented expression with
its newline preserved returns HTTP 200 with correct rows, while the flattened
form returns HTTP 400.

Related code paths checked

One root cause, four distinct consequences — all fixed:

  1. Truncated emission. The comment-truncated text became the emitted native
    PROMQL ... value=(...) query.
  2. Comment prose read as routing structure. Five native gates matched
    function/operator names anywhere in the raw text, so prose mentioning or /
    and / unless, histogram_quantile(, topk(, {}, $var, or
    rate(bar) [5m] needlessly degraded a native-able panel to ES|QL.
  3. Pre-routing gates in translate_panel. Before can_use_native_promql is
    even reached, or-collapsing, the empty-selector guard (PR feat(grafana): MySQL 7362 and PostgreSQL 9628/14114 curated packs #369), live metric
    discovery and the rule-pack label-override check all read the raw expression.
    With --es-url discovery active, a comment's English words were treated as
    metric names and reported missing; the report literally said
    Dropped from migrated query: instead, maybe, missing_metric_total, use.
  4. Issue Grafana: element-wise distinct-metric PromQL arithmetic via native PROMQL silently returns zero rows (panel reported clean, renders empty) #376 vector-matching guard defeated. The guard parses cleaned text,
    which a comment truncated down to the left operand alone — a shape it
    accepts — so a distinct-metric ratio slipped onto the native path.

The ES|QL path had the same omission in preprocess_grafana_macros: the
complexity classifier scans its output, so a comment merely mentioning
predict_linear reported "predict_linear has no ES|QL equivalent" against a
plain sum(rate(...)).

Also checked and deliberately left out of scope: core/telemetry_contract.py
derives phantom metric names from comment words. It is pre-existing on both base
and this branch (this change reduces it from 14 to 12), and core/ never
imports from adapters/source/grafana/.

The fix

A quote-aware _strip_promql_comments in promql.py, applied ahead of every
consumer that reads the expression as structure: both cleaners
(_clean_promql_for_native_with_state, preprocess_grafana_macros), the
routing gate, the exported classify_promql_complexity, and the point where the
expression enters the native panel path.

  • The comment's newline is preserved, so remaining operands stay separated
    once whitespace is collapsed. Dropping it would join sum(a) and + sum(b)
    into different text than Prometheus sees.
  • A # inside a string literal is data, not a comment. The scan tracks all
    three PromQL string forms — double, single, and backquoted raw — because the
    existing regex string-literal helpers do not know the backquoted form. Verified
    against promql-parser, which parses foo{path=`/a#b`} as label value /a#b.
  • An expression that is nothing but comments now declines native emission and
    reports not_feasible, instead of emitting value=(# note) for Kibana to
    reject.

Side effects considered

Recorded provenance is deliberately unchanged. query_ir.source_expression
and PanelResult.promql_expr are provenance, not structure: report.py prints
the latter to operators as "Original query", and execution/source.py sends it
to Prometheus as the source query for side-by-side parity. An earlier revision
of this fix stripped comments from them too. The shipped version keeps a separate
provenance variable that follows the same pre-existing rewrites (or-collapse,
ignored-label stripping) so it is byte-identical to main in every case except
that it retains the comment. Verified across comment-only, or-collapse,
ignored-label, and combined inputs. This matters because recording the pristine
authored text instead would have changed which expression parity comparison
executes for or-collapsed panels — unrelated to this issue.

Routing changes are intentional and only ever in the permissive direction for
comment text: panels that a comment wrongly pushed to ES|QL now go native. The
mirror cases are covered by tests — real or, and, unless, topk, {},
nested aggregation, template-variable matchers, ignored/rewritten labels, and a
genuine distinct-metric ratio are all still refused.

The strip added to _translate_multi_target_native_promql is unreachable today
(an unconditional return None disables that combiner pending Elasticsearch
PROMQL label_replace). It is there so the bug does not return with the feature,
and the code comment says so.

Tests

New tests/test_grafana_issue_443_promql_comments.py, 62 tests across scanner
semantics and boundaries (CRLF, unterminated quotes, trailing backslash, escaped
quotes, ##, all three string forms), native emission, routing-gate blindness,
the mirror "real constructs still refused" set, the #376 guard, the ES|QL path,
outer routing under live discovery, source provenance, and comment-only input.

22 of them fail on the unmodified base commit (d80e8ec), measured in an
isolated worktree.

Full gates, run with source-environment variables unset so CLI defaults could not
leak in:

  • make test — 6434 passed, 61 skipped, 531 subtests
  • make lint — ruff + source-header check clean (508 files)
  • make typecheck — mypy clean (10 source files)

No pre-existing base failures to report. Two failures seen early on were
environmental, not base breakage: a sandbox PermissionError creating .cursor
under /tmp, and an SSL error caused by my own exported ES_URL/ES_API_KEY
becoming CLI defaults.

Kibana visual verification

Done with Chrome DevTools against a local elastic-package stack (Elasticsearch
9.6.0-SNAPSHOT), in view mode with a hard reload, against an isolated seeded
stream (metrics-issue443-default, 1444 docs, 0 errors).

Before/after on the reported expression. A baseline dashboard built from the
unmodified base renders three panels red with "Couldn't parse Elasticsearch ES|QL
query" — the stray # is visible in Kibana's own error text — plus one "No
results found", and 3 console errors. The same dashboard migrated on this branch
renders all five panels as line charts with data, 0 console errors, and five
esql_async requests all returning 200. The two-operand panel reads ~4.2 against
~3.0 for the single-operand panel, visibly confirming it now sums both operands.
The panel with # inside a label value renders identically on both.

The live-discovery routing path (consequence 3 above) was verified
separately, since the first run was offline and could not exercise it. Base:
1 clean / 2 warnings, risk 6. This branch: 3 clean / 0 warnings, risk 0, all
three panels rendering with data, 0 console errors, 3× esql_async 200.

Empirical artifact evidence backing those runs: the uploaded saved object carries
12 stray-# query occurrences on base and 0 on this branch; the stored queries
executed against Elasticsearch go from 2 OK / 3 HTTP 400 to 5 OK / 0 failed. Both
verified dashboards re-migrate to byte-identical artifacts after the final
provenance revision, so the verification above still applies to the committed code.

A PromQL `#` comment ends at its newline, but both translation paths
collapsed whitespace before anything removed comments. The comment lost
its terminator and swallowed the rest of the expression, and that
truncated text became the emitted native query — Kibana reported
"Couldn't parse Elasticsearch ES|QL query" on a panel the run had scored
as migrated with no warning.

The same omission let comment prose act as query structure. Native
routing gates matched function and operator names anywhere in the text,
so prose mentioning `or`, `topk(` or `histogram_quantile(` needlessly
degraded a native-able panel, while live metric discovery treated a
comment's English words as metric names and reported them missing. In
the other direction a comment could *hide* a real construct: it
truncated the text the issue elastic#376 vector-matching guard parses, so a
distinct-metric ratio slipped onto the native path that guard exists to
refuse.

Comments are now removed quote-aware while their end-of-line extent is
still exact, ahead of every consumer that reads the expression as
structure. A `#` inside a string literal is a label value rather than a
comment, so the scan tracks all three PromQL string forms including
backquoted raw strings. Recorded provenance is deliberately unaffected:
the source expression reported to operators as "Original query", and
executed against Prometheus for side-by-side parity, still keeps the
comment the structural passes had to drop.

An expression that is nothing but comments now reports not_feasible
instead of emitting an empty selector, which degrades honestly rather
than shipping a query Kibana cannot parse.

Fixes elastic#443
@shmsr
shmsr merged commit 8612df2 into elastic:main Sep 22, 2026
13 checks passed
stefans-elastic added a commit to stefans-elastic/observability-migration-platform that referenced this pull request Sep 23, 2026
…nels.py

The function was imported from promql.py at line 94 and re-defined verbatim
at line 1556, producing Ruff F811. Inherited from elastic#444, tracked in elastic#455.
Confirmed identical by fuzzing (PR elastic#454 review).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.

PromQL comments truncate the emitted native query because newlines are flattened first

2 participants