Skip to content

fix(grafana): type-safe multi-select control guardrails, disclose dropped variable references - #363

Merged
giorgi-imerlishvili-elastic merged 3 commits into
elastic:mainfrom
giorgi-imerlishvili-elastic:fix/control-guardrail-type-safety
Aug 19, 2026
Merged

fix(grafana): type-safe multi-select control guardrails, disclose dropped variable references#363
giorgi-imerlishvili-elastic merged 3 commits into
elastic:mainfrom
giorgi-imerlishvili-elastic:fix/control-guardrail-type-safety

Conversation

@giorgi-imerlishvili-elastic

Copy link
Copy Markdown
Collaborator

Issues fixed

Validity

Both issues are valid.

  • Numeric control values break the template-variable guardrail, so the panel fails to compile #353: confirmed empirically against a live local Elasticsearch cluster. FROM metrics-generic-default | LIMIT 1 | EVAL matched = MV_CONTAINS(?cpu, ".*") with params: [{"cpu": [0, 1]}] (simulating Kibana binding a numeric-looking multi-select control, e.g. CPU/core indices from label_values(node_cpu_seconds_total, cpu)) returns a real verification_exception: "second argument of [MV_CONTAINS(?cpu, ".*")] must be [integer], found value [".*"] type [keyword]". This is a compile-time ES|QL error, so it fails the whole panel query, not just a subset of rows.
  • Grafana interval template variables are dropped with no control and no warning #356: interval_variable_rule intentionally emits no control for interval variables, on the assumption that Kibana's time picker covers their purpose. That assumption is false when the variable is used as a rate/range-vector window (rate(x[$RateInterval])) rather than the displayed time range — dropping it silently hands control of the rate window to the migrated query's TBUCKET bucket-width heuristic, which is not equivalent and can differ from the source value in either direction.

Root cause

  • Numeric control values break the template-variable guardrail, so the panel fails to compile #353: _mv_contains_filter in promql.py emits MV_CONTAINS(?var, ".*") / MV_CONTAINS(?var, field) unconditionally. Kibana's ES|QL control binding infers ?var's type from the selected option values, not from the control's own (keyword) option-list query text. A variable whose values look numeric binds ?var as an integer array; MV_CONTAINS requires both arguments to share a type, so the keyword ".*" sentinel and keyword label field both fail to type-check against it.
  • Grafana interval template variables are dropped with no control and no warning #356: no pass ever compared "variables referenced by panel queries" against "variables that ended up with a working control or ?var binding" — a variable that fell through every control-synthesis rule with no warning (interval variables by design, but also e.g. a hidden query variable, or any variable type whose rule didn't fire) just vanished.

