diff --git a/docs/sources/grafana.md b/docs/sources/grafana.md index eec3d27e..1c23d9d6 100644 --- a/docs/sources/grafana.md +++ b/docs/sources/grafana.md @@ -449,6 +449,49 @@ 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 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 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`) When a dashboard's templating list defines named variables used in PromQL label @@ -498,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/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..6fb527ff 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,115 @@ 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 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 + ``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 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( + 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 +10606,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..67b4fcd6 100644 --- a/observability_migration/adapters/source/grafana/promql.py +++ b/observability_migration/adapters/source/grafana/promql.py @@ -1747,7 +1747,20 @@ 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): 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 = [] if allow_empty_match_all: # Kibana leaves an unselected multi-values control bound as an empty @@ -1755,8 +1768,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..c5fbc6a2 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,13 @@ 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). Wrapping is unconditional: ``TO_STRING`` on an + already-keyword parameter (``["0", "1"]``) is a no-op. [".*"] -> every series (All) ["a"] -> just a @@ -464,8 +493,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 +513,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..b7b13db6 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): @@ -13105,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, @@ -13118,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") @@ -14585,6 +14596,261 @@ 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 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() + + 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("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 -- + 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