fix(collector): surface per-query metric errors hidden in Refresh results - #1557
Open
JaredTan95 wants to merge 1 commit into
Open
fix(collector): surface per-query metric errors hidden in Refresh results#1557JaredTan95 wants to merge 1 commit into
JaredTan95 wants to merge 1 commit into
Conversation
Contributor
|
Unsigned commits detected! Please sign your commits. For instructions on how to set up GPG/SSH signing and verify your commits, please see GitHub Documentation. |
JaredTan95
force-pushed
the
fix/1151-surface-result-errors
branch
from
September 11, 2026 09:20
33cc4be to
f6a5fa4
Compare
…ults MetricsSource.Refresh() defers per-query failures to each MetricResult and returns err == nil even when the Prometheus backend is down. Callers that only check the returned error therefore treat a total outage as an empty-but-successful refresh, and failed non-critical queries were silently dropped with no log line or error counter. - PrometheusSource.Refresh: return a joined top-level error when every query fails; partial failures stay per-result so healthy queries flow - collectReplicaMetrics: sweep per-query results, record categorized wva_metrics_collection_errors_total increments, and fail collection when all queries failed - CollectSchedulerQueueMetrics: count refresh and per-result failures under the new scheduler_queue query type so a broken query is distinguishable from an absent flow-control metric - scale-from-zero: guard the all_metrics result for nil/HasError so a failed scrape is no longer conflated with "no pending requests" and no longer panics on a missing entry - registration.QueryTypeFor: map query names to counter labels (incl. sglang/ engine prefixes) Fixes llm-d#1151 Signed-off-by: JaredTan95 <jian.tan@daocloud.io>
JaredTan95
force-pushed
the
fix/1151-surface-result-errors
branch
from
September 11, 2026 09:37
f6a5fa4 to
3f9bf17
Compare
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
1 participant
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.
Summary
MetricsSource.Refresh()returnserr == nileven when Prometheus is down — query failures are only visible inside eachMetricResult(#1151). Callers that only check the returned error treat a total outage as an empty-but-successful refresh, and failed non-critical queries were silently dropped (no log, no error counter).Changes
PrometheusSource.Refresh: return a joined top-level error when all queries fail; partial failures stay per-resultcollectReplicaMetrics: sweep per-query results — incrementwva_metrics_collection_errors_totalwith the categorized reason + debug log; fail collection only when every query failed (KV-cache / queue-length severity unchanged)CollectSchedulerQueueMetrics: count refresh and per-result failures under the newscheduler_queuequery typeall_metricsfor nil/HasError()— a failed scrape is no longer conflated with "no pending requests" (which silently pinned variants at zero replicas), and a missing entry no longer panicsregistration.QueryTypeFor()(query name → counter label, incl.sglang/prefixes) and new query types:scheduler_queue,dispatch_rate,token_metrics,latency,throughputTest plan
go build ./...; unit tests for all touched packages, incl. new cases: all queries failing →Refresherrors and collection fails; single non-critical query failing → collection succeeds, counter increments under the right query typeFixes #1151