elastic / elastic/observability-migration-platform
PromQL comments truncate the emitted native query because newlines are flattened first
- Dominant language
- Python
- Stars
- 6
- Forks
- 8
- Avg merge
- 2d 22h
- Merged PRs (30d)
- 23
Description
## Summary
`_clean_promql_for_native` collapses newlines to spaces. A PromQL `#` line comment ends at
its newline, so once the newline is gone the comment swallows the rest of the expression —
and the truncated text is what gets emitted as the native `PROMQL` query.
The panel is still marked `migrated` with no warning, so the operator has no signal that the
query lost most of its expression.
## Reproduction
On `main` (`d80e8ec`), no target or upload needed:
```python
from observability_migration.adapters.source.grafana.panels import (
can_use_native_promql, _clean_promql_for_native,
)
expr = "sum(rate(node_cpu_seconds_total[5m])) # cpu rate\n+ sum(rate(node_disk_reads_completed_total[5m]))"
can_use_native_promql(expr) # True -> routed to native emission
_clean_promql_for_native(expr) # 'sum(rate(node_cpu_seconds_total[5m])) # cpu rate + sum(rate(node_disk_reads_completed_total[5m]))'
```
The emitted query reaches Elasticsearch as everything up to `#`, i.e. only the first `sum(...)`.
The `+ sum(...)` term is silently gone.
## Impact
Two shapes, both from Grafana dashboards that legitimately comment multi-line PromQL:
1. **Silent wrong answer** (above) — a valid query that computes the wrong thing. This is the
worse case, because a wrong number in a rendering panel is harder to notice than an error.
2. **Hard failure** — if the comment falls somewhere that leaves the remainder unparseable,
Elasticsearch rejects the query and the panel shows an error in Kibana.
Grafana itself evaluates these expressions correctly, so this is a translation-side loss.
## Suggested fix
Strip `#` comments from the expression **before** flattening newlines, while their extent is
still exact. Quote state must be tracked — including backtick raw strings — because a `#`
inside a label value is data, not a comment. `_strip_promql_comments` in
`observability_migration/adapters/source/grafana/panels.py` (added in #442) already does
exactly this and could be reused; the only reason #442 did not also apply it inside
`_clean_promql_for_native` is that doing so changes emitted query text for every commented
expression, which deserves its own change and its own artifact diff rather than riding along
in a vector-matching PR.
Note that the gates reading `_clean_promql_for_native` output (`_promql_has_known_server_bug`,
`_promql_has_unsupported_comparison`) must keep seeing structure they can trust: if comments
are removed earlier, those gates get *more* accurate, but the fix should be verified against
them rather than assumed safe.
## Not caused by #442
Found while working on #440. The vector-matching change neither introduces nor worsens this:
it strips comments only when scanning the raw expression to decide routing, and leaves the
emitted-query path untouched. Filing separately as requested.
Contributor guide
Research direction
Start in observability_migration/adapters/source/grafana/panels.py at _clean_promql_for_native, and compare its behavior with _strip_promql_comments and the two PromQL gates named in the issue. Reproduce the supplied multi-line expression, then verify that comments are removed without treating # inside quoted or backtick raw strings as comments, while the gates still see valid structure and the emitted native query retains the expression.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- elasticsearch, grafana, python
- Domain
- observability-sre, tooling
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100