Code paths checked

  • observability_migration/adapters/source/grafana/promql.py: _mv_contains_filter.
  • observability_migration/adapters/source/grafana/panels.py: _ESQL_VALUE_PARAM_FIELD_PATTERNS (field-binding detection used by curated-pack control retargeting), translate_variables / query_variable_rule (both the ES|QL param-control path and the classic non-ESQL options control path), and the new _variable_names_referenced_in_panels / _disclose_dropped_referenced_variables, wired into translate_dashboard after _ensure_param_controls, late-bound group controls, and ?var retargeting.
  • observability_migration/core/verification/parity_oracle.py: _MV_CONTAINS_PARAM_RE (used by the parity oracle to recognize which variables a query binds).
  • Curated packs that hand-author their own MV_CONTAINS(?var, ...) guardrails, bypassing _mv_contains_filter entirely: grafana_763_redis_exporter, grafana_11835_redis_exporter_helm.
  • scripts/dashboard_qa.py: its own independent MV_CONTAINS(?name text scan, used to decide whether to bind a QA probe parameter as a list or a scalar.

Fix

  1. _mv_contains_filter now wraps the parameter in TO_STRING(...) unconditionally: MV_CONTAINS(TO_STRING(?var), ".*") / MV_CONTAINS(TO_STRING(?var), field). TO_STRING on an already-keyword value is a no-op, so this is safe regardless of how Kibana ends up inferring the type. MV_COUNT(?var) == 0 is left unwrapped since MV_COUNT doesn't care about element type.
  2. Updated _ESQL_VALUE_PARAM_FIELD_PATTERNS (panels.py) and _MV_CONTAINS_PARAM_RE (parity_oracle.py) to match both the bare and TO_STRING-wrapped shapes, so existing control-retargeting and parity-oracle logic keeps recognizing the parameter binding.
  3. New _disclose_dropped_referenced_variables(variables, controls, panels, control_warnings), called once after every control-synthesis pass. For each templating-list variable: skip if never referenced by any panel's original PromQL, skip if already bound to a control (checking both the variable_name key used by ES|QL param controls and the _source_variable_name key used by classic non-ESQL controls), skip if some earlier pass already warned about it by name. Otherwise append a control_warnings entry — a specific "rate/range window... bucket-width heuristic" message for interval variables, a generic "referenced but dropped" message for every other type.
  4. Wrapped the same MV_CONTAINS(?instance, ...) shape in the two Redis exporter curated packs' hand-written query overrides (6 occurrences across grafana_763_redis_exporter and grafana_11835_redis_exporter_helm), for consistency with the now-universal type-safety guarantee.
  5. Updated scripts/dashboard_qa.py's multi-value parameter detection (new shared _is_multi_value_param helper) to recognize the TO_STRING-wrapped form, so it keeps binding multi-select QA probe parameters as lists instead of silently falling back to scalars.
  6. Documented the interval/custom variable disclosure behavior in docs/sources/grafana.md.

Side effects considered (found via independent model review before merge)

  • False positive: classic (non-ESQL) controls. The first version of _disclose_dropped_referenced_variables only checked control.get("variable_name") for "is this variable bound". A classic options/range control (emitted when the target doesn't support ES|QL named-parameter binding) never sets that key — only _source_variable_name, attached later in translate_variables. Reproduced directly: a working options control was falsely flagged as "dropped". Fixed by checking both keys, mirroring the existing _covered_control_variable_refs helper's lookup pattern. Added test_variable_bound_to_a_classic_options_control_is_not_disclosed.
  • Incomplete fix: curated-pack overrides. The generic _mv_contains_filter fix doesn't touch curated packs that hand-author their own MV_CONTAINS(?var, ...) text. Found and wrapped the two Redis exporter packs above; updated the one test locking in the old bare shape.
  • Incomplete fix: scripts/dashboard_qa.py. Its own multi-value detection only matched the bare MV_CONTAINS(?name substring; after this fix, that check would always miss the real (wrapped) shape and silently bind multi-select QA parameters as scalars. Fixed with a shared detection helper covering both shapes; added 3 new unit tests.
  • Deliberately left observability_migration/core/coverage/interaction_canary.py's hand-authored MV_CONTAINS(?services, service.name) literal unchanged — its docstring states it is intentionally "self-contained... unaffected by what a real run emits", and check_network_contract (the interaction-audit consumer) does not do any text-based MV_CONTAINS parsing, only structured params/param_kinds, so there is no behavioral risk in leaving it as-is.
  • Verified the disclosure pass doesn't double-warn: a variable already covered by a more specific existing warning (e.g. query_result()'s "no populate-query equivalent") gets exactly one warning, not two.
  • Verified genuinely unused variables (declared but never referenced by any panel) are never warned about.

Tests

  • New: TestMvContainsFilterTypeSafety and a numeric-looking-values regression test in tests/test_grafana_issues_316_319.py; DroppedReferencedVariableDisclosureTests (7 tests, including the classic-control regression) in tests/test_migrate.py; test_esql_param_control_retargets_when_panel_binds_via_to_string_wrapped_mv_contains and an updated curated-override assertion in tests/test_curated_packs.py; a wrapped-form recognition test in tests/core/test_parity_oracle.py; 3 new _is_multi_value_param tests in tests/test_dashboard_qa.py; updated assertions in tests/test_k8s_views_global_interaction_scenario.py.
  • Full suite: 5748 passed, 3 skipped, 355 subtests. make lint and make typecheck clean.

Kibana / Elasticsearch verification

#353's failure mode is a compile-time ES|QL type-check at the Elasticsearch engine level, not a Kibana rendering concern, so it was verified directly against a live local Elasticsearch cluster rather than through the Kibana UI (see Validity above): reproduced the exact verification_exception with the pre-fix bare shape, then confirmed the TO_STRING-wrapped shape executes successfully with the same integer-bound parameter. #356 only produces text in control_warnings (CLI/report output), with no Kibana-rendered element to check either way.

Review

Independent review performed via a subagent on a different model (GPT 5.6 Sol xhigh) per the team's cross-model review workflow. First pass found the three issues described above (classic-control false disclosure, unwrapped curated-pack guardrails, and the dashboard_qa.py detection gap) — all reproduced independently, fixed, and covered by new regression tests. Second pass confirmed APPROVE, no remaining blocking issues.

…pped variable references

Two related gaps in variable/control translation:

- Multi-select guardrail type mismatch (issue elastic#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 elastic#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.
The elastic#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.
… 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.
@giorgi-imerlishvili-elastic
giorgi-imerlishvili-elastic merged commit 4269b24 into elastic:main Aug 19, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants