From c10009daaa70fd9fe16f40c22c70bd3c779f9a8b Mon Sep 17 00:00:00 2001 From: Giorgi Imerlishvili Date: Sun, 16 Aug 2026 15:21:26 +0400 Subject: [PATCH 1/3] fix(grafana): type-safe multi-select control guardrails, disclose dropped variable references Two related gaps in variable/control translation: - Multi-select guardrail type mismatch (issue #353): Kibana infers a bound ES|QL control parameter's type from the selected option *values*, not from the control's own keyword-typed option-list query. A Grafana variable whose label values happen to look numeric (CPU/core indices, ports, PIDs, status codes) can bind `?var` as an integer array, and MV_CONTAINS requires both arguments to share a type -- so comparing that integer-typed `?var` against the keyword ".*" sentinel (or a keyword field) fails ES|QL's compile-time type verification, breaking the whole query rather than degrading gracefully. - Dropped-but-referenced variables go unnoticed (issue #356): a Grafana variable used by a panel's PromQL (most sharply, an `interval` variable used as a rate/range-vector window, e.g. rate(x[$RateInterval])) but intentionally skipped by its variable rule (which assumes "handled by Kibana's time picker") silently disappears with no control and no warning, handing control of the window to the migrated query's bucket-width heuristic instead. Fixes: - `_mv_contains_filter` now wraps the bound parameter in TO_STRING(...) unconditionally (a no-op on an already-keyword value), so the multi-select guardrail type-checks regardless of how Kibana infers the parameter. Field-binding detection regexes in panels.py and parity_oracle.py were updated to recognize both the bare and wrapped shapes. - New `_disclose_dropped_referenced_variables`, run after every control-synthesis pass in translate_dashboard, appends a control_warnings entry naming any variable referenced by a panel's original PromQL but never bound to a control -- a specific message for `interval` variables, a generic one for other types. Skips variables that are genuinely unused, already bound (checking both the variable_name and classic-control _source_variable_name ownership keys), or already covered by a more specific existing warning. Also wraps the same MV_CONTAINS(?instance, ...) guardrail shape in the Redis exporter curated packs (763, 11835), and updates scripts/dashboard_qa.py's own multi-value parameter detection to recognize the wrapped form -- both gaps found via an independent model review before merge, along with the classic-control false-disclosure fix above. --- docs/sources/grafana.md | 38 +++ .../pack.yaml | 2 +- .../grafana_763_redis_exporter/pack.yaml | 10 +- .../adapters/source/grafana/panels.py | 112 +++++++- .../adapters/source/grafana/promql.py | 16 +- .../core/verification/parity_oracle.py | 7 +- scripts/dashboard_qa.py | 18 +- tests/core/test_parity_oracle.py | 10 + tests/test_curated_packs.py | 46 ++- tests/test_dashboard_qa.py | 26 ++ tests/test_grafana_issues_316_319.py | 87 +++++- ...t_k8s_views_global_interaction_scenario.py | 5 +- tests/test_migrate.py | 265 +++++++++++++++++- 13 files changed, 619 insertions(+), 23 deletions(-) diff --git a/docs/sources/grafana.md b/docs/sources/grafana.md index eec3d27e..aec7362f 100644 --- a/docs/sources/grafana.md +++ b/docs/sources/grafana.md @@ -449,6 +449,44 @@ selected. gap. Controls have no `PanelResult`-style per-item tracking of their own, so `control_warnings` is dashboard-scoped rather than per-control. +### Interval, Custom, And Other Non-Query Variables (Issue #356) + +Grafana `interval` variables (a dropdown of durations, e.g. `20s,1m,5m`) have +no Kibana control equivalent and are intentionally skipped by +`interval_variable_rule` — Kibana's time picker controls the *displayed* +range, which is the variable's most common use. `custom` variables (a static +comma-separated value list) are also skipped by default; if one is referenced +as `$var`/`?var` inside a panel query, `_ensure_param_controls` (issue #131) +synthesizes a binding control after translation, but a `custom` variable never +referenced that way has nothing to bind. + +Neither skip is safe when the variable is doing more than that. Dashboard +9852's `RateInterval` is the sharp counter-example: 16 targets use it as the +**rate window** (`rate(node_disk_written_bytes_total[$RateInterval])`), which +has nothing to do with the time picker — Grafana keeps the rate window fixed +at, e.g., `1m` regardless of the displayed range so the line stays smooth. A +duration variable used this way, or any other variable type that ends up with +no control *and* no `?var` binding, is not equivalent to "handled by the time +picker" — it silently hands control of the window to whatever the migrated +query's `TBUCKET` heuristic picks, which does not track the source value and +can differ from it in either direction. + +`translate_dashboard` therefore runs one disclosure pass after every control +has been synthesized (variable translation, `_ensure_param_controls`, +late-bound group controls, `?var` retargeting): for every templating-list +variable whose name never ended up as a control's `variable_name`, it checks +whether any panel's *original* PromQL `expr` still references `$var` / +`${var}` — if so, it appends a `control_warnings` entry naming the variable, +so the loss is printed under `CONTROL WARNINGS` and recorded in the JSON +report / migration manifest / preflight report, matching every other control +degradation on this page. `interval` variables get a specific message calling +out the rate-window bucket-heuristic substitution; every other type gets a +generic "referenced but dropped" message. A variable that is genuinely unused +by every panel is never warned about — there is nothing lost to disclose. This +is disclosure only: the variable is not migrated into a working control (that +would require parameterizing the ES|QL duration literal, which is unverified +and out of scope for this fix). + ### Variable Label Filters (`metric{label="$var"}` → `?var`) When a dashboard's templating list defines named variables used in PromQL label diff --git a/observability_migration/adapters/source/grafana/curated_packs/grafana_11835_redis_exporter_helm/pack.yaml b/observability_migration/adapters/source/grafana/curated_packs/grafana_11835_redis_exporter_helm/pack.yaml index 7ee091e3..49139f6c 100644 --- a/observability_migration/adapters/source/grafana/curated_packs/grafana_11835_redis_exporter_helm/pack.yaml +++ b/observability_migration/adapters/source/grafana/curated_packs/grafana_11835_redis_exporter_helm/pack.yaml @@ -71,7 +71,7 @@ panel: - title_match: "Memory Usage" esql_query: | TS metrics-* - | WHERE (MV_CONTAINS(?instance, ".*") OR MV_CONTAINS(?instance, {{control:instance}})) + | WHERE (MV_CONTAINS(TO_STRING(?instance), ".*") OR MV_CONTAINS(TO_STRING(?instance), {{control:instance}})) | WHERE {{metric:redis_memory_used_bytes:gauge}} IS NOT NULL OR {{metric:redis_memory_max_bytes:gauge}} IS NOT NULL | STATS used = AVG(LAST_OVER_TIME({{metric:redis_memory_used_bytes:gauge}})), max = AVG(LAST_OVER_TIME({{metric:redis_memory_max_bytes:gauge}})) BY time_bucket = TBUCKET(75, ?_tstart, ?_tend) | EVAL value = (used / max) * 100.0 diff --git a/observability_migration/adapters/source/grafana/curated_packs/grafana_763_redis_exporter/pack.yaml b/observability_migration/adapters/source/grafana/curated_packs/grafana_763_redis_exporter/pack.yaml index 36b6cf1b..721cdeb9 100644 --- a/observability_migration/adapters/source/grafana/curated_packs/grafana_763_redis_exporter/pack.yaml +++ b/observability_migration/adapters/source/grafana/curated_packs/grafana_763_redis_exporter/pack.yaml @@ -88,7 +88,7 @@ panel: - title_match: "Memory Usage" esql_query: | TS metrics-* - | WHERE (MV_CONTAINS(?instance, ".*") OR MV_CONTAINS(?instance, {{control:instance}})) + | WHERE (MV_CONTAINS(TO_STRING(?instance), ".*") OR MV_CONTAINS(TO_STRING(?instance), {{control:instance}})) | WHERE {{metric:redis_memory_used_bytes:gauge}} IS NOT NULL OR {{metric:redis_memory_max_bytes:gauge}} IS NOT NULL | STATS used = AVG(LAST_OVER_TIME({{metric:redis_memory_used_bytes:gauge}})), max = AVG(LAST_OVER_TIME({{metric:redis_memory_max_bytes:gauge}})) BY time_bucket = TBUCKET(75, ?_tstart, ?_tend) | EVAL value = (used / max) * 100.0 @@ -104,7 +104,7 @@ panel: esql_query: | TS metrics-* | WHERE @timestamp >= ?_tstart AND @timestamp <= ?_tend - | WHERE (MV_CONTAINS(?instance, ".*") OR MV_CONTAINS(?instance, {{control:instance}})) + | WHERE (MV_CONTAINS(TO_STRING(?instance), ".*") OR MV_CONTAINS(TO_STRING(?instance), {{control:instance}})) | WHERE {{metric:redis_net_input_bytes_total:counter}} IS NOT NULL OR {{metric:redis_net_output_bytes_total:counter}} IS NOT NULL | STATS input = SUM(RATE({{metric:redis_net_input_bytes_total:counter}})), output = SUM(RATE({{metric:redis_net_output_bytes_total:counter}})) BY time_bucket = TBUCKET(2 minute) | KEEP time_bucket, input, output @@ -114,7 +114,7 @@ panel: esql_query: | TS metrics-* | WHERE @timestamp >= ?_tstart AND @timestamp <= ?_tend - | WHERE (MV_CONTAINS(?instance, ".*") OR MV_CONTAINS(?instance, {{control:instance}})) + | WHERE (MV_CONTAINS(TO_STRING(?instance), ".*") OR MV_CONTAINS(TO_STRING(?instance), {{control:instance}})) | WHERE {{metric:redis_keyspace_hits_total:counter}} IS NOT NULL OR {{metric:redis_keyspace_misses_total:counter}} IS NOT NULL | STATS hits = AVG(IRATE({{metric:redis_keyspace_hits_total:counter}})), misses = AVG(IRATE({{metric:redis_keyspace_misses_total:counter}})) BY time_bucket = TBUCKET(2 minute), labels.instance | KEEP time_bucket, `labels.instance`, hits, misses @@ -124,7 +124,7 @@ panel: esql_query: | TS metrics-* | WHERE @timestamp >= ?_tstart AND @timestamp <= ?_tend - | WHERE (MV_CONTAINS(?instance, ".*") OR MV_CONTAINS(?instance, {{control:instance}})) + | WHERE (MV_CONTAINS(TO_STRING(?instance), ".*") OR MV_CONTAINS(TO_STRING(?instance), {{control:instance}})) | WHERE {{metric:redis_commands_duration_seconds_total:counter}} IS NOT NULL OR {{metric:redis_commands_total:counter}} IS NOT NULL | STATS dur = SUM(IRATE({{metric:redis_commands_duration_seconds_total:counter}})), cnt = SUM(IRATE({{metric:redis_commands_total:counter}})) BY time_bucket = TBUCKET(2 minute), labels.cmd | EVAL computed_value = (dur / cnt) @@ -135,7 +135,7 @@ panel: esql_query: | TS metrics-* | WHERE @timestamp >= ?_tstart AND @timestamp <= ?_tend - | WHERE (MV_CONTAINS(?instance, ".*") OR MV_CONTAINS(?instance, {{control:instance}})) + | WHERE (MV_CONTAINS(TO_STRING(?instance), ".*") OR MV_CONTAINS(TO_STRING(?instance), {{control:instance}})) | WHERE {{metric:redis_commands_duration_seconds_total:counter}} IS NOT NULL | STATS redis_commands_duration_seconds_total = SUM(IRATE({{metric:redis_commands_duration_seconds_total:counter}})) BY time_bucket = TBUCKET(2 minute), labels.cmd | KEEP time_bucket, `labels.cmd`, redis_commands_duration_seconds_total diff --git a/observability_migration/adapters/source/grafana/panels.py b/observability_migration/adapters/source/grafana/panels.py index fe8aa6dd..d4ca95e1 100644 --- a/observability_migration/adapters/source/grafana/panels.py +++ b/observability_migration/adapters/source/grafana/panels.py @@ -8431,8 +8431,10 @@ def _query_param_names(query): _ESQL_FIELD_CONTROL_RE = re.compile(r"\?\?(?P[A-Za-z][A-Za-z0-9_]*)") _ESQL_VALUE_PARAM_FIELD_PATTERNS = ( + # ``?var`` may be wrapped in ``TO_STRING(...)`` (issue #353's multi-select + # guardrail type-fix); match with or without that wrapper. lambda name: re.compile( - rf"MV_CONTAINS\(\s*\?{re.escape(name)}\s*,\s*(?P`[^`]+`|[A-Za-z_][A-Za-z0-9_.]*)\s*\)" + rf"MV_CONTAINS\(\s*(?:TO_STRING\(\s*)?\?{re.escape(name)}\s*\)?\s*,\s*(?P`[^`]+`|[A-Za-z_][A-Za-z0-9_.]*)\s*\)" ), lambda name: re.compile( rf"(?P`[^`]+`|[A-Za-z_][A-Za-z0-9_.]*)\s+(?:RLIKE|LIKE|==|!=|>=|<=|>|<)\s+\?{re.escape(name)}\b" @@ -8928,6 +8930,111 @@ def _repeat_variable_name(value): _VARIABLE_REFERENCE_RE = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)(?::[^}]*)?\}|\$([A-Za-z_][A-Za-z0-9_]*)") +def _variable_names_referenced_in_panels(panels) -> set[str]: + """Grafana variable names referenced by any panel target's raw PromQL. + + Scans ``$var`` / ``${var}`` / ``${var:fmt}`` tokens in each target's + ``expr`` *before* any translation-time rewrite, so it reflects what the + source dashboard actually used the variable for -- independent of + whether the migrated query still carries an equivalent reference. Used + by :func:`_disclose_dropped_referenced_variables` (issue #356) to tell a + variable that is genuinely unused from one whose loss changes query + behavior. + """ + names: set[str] = set() + for panel in panels or []: + if not isinstance(panel, dict): + continue + targets = panel.get("targets") + if not isinstance(targets, list): + continue + for target in targets: + if not isinstance(target, dict): + continue + expr = target.get("expr") + if not isinstance(expr, str) or not expr: + continue + for match in _VARIABLE_REFERENCE_RE.finditer(expr): + name = match.group(1) or match.group(2) + if name: + names.add(name) + return names + + +def _disclose_dropped_referenced_variables(variables, controls, panels, control_warnings): + """Warn when a variable referenced by panel queries never became a + control (issue #356). + + ``interval`` variables are the sharpest case: Grafana's own docs and this + codebase's ``interval_variable_rule`` frame them as "handled by Kibana's + time picker", but a variable used as a rate/range-vector window + (``rate(x[$RateInterval])``) has nothing to do with the displayed time + range. Dropping it does not just remove a dropdown -- it silently hands + control of the rate window to the migrated query's bucket-width + heuristic, which is not equivalent (AGENTS.md: degrade gracefully, do + not hide a semantic gap). + + Generalised to any Grafana variable type that ends up with no bound + control, since the same silent loss applies to any of them -- e.g. a + ``query`` variable hidden with ``hide: 2`` skips straight past + ``query_variable_rule`` with no control and no warning today. Must run + after every control-synthesis pass (``_ensure_param_controls``, + late-bound group controls, ``?var`` retargeting) so a variable that one + of those passes did bind is correctly excluded. Also skips a variable + that some earlier pass already named in a ``control_warnings`` entry + (for example ``query_variable_rule``'s "could not resolve source field" + or ``textbox_variable_rule``'s "no direct Kibana control equivalent") -- + that is already disclosed, just in more specific language, and a second + generic entry would only add noise. + """ + if control_warnings is None: + return + # A control's owning variable name is ``variable_name`` for ES|QL + # parameter-binding controls, but a classic (non-ESQL) options/range + # control -- built directly from ``context.control`` in + # ``query_variable_rule`` and friends -- never sets that key; it only + # gets ``_CONTROL_SOURCE_VARIABLE_NAME`` attached afterwards in + # ``translate_variables``. Checking only ``variable_name`` here would + # falsely flag every variable that resolved to a working classic control + # as "dropped". Mirrors ``_covered_control_variable_refs``'s lookup. + bound_names = { + name + for control in controls or [] + if isinstance(control, dict) + for name in ( + control.get("variable_name"), + control.get(_CONTROL_SOURCE_VARIABLE_NAME), + ) + if name + } + referenced = _variable_names_referenced_in_panels(panels) + for variable in variables or []: + if not isinstance(variable, dict): + continue + name = variable.get("name") + if not name or name in bound_names or name not in referenced: + continue + if any(f"'{name}'" in warning for warning in control_warnings): + continue + var_type = variable.get("type") or "unknown" + if var_type == "interval": + control_warnings.append( + f"variable '{name}' (type 'interval') is used by panel queries as a " + f"rate/range window (e.g. '[${name}]') but was dropped during migration " + "-- no Kibana control was emitted, and Kibana's time picker only " + "controls the displayed range, not this window. The migrated query's " + "bucket-width heuristic now implicitly determines the window instead, " + "which can be narrower or wider than the Grafana original" + ) + else: + control_warnings.append( + f"variable '{name}' (type '{var_type}') is referenced by panel queries " + "but was dropped during migration -- no Kibana control or query " + "parameter was emitted for it, so it no longer has any effect on " + "query behavior" + ) + + def _resolve_variable_values(variable: dict) -> tuple[list[str], str]: """Return ``(values, source)`` for a Grafana templating variable. @@ -10495,6 +10602,9 @@ def translate_dashboard(dashboard, datasource_index="metrics-*", esql_index=None control_warnings=result.control_warnings, ) controls = _retarget_esql_param_controls_to_panel_bindings(controls, flat_panels) + _disclose_dropped_referenced_variables( + variables, controls, all_panels, result.control_warnings + ) rewritten_panel_results = _rewrite_variable_warnings( result.panel_results, _covered_control_variable_refs(controls), diff --git a/observability_migration/adapters/source/grafana/promql.py b/observability_migration/adapters/source/grafana/promql.py index e472a9ba..ff460907 100644 --- a/observability_migration/adapters/source/grafana/promql.py +++ b/observability_migration/adapters/source/grafana/promql.py @@ -1747,7 +1747,19 @@ def _mv_contains_filter(label, param_name, negate=False, allow_empty_match_all=F preference: ES|QL ``RLIKE`` requires a literal pattern and rejects a computed one, so ``RLIKE MV_CONCAT(?var, "|")`` -- which would have rebuilt Grafana's own ``(a|b)`` alternation -- is not expressible. + + ``?param`` is wrapped in ``TO_STRING(...)`` (issue #353): Kibana infers a + bound ES|QL parameter's type from the selected option *values*, not from + the control's keyword-typed option-list query. A variable whose values + happen to look numeric (CPU/core indices, ports, PIDs, status codes) binds + ``?param`` as an integer array, and ``MV_CONTAINS`` requires both + arguments to share a type, so the ``".*"`` sentinel (keyword) and the + keyword label field both fail to type-check against it -- a compile-time + verification error, not a runtime one, so it fails the whole query. + ``TO_STRING`` on an already-keyword parameter is a no-op, so this is safe + regardless of how Kibana ends up inferring the type. """ + param = f"TO_STRING(?{param_name})" clauses = [] if allow_empty_match_all: # Kibana leaves an unselected multi-values control bound as an empty @@ -1755,8 +1767,8 @@ def _mv_contains_filter(label, param_name, negate=False, allow_empty_match_all=F # like the source default All selection ([".*"]), not like "match # nothing" which blanks the dashboard on first load. clauses.append(f"MV_COUNT(?{param_name}) == 0") - clauses.append(f'MV_CONTAINS(?{param_name}, ".*")') - clauses.append(f"MV_CONTAINS(?{param_name}, {label})") + clauses.append(f'MV_CONTAINS({param}, ".*")') + clauses.append(f"MV_CONTAINS({param}, {label})") expr = "(" + " OR ".join(clauses) + ")" return f"NOT {expr}" if negate else expr diff --git a/observability_migration/core/verification/parity_oracle.py b/observability_migration/core/verification/parity_oracle.py index 2ddb048d..3809bfc5 100644 --- a/observability_migration/core/verification/parity_oracle.py +++ b/observability_migration/core/verification/parity_oracle.py @@ -1180,7 +1180,12 @@ def _exact_control_param_names(esql: str) -> set[str]: -_MV_CONTAINS_PARAM_RE = re.compile(r"MV_CONTAINS\s*\(\s*\?([A-Za-z_][A-Za-z0-9_]*)", re.IGNORECASE) +_MV_CONTAINS_PARAM_RE = re.compile( + # ``?var`` may be wrapped in ``TO_STRING(...)`` (issue #353's multi-select + # guardrail type-fix); match with or without that wrapper. + r"MV_CONTAINS\s*\(\s*(?:TO_STRING\s*\(\s*)?\?([A-Za-z_][A-Za-z0-9_]*)", + re.IGNORECASE, +) def _mv_contains_param_names(esql: str) -> set[str]: diff --git a/scripts/dashboard_qa.py b/scripts/dashboard_qa.py index 294ddcc3..0f756f9a 100644 --- a/scripts/dashboard_qa.py +++ b/scripts/dashboard_qa.py @@ -51,6 +51,18 @@ _PARAM = re.compile(r"(? bool: + """Whether ``?name`` is bound through an ``MV_CONTAINS`` multi-select + guardrail, so it must be sent as a list rather than a scalar. + + Matches both ``MV_CONTAINS(?name`` and the type-safe + ``MV_CONTAINS(TO_STRING(?name)`` form (issue #353) -- a multi-select + control's parameter is still an array either way, ``TO_STRING`` just + wraps it for ES|QL's benefit. + """ + return bool(re.search(rf"MV_CONTAINS\(\s*(?:TO_STRING\(\s*)?\?{re.escape(name)}\b", query)) + + # --------------------------------------------------------------------------- # # payload helpers # --------------------------------------------------------------------------- # @@ -235,7 +247,7 @@ def run_query(es_url: str, query: str, tstart: str, tend: str, api_key: str): for name in sorted(set(_PARAM.findall(query))): if name in ("_tstart", "_tend"): continue - params.append({name: [".*"] if f"MV_CONTAINS(?{name}" in query else ".*"}) + params.append({name: [".*"] if _is_multi_value_param(name, query) else ".*"}) try: doc = _es(es_url, {"query": query, "params": params}, api_key) except urllib.error.HTTPError as exc: @@ -412,7 +424,7 @@ def _our_last_bucket_series(es_url, query, tstart, tend, api_key): for name in sorted(set(_PARAM.findall(query))): if name in ("_tstart", "_tend"): continue - params.append({name: [".*"] if f"MV_CONTAINS(?{name}" in query else ".*"}) + params.append({name: [".*"] if _is_multi_value_param(name, query) else ".*"}) doc = _es(es_url, {"query": query, "params": params}, api_key) columns = [c["name"] for c in doc.get("columns", [])] rows = doc.get("values") or [] @@ -607,7 +619,7 @@ def run_query_values(es_url: str, query: str, tstart: str, tend: str, api_key: s for name in sorted(set(_PARAM.findall(query))): if name in ("_tstart", "_tend"): continue - params.append({name: [".*"] if f"MV_CONTAINS(?{name}" in query else ".*"}) + params.append({name: [".*"] if _is_multi_value_param(name, query) else ".*"}) doc = _es(es_url, {"query": query, "params": params}, api_key) columns = [c["name"] for c in doc.get("columns", [])] # `_gauge_min`/`_gauge_max` are panel display config, not data: including diff --git a/tests/core/test_parity_oracle.py b/tests/core/test_parity_oracle.py index f5199852..906d9eb2 100644 --- a/tests/core/test_parity_oracle.py +++ b/tests/core/test_parity_oracle.py @@ -1332,6 +1332,16 @@ def test_mv_contains_params_are_recognised_as_bindable(): assert po._mv_contains_param_names(esql) == {"instance"} +def test_mv_contains_params_are_recognised_when_wrapped_in_to_string(): + """issue #353: ?param is wrapped in TO_STRING(...) so MV_CONTAINS + type-checks regardless of how Kibana infers the bound parameter type.""" + esql = ( + '| WHERE MV_CONTAINS(TO_STRING(?instance), ".*") ' + "OR MV_CONTAINS(TO_STRING(?instance), labels.instance)" + ) + assert po._mv_contains_param_names(esql) == {"instance"} + + def test_source_metric_names_are_qualified_to_es_field_paths(): """Native PROMQL addresses real field paths, not bare Prometheus names. diff --git a/tests/test_curated_packs.py b/tests/test_curated_packs.py index a5bb31e5..800ed7cb 100644 --- a/tests/test_curated_packs.py +++ b/tests/test_curated_packs.py @@ -448,8 +448,11 @@ def test_redis_memory_ratio_uses_ts_source(): assert "LAST_OVER_TIME(redis_memory_used_bytes)" in query, query assert "LAST_OVER_TIME(redis_memory_max_bytes)" in query, query assert "STATS value = LAST(value, time_bucket)" in query, query - assert "MV_CONTAINS(?instance" in query, f"should preserve multi-select binding: {query}" - assert 'MV_CONTAINS(?instance, ".*")' in query, query + # ``?instance`` is wrapped in ``TO_STRING(...)`` (issue #353) so the + # guardrail still type-checks if Kibana ever infers ``?instance`` as a + # non-keyword array. + assert "MV_CONTAINS(TO_STRING(?instance)" in query, f"should preserve multi-select binding: {query}" + assert 'MV_CONTAINS(TO_STRING(?instance), ".*")' in query, query assert result.status == "migrated", f"status_override should set migrated, got: {result.status}" # Dial domain 0-100 must survive sync: emitted query carries ``_gauge_*`` # and ``panel_result.esql_query`` must match so validate does not strip them. @@ -1282,6 +1285,45 @@ def test_esql_param_control_keeps_original_when_panel_bindings_disagree(): assert rewritten[0]["_resolved_field_name"] == "labels.instance" +def test_esql_param_control_retargets_when_panel_binds_via_to_string_wrapped_mv_contains(): + """issue #353: the field-binding scanner must still recognize + ``MV_CONTAINS(TO_STRING(?var), field)`` (the type-safe multi-select + guardrail shape), not just the bare ``MV_CONTAINS(?var, field)`` form.""" + controls = [ + { + "type": "esql", + "label": "instance", + "variable_name": "instance", + "variable_type": "multi_values", + "query": ( + "FROM metrics-* | WHERE redis_up IS NOT NULL AND `labels.instance` IS NOT NULL " + '| STATS count = COUNT(*) BY `labels.instance` | EVAL options = MV_APPEND(".*", `labels.instance`) ' + '| MV_EXPAND options | STATS count = COUNT(*) BY options | KEEP options ' + '| RENAME options AS `labels.instance` | SORT `labels.instance` ASC | LIMIT 1000' + ), + "_resolved_field_name": "labels.instance", + } + ] + panels = [ + { + "esql": { + "query": ( + "TS metrics-* | WHERE (MV_COUNT(?instance) == 0 OR " + 'MV_CONTAINS(TO_STRING(?instance), ".*") OR ' + "MV_CONTAINS(TO_STRING(?instance), instance)) " + "| WHERE redis_up IS NOT NULL | STATS value = COUNT(*)" + ) + } + } + ] + + rewritten = _retarget_esql_param_controls_to_panel_bindings(controls, panels) + query = rewritten[0]["query"] + assert "`labels.instance`" not in query + assert "BY instance" in query + assert rewritten[0]["_resolved_field_name"] == "instance" + + def test_11835_memory_usage_panel_uses_curated_override(): """The 11835 pack's Memory Usage singlestat uses the curated ES|QL, status=migrated.""" dashboard = {"gnetId": 11835, "title": "Redis...", "tags": []} diff --git a/tests/test_dashboard_qa.py b/tests/test_dashboard_qa.py index 6e21c481..82394fbd 100644 --- a/tests/test_dashboard_qa.py +++ b/tests/test_dashboard_qa.py @@ -153,6 +153,32 @@ def test_ui_flags_a_metric_panel_with_no_metric(): assert any("no metric configured" in i for i in issues), issues +# --------------------------------------------------------------------------- # +# multi-value parameter detection (issue #353) +# --------------------------------------------------------------------------- # +def test_is_multi_value_param_recognises_bare_mv_contains(): + query = 'FROM metrics-* | WHERE MV_CONTAINS(?instance, ".*") OR MV_CONTAINS(?instance, instance)' + assert qa._is_multi_value_param("instance", query) is True + + +def test_is_multi_value_param_recognises_to_string_wrapped_mv_contains(): + """Real translator output now wraps the parameter in ``TO_STRING(...)`` + (issue #353's type-safety guardrail). Missing this shape here would + silently bind a multi-select parameter as a scalar string instead of a + list, breaking the QA harness's own query execution for every + multi-select-controlled panel.""" + query = ( + 'FROM metrics-* | WHERE MV_CONTAINS(TO_STRING(?instance), ".*") ' + "OR MV_CONTAINS(TO_STRING(?instance), instance)" + ) + assert qa._is_multi_value_param("instance", query) is True + + +def test_is_multi_value_param_false_for_a_scalar_binding(): + query = "FROM metrics-* | WHERE instance == ?instance" + assert qa._is_multi_value_param("instance", query) is False + + # --------------------------------------------------------------------------- # # payload traversal # --------------------------------------------------------------------------- # diff --git a/tests/test_grafana_issues_316_319.py b/tests/test_grafana_issues_316_319.py index fad2e74b..8698a49d 100644 --- a/tests/test_grafana_issues_316_319.py +++ b/tests/test_grafana_issues_316_319.py @@ -8,6 +8,7 @@ import unittest from observability_migration.adapters.source.grafana import panels, rules, schema +from observability_migration.adapters.source.grafana.promql import _mv_contains_filter from observability_migration.adapters.source.grafana.runtime_features import ( KIBANA_PROMQL_CONTROL_PARAMS, PROMQL_LABEL_MATCHER_PARAMS, @@ -402,6 +403,28 @@ def test_emitted_range_panel_query_carries_timing_args(self): ) +class TestMvContainsFilterTypeSafety(unittest.TestCase): + """Direct unit tests for ``_mv_contains_filter`` (issue #353).""" + + def test_wraps_param_in_to_string(self): + expr = _mv_contains_filter("labels.cpu", "cpu") + self.assertEqual( + expr, + '(MV_CONTAINS(TO_STRING(?cpu), ".*") OR MV_CONTAINS(TO_STRING(?cpu), labels.cpu))', + ) + + def test_negated_still_wraps_param(self): + expr = _mv_contains_filter("labels.cpu", "cpu", negate=True) + self.assertTrue(expr.startswith("NOT (")) + self.assertIn("MV_CONTAINS(TO_STRING(?cpu)", expr) + + def test_allow_empty_match_all_still_wraps_param(self): + expr = _mv_contains_filter("labels.cpu", "cpu", allow_empty_match_all=True) + self.assertIn("MV_COUNT(?cpu) == 0", expr) + self.assertIn('MV_CONTAINS(TO_STRING(?cpu), ".*")', expr) + self.assertIn("MV_CONTAINS(TO_STRING(?cpu), labels.cpu)", expr) + + class TestMultiSelectControlsUseMvContains(unittest.TestCase): """A Grafana multi-select variable must stay multi-select in Kibana. @@ -411,7 +434,12 @@ class TestMultiSelectControlsUseMvContains(unittest.TestCase): The ``.*`` sentinel preserves Grafana's All option, because the control query already offers ``.*`` via MV_APPEND: - WHERE MV_CONTAINS(?v, ".*") OR MV_CONTAINS(?v, field) + WHERE MV_CONTAINS(TO_STRING(?v), ".*") OR MV_CONTAINS(TO_STRING(?v), field) + + ``?v`` is wrapped in ``TO_STRING(...)`` (issue #353) so the comparison + still type-checks when Kibana infers the bound control values as an + integer array instead of a keyword array (e.g. numeric label values like + CPU/core indices). [".*"] -> every series (All) ["a"] -> just a @@ -464,8 +492,11 @@ def test_multi_select_emits_mv_contains_with_all_sentinel(self): doc = self._translate(multi=True) query = doc["panels"][0]["esql"]["query"] self.assertIn("MV_COUNT(?instance) == 0", query) - self.assertIn("MV_CONTAINS(?instance", query) - self.assertIn('MV_CONTAINS(?instance, ".*")', query) + # issue #353: ?param is wrapped in TO_STRING(...) so MV_CONTAINS + # type-checks regardless of whether Kibana infers the bound control + # parameter as an integer or a keyword array. + self.assertIn("MV_CONTAINS(TO_STRING(?instance)", query) + self.assertIn('MV_CONTAINS(TO_STRING(?instance), ".*")', query) self.assertNotIn("RLIKE ?instance", query) def test_multi_select_control_is_not_single_select(self): @@ -481,3 +512,53 @@ def test_single_select_keeps_rlike_binding(self): self.assertNotIn("MV_CONTAINS", query) control = next(c for c in doc["controls"] if c.get("variable_name") == "instance") self.assertFalse(control.get("multiple")) + + def test_numeric_looking_variable_values_still_use_to_string_guard(self): + """Issue #353: a variable whose label values are numeric (e.g. CPU/core + indices from ``label_values(node_cpu_seconds_total, cpu)``) must still + emit a guard that type-checks once Kibana binds ``?var`` as an integer + array. The fix is unconditional (always wraps in ``TO_STRING``), so + this locks in the emitted shape for the exact numeric-values repro + from the issue rather than relying only on the generic multi-select + case above. + """ + dashboard = { + "title": "numeric multi-select repro", + "uid": "ms-numeric-repro", + "panels": [{ + "id": 1, "type": "timeseries", "title": "IO Wait per core", + "gridPos": {"h": 8, "w": 12, "x": 0, "y": 0}, + "datasource": {"type": "prometheus", "uid": "p"}, + "targets": [{"expr": 'sum(up{cpu=~"$cpu"})', "refId": "A"}], + }], + "templating": {"list": [{ + "name": "cpu", "type": "query", "multi": True, + "includeAll": True, + "definition": "label_values(node_cpu_seconds_total, cpu)", + "current": {"text": "0,1", "value": ["0", "1"]}, + "options": [ + {"text": "0", "value": "0"}, + {"text": "1", "value": "1"}, + ], + }]}, + } + from observability_migration.adapters.source.grafana.runtime_features import ( + ESQL_NAMED_PARAM_BINDING, + set_runtime_feature, + ) + + rule_pack = rules.RulePackConfig() + set_runtime_feature( + rule_pack, ESQL_NAMED_PARAM_BINDING, + supported=True, source="test", confidence="assumed", + ) + resolver = schema.SchemaResolver(rule_pack) + result = panels.translate_dashboard( + dashboard, datasource_index="metrics-*", + esql_index="metrics-*", rule_pack=rule_pack, resolver=resolver, + ) + doc = result.dashboard_ir.to_yaml_dict() + query = doc["panels"][0]["esql"]["query"] + self.assertIn('MV_CONTAINS(TO_STRING(?cpu), ".*")', query) + self.assertIn("MV_CONTAINS(TO_STRING(?cpu), ", query) + self.assertNotIn("MV_CONTAINS(?cpu", query) diff --git a/tests/test_k8s_views_global_interaction_scenario.py b/tests/test_k8s_views_global_interaction_scenario.py index dbc1e73e..c5c1652a 100644 --- a/tests/test_k8s_views_global_interaction_scenario.py +++ b/tests/test_k8s_views_global_interaction_scenario.py @@ -481,7 +481,10 @@ def test_k8s_cluster_and_job_query_bindings(k8s_artifacts: Path) -> None: # ``job`` is multi=True in the source dashboard, so it binds through # MV_CONTAINS and the control stays multi-select. A scalar RLIKE position # could only ever hold one value and would force single-select. - assert "MV_CONTAINS(?job" in job_query + # ``TO_STRING(...)`` wraps the parameter so MV_CONTAINS type-checks + # regardless of whether Kibana infers ``?job`` as numeric or keyword + # (issue #353). + assert "MV_CONTAINS(TO_STRING(?job)" in job_query assert "RLIKE ?job" not in job_query assert "cluster" in cluster_query assert "job" in job_query diff --git a/tests/test_migrate.py b/tests/test_migrate.py index afedb3fd..3b1a6ef3 100644 --- a/tests/test_migrate.py +++ b/tests/test_migrate.py @@ -8420,8 +8420,11 @@ def test_dashboard_equality_matcher_on_include_all_var_renders_regex(self): # the field against the literal string ".*". compact = " ".join(rendered.split()) self.assertIn("MV_COUNT(?host) == 0", compact) - self.assertIn('MV_CONTAINS(?host, ".*")', compact) - self.assertIn("MV_CONTAINS(?host, host)", compact) + # ``TO_STRING(...)`` wraps the parameter so MV_CONTAINS type-checks + # regardless of whether Kibana infers ``?host`` as numeric or + # keyword (issue #353). + self.assertIn('MV_CONTAINS(TO_STRING(?host), ".*")', compact) + self.assertIn("MV_CONTAINS(TO_STRING(?host), host)", compact) self.assertNotIn("== ?host", compact) controls = doc["dashboards"][0].get("controls", []) binding = next(c for c in controls if c.get("variable_name") == "host") @@ -8609,7 +8612,8 @@ def test_dashboard_native_equality_matcher_on_include_all_var_uses_regex(self): self.assertNotIn("PROMQL", rendered) # multi=True -> MV_CONTAINS binding (see the equality-all test above); # the ".*" disjunct keeps the first-load select-everything behaviour. - self.assertIn('MV_CONTAINS(?host, ".*")', rendered) + # ``TO_STRING(...)`` wraps the parameter (issue #353). + self.assertIn('MV_CONTAINS(TO_STRING(?host), ".*")', rendered) self.assertNotIn("host=~?host", rendered) def test_dashboard_native_equality_matcher_falls_to_esql_without_label_matcher_params(self): @@ -8663,7 +8667,8 @@ def test_dashboard_native_equality_matcher_falls_to_esql_without_label_matcher_p self.assertNotIn("PROMQL", rendered) # multi=True -> MV_CONTAINS binding (see the equality-all test above); # the ".*" disjunct keeps the first-load select-everything behaviour. - self.assertIn('MV_CONTAINS(?host, ".*")', rendered) + # ``TO_STRING(...)`` wraps the parameter (issue #353). + self.assertIn('MV_CONTAINS(TO_STRING(?host), ".*")', rendered) self.assertNotIn("== ?host", rendered) def test_dashboard_equality_matcher_on_concrete_var_keeps_exact_match(self): @@ -14585,6 +14590,258 @@ def test_custom_variable_is_skipped(self): self.assertEqual(len(controls), 0) +class DroppedReferencedVariableDisclosureTests(unittest.TestCase): + """Regression tests for issue #356: a template variable that is used by + a panel's source PromQL but ends up with no Kibana control (or ES|QL + parameter binding) must surface a control warning naming it, instead of + disappearing without a trace. ``interval`` variables used as a rate/range + window (dashboard 9852's ``RateInterval``) are the sharpest case, since + losing them silently hands control of the rate window to the migrated + query's bucket-width heuristic.""" + + def setUp(self): + self.rule_pack = migrate.RulePackConfig() + + def test_interval_variable_used_as_rate_window_is_disclosed_when_dropped(self): + dashboard = { + "title": "Disk Graphs", + "uid": "disk-graphs", + "templating": { + "list": [ + {"name": "RateInterval", "type": "interval", "query": "20s,1m,5m"}, + ] + }, + "panels": [ + { + "id": 1, + "title": "Disk Written Bytes", + "type": "graph", + "targets": [ + { + "refId": "A", + "expr": "rate(node_disk_written_bytes_total[$RateInterval])", + } + ], + } + ], + } + result = migrate.translate_dashboard( + dashboard, + datasource_index="metrics-*", + esql_index="metrics-*", + rule_pack=self.rule_pack, + ) + warnings_text = " ".join(result.control_warnings) + self.assertIn("RateInterval", warnings_text) + self.assertIn("rate", warnings_text.lower()) + self.assertIn("bucket", warnings_text.lower()) + + def test_interval_variable_never_referenced_by_a_panel_is_not_disclosed(self): + """A declared-but-unused interval variable has nothing to lose -- + warning about it would be noise, not disclosure.""" + dashboard = { + "title": "Unused Interval", + "uid": "unused-interval", + "templating": { + "list": [ + {"name": "RateInterval", "type": "interval", "query": "20s,1m,5m"}, + ] + }, + "panels": [ + { + "id": 1, + "title": "CPU", + "type": "graph", + "targets": [{"refId": "A", "expr": "sum(cpu)"}], + } + ], + } + result = migrate.translate_dashboard( + dashboard, + datasource_index="metrics-*", + esql_index="metrics-*", + rule_pack=self.rule_pack, + ) + self.assertNotIn( + "RateInterval", + " ".join(result.control_warnings), + "no warning should be raised for a variable no panel actually uses", + ) + + def test_custom_variable_used_as_duration_is_disclosed_when_dropped(self): + """Generalises beyond ``interval``: any variable type that is + referenced by a panel but never becomes a control or a bound ``?var`` + parameter is disclosed, e.g. a ``custom`` variable used as a + range-vector duration (not a scalar slot, so issue #157's dropdown + substitution does not apply, and it is not a label matcher either).""" + dashboard = { + "title": "Custom Window", + "uid": "custom-window", + "templating": { + "list": [ + {"name": "Resolution", "type": "custom", "query": "1m,5m,15m"}, + ] + }, + "panels": [ + { + "id": 1, + "title": "Disk", + "type": "graph", + "targets": [{"refId": "A", "expr": "rate(disk_bytes[$Resolution])"}], + } + ], + } + result = migrate.translate_dashboard( + dashboard, + datasource_index="metrics-*", + esql_index="metrics-*", + rule_pack=self.rule_pack, + ) + warnings_text = " ".join(result.control_warnings) + self.assertIn("Resolution", warnings_text) + self.assertIn("custom", warnings_text.lower()) + + def test_custom_variable_bound_as_label_filter_param_is_not_disclosed(self): + """A ``custom`` variable that DOES end up bound (here, as an ES|QL + named parameter on a label matcher via ``_ensure_param_controls``) + must not be flagged as dropped -- it has a working control.""" + from observability_migration.adapters.source.grafana.runtime_features import ( + ESQL_NAMED_PARAM_BINDING, + set_runtime_feature, + ) + + set_runtime_feature( + self.rule_pack, ESQL_NAMED_PARAM_BINDING, supported=True, source="test", confidence="assumed" + ) + resolver = migrate.SchemaResolver(self.rule_pack) + dashboard = { + "title": "Custom Label Filter", + "uid": "custom-label-filter", + "templating": { + "list": [ + {"name": "env", "type": "custom", "query": "prod,staging,dev"}, + ] + }, + "panels": [ + { + "id": 1, + "title": "CPU", + "type": "graph", + "targets": [{"refId": "A", "expr": 'sum(cpu{environment="$env"})'}], + } + ], + } + result = migrate.translate_dashboard( + dashboard, + datasource_index="metrics-*", + esql_index="metrics-*", + rule_pack=self.rule_pack, + resolver=resolver, + ) + self.assertNotIn("env", " ".join(result.control_warnings)) + + def test_query_variable_with_existing_specific_warning_is_not_double_disclosed(self): + """A ``query`` variable that already gets its own specific + ``control_warnings`` entry (here, Grafana's ``query_result()`` helper, + which has no Kibana populate-query equivalent) must not also collect + this pass's generic message -- one clear warning, not two competing + explanations for the same drop.""" + dashboard = { + "title": "Query result helper", + "uid": "query-result-helper", + "templating": { + "list": [ + {"name": "topn", "type": "query", "query": "query_result(topk(5, foo))"}, + ] + }, + "panels": [ + { + "id": 1, + "title": "CPU", + "type": "graph", + "targets": [{"refId": "A", "expr": 'sum(cpu{host="$topn"})'}], + } + ], + } + result = migrate.translate_dashboard( + dashboard, + datasource_index="metrics-*", + esql_index="metrics-*", + rule_pack=self.rule_pack, + ) + matching = [w for w in result.control_warnings if "topn" in w] + self.assertEqual( + len(matching), + 1, + f"expected exactly one warning naming 'topn', got: {matching}", + ) + self.assertIn("query_result()", matching[0]) + + def test_variable_bound_to_a_classic_options_control_is_not_disclosed(self): + """A ``query`` variable that resolves to a classic (non-ESQL) + ``options`` control -- built directly by ``query_variable_rule`` + without ``ESQL_NAMED_PARAM_BINDING``/``PROMQL_LABEL_MATCHER_PARAMS`` + -- must not be flagged as dropped. That control dict never carries a + top-level ``variable_name`` key (only ``_source_variable_name``, + attached afterwards in ``translate_variables``), so the disclosure + pass's ``bound_names`` lookup must check both keys, mirroring + ``_covered_control_variable_refs``.""" + from observability_migration.adapters.source.grafana.runtime_features import ( + ESQL_NAMED_PARAM_BINDING, + PROMQL_LABEL_MATCHER_PARAMS, + set_runtime_feature, + ) + + set_runtime_feature( + self.rule_pack, ESQL_NAMED_PARAM_BINDING, supported=False, source="test", confidence="assumed" + ) + set_runtime_feature( + self.rule_pack, PROMQL_LABEL_MATCHER_PARAMS, supported=False, source="test", confidence="assumed" + ) + dashboard = { + "title": "Classic options control", + "uid": "classic-options-control", + "templating": { + "list": [ + { + "name": "host", + "type": "query", + "query": "label_values(up, instance)", + "current": {"text": "All", "value": "$__all"}, + "options": [], + "multi": False, + "includeAll": True, + }, + ] + }, + "panels": [ + { + "id": 1, + "title": "Up", + "type": "graph", + "targets": [{"refId": "A", "expr": 'up{instance=~"$host"}'}], + } + ], + } + result = migrate.translate_dashboard( + dashboard, + datasource_index="metrics-*", + esql_index="metrics-*", + rule_pack=self.rule_pack, + ) + doc = result.dashboard_ir.to_yaml_dict() + controls = doc.get("controls") or [] + self.assertEqual( + [c.get("type") for c in controls], ["options"], + f"expected a classic options control, got: {controls}", + ) + self.assertNotIn( + "host", + " ".join(result.control_warnings), + "a variable with a working classic control must not be disclosed as dropped", + ) + + class ChainedVariableControlFidelityTests(unittest.TestCase): """Regression tests for issue #269: metric-scoped, label-filtered, and chained Grafana query variables must not silently drop/degrade in the From 6421d66be3610567703280ecf84028d5d806c5c0 Mon Sep 17 00:00:00 2001 From: subham sarkar Date: Wed, 19 Aug 2026 14:48:45 +0530 Subject: [PATCH 2/3] fix(grafana): name native PROMQL interval substitute in drop warnings The #356 RateInterval warning only mentioned the ES|QL TBUCKET heuristic, but dashboard 9852's native PROMQL panels inline a fixed [5m] range. Name both substitutes, document classic-control ownership keys, and note that TO_STRING is a no-op when Kibana binds keyword strings. --- docs/sources/grafana.md | 55 +++++++++++-------- .../adapters/source/grafana/panels.py | 16 ++++-- .../adapters/source/grafana/promql.py | 21 +++---- tests/test_grafana_issues_316_319.py | 3 +- tests/test_migrate.py | 9 ++- 5 files changed, 61 insertions(+), 43 deletions(-) diff --git a/docs/sources/grafana.md b/docs/sources/grafana.md index aec7362f..1c23d9d6 100644 --- a/docs/sources/grafana.md +++ b/docs/sources/grafana.md @@ -467,25 +467,30 @@ has nothing to do with the time picker — Grafana keeps the rate window fixed at, e.g., `1m` regardless of the displayed range so the line stays smooth. A duration variable used this way, or any other variable type that ends up with no control *and* no `?var` binding, is not equivalent to "handled by the time -picker" — it silently hands control of the window to whatever the migrated -query's `TBUCKET` heuristic picks, which does not track the source value and -can differ from it in either direction. +picker" — it silently hands control of the window to a translator-chosen +substitute. ES|QL panels pick a `TBUCKET` bucket-width; native PROMQL panels +typically inline a fixed range (dashboard 9852's disk panels become +`rate(...[5m])` even when Grafana's current `RateInterval` was `1m`). Neither +tracks the source value, and either can differ from it in either direction. `translate_dashboard` therefore runs one disclosure pass after every control has been synthesized (variable translation, `_ensure_param_controls`, late-bound group controls, `?var` retargeting): for every templating-list -variable whose name never ended up as a control's `variable_name`, it checks -whether any panel's *original* PromQL `expr` still references `$var` / -`${var}` — if so, it appends a `control_warnings` entry naming the variable, -so the loss is printed under `CONTROL WARNINGS` and recorded in the JSON -report / migration manifest / preflight report, matching every other control -degradation on this page. `interval` variables get a specific message calling -out the rate-window bucket-heuristic substitution; every other type gets a -generic "referenced but dropped" message. A variable that is genuinely unused -by every panel is never warned about — there is nothing lost to disclose. This -is disclosure only: the variable is not migrated into a working control (that -would require parameterizing the ES|QL duration literal, which is unverified -and out of scope for this fix). +variable that is not bound to a control — checking both the ES|QL +`variable_name` key and the classic options/range `_source_variable_name` key +(a classic control never sets `variable_name`, so looking at that key alone +would falsely flag a working dropdown as dropped) — it checks whether any +panel's *original* PromQL `expr` still references `$var` / `${var}`. If so, it +appends a `control_warnings` entry naming the variable, so the loss is printed +under `CONTROL WARNINGS` and recorded in the JSON report / migration manifest +/ preflight report, matching every other control degradation on this page. +`interval` variables get a specific message calling out both the ES|QL +`TBUCKET` substitute and the native PROMQL fixed-range inline; every other +type gets a generic "referenced but dropped" message. A variable that is +genuinely unused by every panel is never warned about — there is nothing lost +to disclose. This is disclosure only: the variable is not migrated into a +working control (that would require parameterizing the ES|QL duration literal, +which is unverified and out of scope for this fix). ### Variable Label Filters (`metric{label="$var"}` → `?var`) @@ -536,14 +541,18 @@ target can **bind ES|QL named parameters** for that migration pass: The split is therefore *per target capability*, never per individual variable. -**Multi-select.** A variable (ES|QL) control binds its selection into a scalar -parameter position (`== ?var` / `RLIKE ?var`), which cannot accept a -multi-value selection, so a Grafana multi-select variable is emitted as a -single-select control. That loss is reported (not silent) as a -`control_warnings` entry (`"variable '' was multi-select in Grafana but -binds a scalar ES|QL parameter in Kibana; emitted a single-select control"`). -Regular options controls, which do not bind a scalar parameter, keep the source -`multi` flag. +**Multi-select.** A Grafana `multi: true` variable stays multi-select in +Kibana. Scalar `== ?var` / `RLIKE ?var` cannot bind an array, so the matcher is +emitted as `MV_CONTAINS(TO_STRING(?var), ".*") OR MV_CONTAINS(TO_STRING(?var), field)` +with `single_select: false`. The `".*"` sentinel preserves Grafana's All option. +`TO_STRING` (issue #353) keeps that guardrail type-safe: Elasticsearch infers +the bound parameter's type from the JSON values Kibana sends, so numeric-looking +options (CPU indices, ports, PIDs) can arrive as an integer array and fail +compile-time type verification against the keyword sentinel/field. Some Kibana +builds still send those options as keyword strings (`["0","1"]`); `TO_STRING` on +an already-keyword value is a no-op, so the wrap is unconditional. Matching is +exact rather than regex (`RLIKE` rejects a computed pattern). Regular options +controls, which do not bind an ES|QL parameter, keep the source `multi` flag. **Value-list filters.** A `label_values(metric{device!="nbd1"}, device)` variable restricts its option list to series matching the selector. The diff --git a/observability_migration/adapters/source/grafana/panels.py b/observability_migration/adapters/source/grafana/panels.py index d4ca95e1..6fb527ff 100644 --- a/observability_migration/adapters/source/grafana/panels.py +++ b/observability_migration/adapters/source/grafana/panels.py @@ -8970,9 +8970,11 @@ def _disclose_dropped_referenced_variables(variables, controls, panels, control_ time picker", but a variable used as a rate/range-vector window (``rate(x[$RateInterval])``) has nothing to do with the displayed time range. Dropping it does not just remove a dropdown -- it silently hands - control of the rate window to the migrated query's bucket-width - heuristic, which is not equivalent (AGENTS.md: degrade gracefully, do - not hide a semantic gap). + control of the rate window to a translator-chosen substitute: ES|QL + panels use the TBUCKET bucket-width heuristic, and native PROMQL panels + typically inline a fixed range (e.g. ``[5m]``). Neither tracks the + Grafana value (AGENTS.md: degrade gracefully, do not hide a semantic + gap). Generalised to any Grafana variable type that ends up with no bound control, since the same silent loss applies to any of them -- e.g. a @@ -9022,9 +9024,11 @@ def _disclose_dropped_referenced_variables(variables, controls, panels, control_ f"variable '{name}' (type 'interval') is used by panel queries as a " f"rate/range window (e.g. '[${name}]') but was dropped during migration " "-- no Kibana control was emitted, and Kibana's time picker only " - "controls the displayed range, not this window. The migrated query's " - "bucket-width heuristic now implicitly determines the window instead, " - "which can be narrower or wider than the Grafana original" + "controls the displayed range, not this window. The migrated query no " + "longer uses the Grafana interval: ES|QL panels pick a TBUCKET " + "bucket-width, and native PROMQL panels typically inline a fixed range " + "(e.g. [5m]), either of which can be narrower or wider than the source " + "value" ) else: control_warnings.append( diff --git a/observability_migration/adapters/source/grafana/promql.py b/observability_migration/adapters/source/grafana/promql.py index ff460907..67b4fcd6 100644 --- a/observability_migration/adapters/source/grafana/promql.py +++ b/observability_migration/adapters/source/grafana/promql.py @@ -1748,16 +1748,17 @@ def _mv_contains_filter(label, param_name, negate=False, allow_empty_match_all=F computed one, so ``RLIKE MV_CONCAT(?var, "|")`` -- which would have rebuilt Grafana's own ``(a|b)`` alternation -- is not expressible. - ``?param`` is wrapped in ``TO_STRING(...)`` (issue #353): Kibana infers a - bound ES|QL parameter's type from the selected option *values*, not from - the control's keyword-typed option-list query. A variable whose values - happen to look numeric (CPU/core indices, ports, PIDs, status codes) binds - ``?param`` as an integer array, and ``MV_CONTAINS`` requires both - arguments to share a type, so the ``".*"`` sentinel (keyword) and the - keyword label field both fail to type-check against it -- a compile-time - verification error, not a runtime one, so it fails the whole query. - ``TO_STRING`` on an already-keyword parameter is a no-op, so this is safe - regardless of how Kibana ends up inferring the type. + ``?param`` is wrapped in ``TO_STRING(...)`` (issue #353): Elasticsearch + infers a bound ES|QL parameter's type from the JSON values Kibana sends, + not from the control's keyword-typed option-list query. A variable whose + values happen to look numeric (CPU/core indices, ports, PIDs, status + codes) can bind ``?param`` as an integer array; ``MV_CONTAINS`` requires + both arguments to share a type, so the ``".*"`` sentinel (keyword) and + the keyword label field both fail to type-check against it -- a + compile-time verification error, not a runtime one, so it fails the whole + query. Some Kibana versions send those same options as keyword strings + (``["0", "1"]``); ``TO_STRING`` on an already-keyword parameter is a + no-op, so wrapping unconditionally is safe either way. """ param = f"TO_STRING(?{param_name})" clauses = [] diff --git a/tests/test_grafana_issues_316_319.py b/tests/test_grafana_issues_316_319.py index 8698a49d..c5fbc6a2 100644 --- a/tests/test_grafana_issues_316_319.py +++ b/tests/test_grafana_issues_316_319.py @@ -439,7 +439,8 @@ class TestMultiSelectControlsUseMvContains(unittest.TestCase): ``?v`` is wrapped in ``TO_STRING(...)`` (issue #353) so the comparison still type-checks when Kibana infers the bound control values as an integer array instead of a keyword array (e.g. numeric label values like - CPU/core indices). + CPU/core indices). Wrapping is unconditional: ``TO_STRING`` on an + already-keyword parameter (``["0", "1"]``) is a no-op. [".*"] -> every series (All) ["a"] -> just a diff --git a/tests/test_migrate.py b/tests/test_migrate.py index 3b1a6ef3..059bc530 100644 --- a/tests/test_migrate.py +++ b/tests/test_migrate.py @@ -14596,8 +14596,9 @@ class DroppedReferencedVariableDisclosureTests(unittest.TestCase): parameter binding) must surface a control warning naming it, instead of disappearing without a trace. ``interval`` variables used as a rate/range window (dashboard 9852's ``RateInterval``) are the sharpest case, since - losing them silently hands control of the rate window to the migrated - query's bucket-width heuristic.""" + losing them silently hands control of the rate window to a + translator-chosen substitute (ES|QL TBUCKET width, or a fixed native + PROMQL range such as ``[5m]``).""" def setUp(self): self.rule_pack = migrate.RulePackConfig() @@ -14634,7 +14635,9 @@ def test_interval_variable_used_as_rate_window_is_disclosed_when_dropped(self): warnings_text = " ".join(result.control_warnings) self.assertIn("RateInterval", warnings_text) self.assertIn("rate", warnings_text.lower()) - self.assertIn("bucket", warnings_text.lower()) + self.assertIn("TBUCKET", warnings_text) + self.assertIn("[5m]", warnings_text) + self.assertIn("PROMQL", warnings_text) def test_interval_variable_never_referenced_by_a_panel_is_not_disclosed(self): """A declared-but-unused interval variable has nothing to lose -- From d99747b57cb6c96f9a509c2a46829065f82784a2 Mon Sep 17 00:00:00 2001 From: Giorgi Imerlishvili Date: Wed, 19 Aug 2026 13:59:10 +0400 Subject: [PATCH 3/3] fix(test): stop reusing the reserved "percentage" axis-label alias as incidental sample data A same-day commit (4cb7726) reserved the literal axis-label text "percentage" as an opaque Grafana-unit-id alias whose title is intentionally suppressed (so unit-inferred titles like "%" can take over instead) and added a correct, dedicated test for that behavior. It didn't touch this unrelated, pre-existing test, which happened to reuse the same literal string purely as incidental sample text for testing something else entirely (that bar charts keep axis config while omitting line/area-only appearance keys) -- breaking it on main and therefore on every open PR whose CI merges against main. Swap the incidental fixture text for an ordinary, non-reserved label so the test again exercises its own actual intent without colliding with the new opaque-alias behavior. --- tests/test_migrate.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test_migrate.py b/tests/test_migrate.py index 059bc530..b7b13db6 100644 --- a/tests/test_migrate.py +++ b/tests/test_migrate.py @@ -13110,11 +13110,17 @@ def test_extract_xy_appearance_hides_time_axis_title_from_override(self): def test_extract_xy_appearance_omits_line_area_style_for_bar(self): from observability_migration.targets.kibana.emit.display import extract_xy_appearance + # NOTE: "percentage" is deliberately NOT used here -- it is a reserved + # opaque Grafana-unit-id alias (see _OPAQUE_AXIS_TITLE_ALIASES / + # test_extract_axis_label_suppresses_grafana_unit_id_aliases) whose + # title is intentionally suppressed. This test's own purpose is + # unrelated: it only checks that bar charts keep axis config while + # dropping line/area-only appearance keys, so use ordinary label text. panel = { "fieldConfig": { "defaults": { "custom": { - "axisLabel": "percentage", + "axisLabel": "CPU usage", "drawStyle": "bars", "lineInterpolation": "smooth", "fillOpacity": 70, @@ -13123,7 +13129,7 @@ def test_extract_xy_appearance_omits_line_area_style_for_bar(self): } } appearance = extract_xy_appearance(panel, chart_type="bar") - self.assertEqual(appearance["y_left_axis"]["title"], "percentage") + self.assertEqual(appearance["y_left_axis"]["title"], "CPU usage") self.assertNotIn("line_style", appearance) self.assertNotIn("fill_opacity", appearance) area_appearance = extract_xy_appearance(panel, chart_type="area")