elastic / elastic/observability-migration-platform

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

Open
#443 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.