fix(grafana): batch metric-scoped co-occurrence probes (#182) - #188
Conversation
elastic#163 made label resolution metric-aware by probing each candidate field's co-occurrence with the scoped metric via a blocking `POST /_query`, one candidate at a time until one co-occurs. Over a 500-dashboard corpus with live schema resolution this is thousands of serial round-trips, ~doubling benchmark wall-clock and slow-failing 9 dashboards past their per-dashboard budget. Collapse the round-trips on two axes, without changing which field a label resolves to (so elastic#163 behavior is preserved): - Per-candidate -> per-label: one batched probe counts every candidate at once (`WHERE metric IS NOT NULL | STATS COUNT(c0), COUNT(c1), ...`), semantically identical to the old `metric AND cand IS NOT NULL | COUNT(*)` check. Results map back by column name (robust to column reordering) and still populate the cross-dashboard `(metric, candidate)` cache. - Per-label -> per-metric: `prime_label_cooccurrence(labels, metric_field)` pre-warms the cache for a whole fragment's selector + group-by labels in a single query, wired into the `_frag_*` helpers in promql.py. Subsequent per-label resolutions hit the warm cache. Local lab validation (5 bundled dashboards, live metrics-*): probe /_query calls 686 -> 360 (-48%), probe seconds 13.25 -> 4.56 (-66%), wall-clock -30%, and the 274 generated ES|QL queries are byte-for-byte identical across baseline and after (no correctness regression). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`resolve_label` short-circuits rule-pack `ignored_labels` and `label_rewrites` before any co-occurrence probe, but `prime_label_cooccurrence` walked the raw label set through `_scoped_candidate_fields`, so a rewritten or ignored label issued an unnecessary `/_query` probe that resolution would never make — working against this PR's goal of cutting probe volume. Mirror the `resolve_label` short-circuits during priming. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Staging A/B validation (network-RTT regime)Ran an A/B on a remote staging cluster (Serverless,
Per-candidate → batched (this PR's effect):
Correctness: compiled YAML is byte-identical across all three versions, and all 30 Effect on the two open acceptance criteriaThis staging cluster (tiny indices) cannot tick either production-gated box, but the A/B nails down the mechanism:
Net: strengthens confidence in the fix — round-trip reduction converts to real wall-clock savings under network latency, output-preserving — but the two remaining boxes still require the production benchmark cluster with large Caveats: single dashboard, single run per version; directionally strong but not averaged over repetitions. |
giorgi-imerlishvili-elastic
left a comment
There was a problem hiding this comment.
Thanks for this — the approach is clean and the core transformation is sound. I verified locally: all unit tests pass (3027 passed), ruff and mypy are clean, and the batched probe is semantically equivalent to the old per-pair check in the happy path. COUNT(\cand`)scoped toWHERE `metric` IS NOT NULLis indeed identical to the oldmetric AND cand IS NOT NULL | COUNT(*), the candidate priority order in _scoped_candidate_fields` matches the previous inline logic, and the column-name remapping is robust. New tests cover the batched probe, column-name mapping, per-pair caching, per-metric priming, and the ignored/rewritten short-circuit. Nice work, and the priming-as-pure-cache-prefill design keeps it safe by construction.
I'm holding off on approval for two reasons.
1. The batched probe couples the fate of all candidates — this can silently re-introduce the #163 failure mode (correctness)
What: In the old _resolve_label_scoped_to_metric, candidates were probed one at a time and the loop returned on the first co-occurring candidate, so a probe error on a lower-priority candidate never affected resolution of the primary one. Now _resolve_label_scoped_to_metric issues a single STATS COUNT(\cand0`), COUNT(`cand1`), …over every advertised candidate, and_probe_cooccurrence_batchreturnsNonefor the **entire** batch on any non-200 / exception (schema.py:457-458, 475-476)._cooccurring_candidatesthen cachesNonefor every candidate, so the loop finds nothing truthy and_resolve_label_scoped_to_metricreturnsNone` → the label falls back to index-global resolution.
Why it matters: A single problematic candidate field poisons the whole label's resolution. This is realistic in exactly the multi-index / dual-shipping metrics-* environments this code targets: if a lower-priority candidate has conflicting types across backing indices, ES|QL fails the query with a verification_exception (400). Previously only that one candidate's probe failed and the primary candidate still resolved correctly; now the primary candidate never gets confirmed, the metric-scoped resolution returns None, and the label silently reverts to the index-global field — which is precisely the disjoint-document-set bug #163 was written to prevent. So the PR's central claim ("without changing which field a label resolves to … #163 behavior is fully preserved") does not hold when the batch query errors. There's also a wider blast radius for transient errors: one blip now caches None for the whole label set of a metric for the rest of the run, not just one pair.
What to change: Either (a) on a batch error, fall back to probing candidates individually (or at least re-probe the primary candidate alone) so one incompatible/ambiguous field can't suppress resolution of the others, or (b) if the intent is to accept this trade-off, please document it explicitly and add a test for "batched probe errors → resolution must still match the per-pair baseline" so the divergence is intentional and guarded. A regression test driving a batch 400 with a co-occurring primary candidate would capture the gap.
2. The primary acceptance criteria are still unverified (completeness)
What: The two acceptance boxes that represent the actual goal of #182 — "Benchmark runtime near the 14c0a94 baseline" and "the 9 slow-failed dashboards migrate again" — are unchecked, marked "to be confirmed on the production benchmark cluster."
Why it matters: The local 5-dashboard numbers (−48% probes, −30% wall-clock) are a good leading indicator, but the regression that motivated the issue was observed only on the remote production cluster, where each probe is a real RTT. Until the production benchmark confirms the wall-clock recovery and the 9 dashboards completing, we don't yet have evidence the PR fixes the reported problem rather than just reducing probe count on a fast local cluster.
What to change: Please post the production benchmark run (wall-clock vs. the 14c0a94 baseline and confirmation the 9 dashboards migrate within budget), or note explicitly if this is intended to merge ahead of that validation and why.
Happy to approve once the batch-error fallback is addressed (or the trade-off is documented + tested) and the production numbers are in.
… errors A batched STATS couples every candidate's fate: one incompatible field (e.g. a type conflict across dual-shipping metrics-* indices → verification_exception) fails the whole query, caching None for every candidate. The label then reverts to index-global resolution — exactly the disjoint-document-set bug elastic#163 was written to prevent, breaking the PR's "no change in which field a label resolves to" guarantee. On a multi-candidate batch error, re-probe each candidate alone so one bad field can't suppress the others, matching the pre-elastic#182 per-pair behaviour. The fan-out is the error path only; the happy path still costs a single probe, and a single-candidate batch is not re-probed (it is already the per-candidate query). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@giorgi-imerlishvili-elastic — thanks for the careful read; the batch-coupling failure mode is real and the fix is in 1. Batch error no longer couples candidates (fixed + tested)You're right that returning
Two regression tests added in
2. Production benchmark — still outstanding, agreedThe two production-gated boxes remain unchecked. The staging A/B above nails the mechanism under real network RTT (−31% wall-clock, −35% round-trips, byte-identical compiled YAML), but as noted there a tiny-index staging cluster cannot tick either box — both genuinely require the production benchmark cluster with large |
Summary
Closes #182.
#163made label resolution metric-aware by probing each candidate field's co-occurrence with the scoped metric via a blockingPOST /_query, one candidate at a time until one co-occurs. Over a 500-dashboard corpus with live schema resolution (--es-url) this is thousands of serial round-trips — ~doubling benchmark wall-clock (34m → 60m) and slow-failing 9 dashboards past their per-dashboard budget.This PR collapses those round-trips on two axes without changing which field a label resolves to, so the
#163behavior is fully preserved:COUNT(field)scoped to where the metric is non-null is semantically identical to the oldmetric AND cand IS NOT NULL | COUNT(*)check. Results map back by column name (robust to ES|QL column reordering) and still populate the cross-dashboard(metric, candidate)cache.prime_label_cooccurrence(labels, metric_field)pre-warms the cache for a whole panel fragment's selector + group-by labels in a single query, wired into the_frag_*helpers inpromql.py. The subsequent per-label resolutions then hit the warm cache and issue no further round-trips.Changes
adapters/source/grafana/schema.py:_probe_cooccurrence_batch(single multi-field query),_cooccurring_candidates(cache-first batched resolution), extracted_scoped_candidate_fields, newprime_label_cooccurrence._cooccursretained as a thin per-pair wrapper.adapters/source/grafana/promql.py:_prime_frag_label_cooccurrence, wired into_frag_filters,_frag_group_labels,_frag_has_incompatible_target_fields, and_frag_has_incompatible_group_fields.Testing
Unit + lint + typecheck: 3026 tests passed,
make lintclean,make typecheck(mypy) clean. New tests cover the batched probe, column-name mapping, per-pair caching, per-metric priming, and single-probe panel translation.#163correctness tests intests/test_metric_aware_label_resolution.pypreserved.Local lab validation — migrate 5 bundled dashboards against live
metrics-*with--es-url, baseline (per-candidate probes) vs. this PR:/_querycallsCOUNT(*)COUNT(field)The −48% probe-count reduction is the deterministic, timing-independent signal; the 274 generated ES|QL queries are byte-for-byte identical across both runs (no correctness regression). On the production remote cluster — where each probe is a real network RTT against large indices (the source of the +26m regression) — eliminating ~half the round-trips and the per-candidate sequential fan-out should recover substantially more wall-clock than the −30% observed on this fast local cluster.
Acceptance criteria
tests/test_metric_aware_label_resolution.py(#163behavior preserved — identical ES|QL output).14c0a94baseline — to be confirmed on the production benchmark cluster.🤖 Generated with Claude Code