refactor(chart): apply three dashboard conventions across every board - #1368
Open
Tanguille wants to merge 2 commits into
Open
refactor(chart): apply three dashboard conventions across every board#1368Tanguille wants to merge 2 commits into
Tanguille wants to merge 2 commits into
Conversation
…eries color Three conventions made consistent across every dashboard, no panels or features added: - percentunit ratio-against-a-hard-limit panels get min:0/max:1 so they read against the limit instead of autoscaling (llmkube-quota GPU/VRAM utilization). - sum()/count() queries that can return an empty vector on a fresh cluster get noValue:"0" instead of "No data" (model-router request and fail-closed rates; sglang/vllm throughput, queue depth, and pool panels). - Panels whose series vary with a template variable (service/slo) use color.mode palette-classic-by-name so filtering doesn't repaint survivors (every timeseries panel in sglang-dashboard, vllm-dashboard, and the llmkube-slo over-time/burn-rate panels). amd-gpu-observability and llmkube-inference have no qualifying panels and are untouched. model-router's own $router variable isn't wired into any query, so its series aren't template-driven and its colors are left alone. Signed-off-by: Tanguille <91473554+Tanguille@users.noreply.github.com>
…Conventions
Walks fieldConfig.defaults per panel (dashboardPanels), not the flat
expr list dashboardQueries returns, so a rule can see a panel's unit,
color and target queries together. Table-driven over three rules,
each scoped to timeseries panels (stat/gauge color by threshold,
tables have no per-series color or axis, so none of the three read on
them the way they do on a graph):
- percentunit + a bare metric/metric division -> min:0/max:1. Error
budgets ("1 - (...)") and burn-rate reference lines
("14 * (1 - $objective/100)") are excluded structurally: more than
one '/' or any '*'/'+' in the expr disqualifies it.
- sum()/count() as the outer operator -> noValue:"0". A ratio-of-sums
like sum(a)/sum(b) starts with "sum(" too, but its outer op is the
division; the sum() match must close at the very end of the trimmed
expr to count, which excludes it without an explicit list.
- a template variable inside a {...} label matcher ->
color.mode:"palette-classic-by-name". model-router's $router isn't
referenced in any query, so it correctly stays unmatched.
Scoped to charts/llmkube/dashboards only: config/grafana's standalone
pair predates the convention pass and was never touched by it.
Passes clean on the current tree, and a targeted sanity check
(stripping min/max from one panel) confirmed it fails when a
convention is actually violated.
Signed-off-by: Tanguille <91473554+Tanguille@users.noreply.github.com>
This was referenced Jul 30, 2026
Tanguille
marked this pull request as ready for review
July 30, 2026 21:33
Defilan
reviewed
Jul 31, 2026
Defilan
left a comment
Member
There was a problem hiding this comment.
Tanguille, this is one of the cleaner refactors I've seen, thank you. Zero PromQL touched (I diffed it: 0 changed expr lines), the key-change tally matches your description exactly, and the guard test is genuinely falsifiable, it already caught 41 real inconsistencies in the pre-PR dashboards. One thing before I approve: please add the AI-assistance disclosure line to the PR body per CONTRIBUTING (Assisted-by: <tool> (what it did; what you verified)). Add that and I'll approve and merge this one first, since #1369/#1370/#1371 all stack on it and will rebase onto it.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of four. The three layers stacked on this branch (operator-health row, SGLang panels, llama.cpp board) follow one at a time as this merges, so each arrives as its own reviewable diff rather than one large change.
Adds no panels and no features: it exists so those layers inherit one convention instead of each inventing their own.
What
min: 0/max: 1on percentunit ratios against a hard limitnoValue: "0"where the outer PromQL issum()/count()palette-classic-by-namewhere series vary with a template variablePlus
TestDashboardConventionsininternal/metrics/dashboard_sync_test.go, so a future dashboard cannot silently violate any of the three. It walksfieldConfig.defaultsper panel, pairing each panel with its own target exprs, and scopes the rules structurally rather than with an exemption list: error budgets (1 - (...)) and burn-rate thresholds (14 * (...)) fall out of rule 1 by shape, andsum(a)/sum(b)ratio-of-sums falls out of rule 2 because thesum()match does not close at the end of the string.Why
palette-classicassigns colour by series order, so filtering$servicerepaints the survivors and a reader who learned "service X is green" is misled. Ratio panels without amaxautoscale, so a 2% blip renders identically to saturation.count()over an empty vector reads "No data" on a fresh cluster instead of 0.Not in scope
Legend calcs (
["mean","max"]is the existing convention and stays), fillOpacity (adrawStyle: barspanel legitimately needs a high fill), colours on fixed series sets, andconfig/grafanas two boards, which predate this pass.Evidence
amd-gpu-observabilityandllmkube-inferencehave no qualifying panels.helm unittest charts/llmkube60/60,go test ./internal/metrics/...ok, all dashboard JSON valid.noValuemakes it fail naming the exact panel, restoring makes it pass. It has already caught six real violations in the layers stacked on top, two of them in a commit that claimed to have fixed them.Happy to split or reorder if you would rather see the test separately from the JSON changes.
Live validation
Chart rendered from all four branches merged, applied to a production cluster (AMD R9700, SGLang v0.5.16 + three llama.cpp services) behind a suspended HelmRelease, then reverted and proved clean.
33 of 37 panels return live data. The four empties: two are
histogram_quantileover an idle operator, one was the queue-wait panel awaiting its recording rules (which materialised on apply at p95 18.0s / p50 1.0s), and one was a real defect in the llama.cpp branch, now fixed there.Revert proof: recording rules back to the 5 the release ships, dashboards back to the 2 Flux owns, zero suspended Flux objects.