fix: use exact vLLM counter metric names for GMP compatibility - #568
Conversation
kubernetes-prow
Bot
added
cncf-cla: yes
|
verified with the new change I can get the correct prefix_cache_hit metrics with GMP |
|
Note this change will break queries to pre-v0.7.0 vLLM, but given its a year and a half old its likely acceptable. @jjk-g @SachinVarghese any thoughts here? |
|
/priority important-longterm |
kubernetes-prow
Bot
added
priority/important-longterm
|
Needs a rebase, and e2e-tests is failing at this head. |
Google Managed Prometheus rejects regex matchers on __name__ with HTTP 400,
silently zeroing every counter declared via {__name__=~"name(_total)?"}.
CounterMetric now queries the _total-suffixed and bare forms exactly, joined
with 'or', which works on GMP and still covers legacy expositions.
039a307 to
e2210a4
Compare
kubernetes-prow
Bot
added
size/L
github-actions
Bot
added
the
do-not-merge
|
Rebased onto main and reworked on top of the #382/#699 metric refactor. The exact- |
|
Approach verified. I ran the generated queries against a real Prometheus: all The e2e failure is unrelated. Two asks before lgtm:
Worth adding to the description: this is broader than GMP. On main It also explains why #697's drift check was silent. Nit, non-blocking: |
github-actions
Bot
removed
the
do-not-merge
The drift checks decided whether a declared name resolves by restating the
exposition's naming conventions ("a counter matches X or X_total") in the test
utils, a second copy of what Metric.get_queries already implements. The copies
drifted: CounterMetric("vllm:prompt_tokens") queries the bare name only, while
the check accepted the declaration because v0.26.0 exposes
vllm:prompt_tokens_total. The check passed on a metric whose query selects
nothing, which is the exact failure mode it exists to catch.
Metric.candidate_names() now reports the series a metric's queries select, as
groups that are OR'd with the names within a group AND'd, and both drift checks
ask the metric rather than restating the rules. The test utils keep only the
series to family/type mapping, which is genuinely fixture-format knowledge.
Against the v0.26.0 golden this reds exactly vllm:prompt_tokens and
vllm:generation_tokens, both fixed by kubernetes-sigs#568. They go on a KNOWN_UNRESOLVED list,
kept separate from CONDITIONALLY_EXPOSED (metrics gated off on a stock server)
because the reason differs, and guarded by
test_known_unresolved_still_do_not_resolve so the entries cannot outlive the fix.
Part of kubernetes-sigs#669.
github-actions
Bot
added
the
do-not-merge
|
Thanks for the thorough verification. Addressed in f0d31c9:
|
github-actions
Bot
removed
the
do-not-merge
|
Bump up this fix as the issue blocks multiple work streams |
The drift checks decided whether a declared name resolves by restating the
exposition's naming conventions ("a counter matches X or X_total") in the test
utils, a second copy of what Metric.get_queries already implements. The copies
drifted: CounterMetric("vllm:prompt_tokens") queries the bare name only, while
the check accepted the declaration because v0.26.0 exposes
vllm:prompt_tokens_total. The check passed on a metric whose query selects
nothing, which is the exact failure mode it exists to catch.
Metric.candidate_names() now reports the series a metric's queries select, as
groups that are OR'd with the names within a group AND'd, and both drift checks
ask the metric rather than restating the rules. The test utils keep only the
series to family/type mapping, which is genuinely fixture-format knowledge.
Against the v0.26.0 golden this reds exactly vllm:prompt_tokens and
vllm:generation_tokens, both fixed by kubernetes-sigs#568. They go on a KNOWN_UNRESOLVED list,
kept separate from CONDITIONALLY_EXPOSED (metrics gated off on a stock server)
because the reason differs, and guarded by
test_known_unresolved_still_do_not_resolve so the entries cannot outlive the fix.
Part of kubernetes-sigs#669.
The drift checks decided whether a declared name resolves by restating the
exposition's naming conventions ("a counter matches X or X_total") in the test
utils, a second copy of what Metric.get_queries already implements. The copies
drifted: CounterMetric("vllm:prompt_tokens") queries the bare name only, while
the check accepted the declaration because v0.26.0 exposes
vllm:prompt_tokens_total. The check passed on a metric whose query selects
nothing, which is the exact failure mode it exists to catch.
Metric.candidate_names() now reports the series a metric's queries select, as
groups that are OR'd with the names within a group AND'd, and both drift checks
ask the metric rather than restating the rules. The test utils keep only the
series to family/type mapping, which is genuinely fixture-format knowledge.
Against the v0.26.0 golden this reds exactly vllm:prompt_tokens and
vllm:generation_tokens, both fixed by kubernetes-sigs#568. They go on a KNOWN_UNRESOLVED list,
kept separate from CONDITIONALLY_EXPOSED (metrics gated off on a stock server)
because the reason differs, and guarded by
test_known_unresolved_still_do_not_resolve so the entries cannot outlive the fix.
Part of kubernetes-sigs#669.
|
Re-reviewed at the last commit. All three asks are done, and I checked the two load-bearing claims rather than taking them: Not holding lgtm on the refreshed GMP artifact: the Verification section records the direct GMP result, and the CI assertion on both name forms is better evidence. Worth attaching one to #567 when convenient. /lgtm |
kubernetes-prow
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bslabe123, zetxqx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
approved
The drift checks decided whether a declared name resolves by restating the
exposition's naming conventions ("a counter matches X or X_total") in the test
utils, a second copy of what Metric.get_queries already implements. The copies
drifted: CounterMetric("vllm:prompt_tokens") queries the bare name only, while
the check accepted the declaration because v0.26.0 exposes
vllm:prompt_tokens_total. The check passed on a metric whose query selects
nothing, which is the exact failure mode it exists to catch.
Metric.candidate_names() now reports the series a metric's queries select, as
groups that are OR'd with the names within a group AND'd, and both drift checks
ask the metric rather than restating the rules. The test utils keep only the
series to family/type mapping, which is genuinely fixture-format knowledge.
Against the v0.26.0 golden this reds exactly vllm:prompt_tokens and
vllm:generation_tokens, both fixed by kubernetes-sigs#568. They go on a KNOWN_UNRESOLVED list,
kept separate from CONDITIONALLY_EXPOSED (metrics gated off on a stock server)
because the reason differs, and guarded by
test_known_unresolved_still_do_not_resolve so the entries cannot outlive the fix.
Part of kubernetes-sigs#669.
The drift checks decided whether a declared name resolves by restating the
exposition's naming conventions ("a counter matches X or X_total") in the test
utils, a second copy of what Metric.get_queries already implements. The copies
drifted: CounterMetric("vllm:prompt_tokens") queries the bare name only, while
the check accepted the declaration because v0.26.0 exposes
vllm:prompt_tokens_total. The check passed on a metric whose query selects
nothing, which is the exact failure mode it exists to catch.
Metric.candidate_names() now reports the series a metric's queries select, as
groups that are OR'd with the names within a group AND'd, and both drift checks
ask the metric rather than restating the rules. The test utils keep only the
series to family/type mapping, which is genuinely fixture-format knowledge.
Against the v0.26.0 golden this reds exactly vllm:prompt_tokens and
vllm:generation_tokens, both fixed by kubernetes-sigs#568. They go on a KNOWN_UNRESOLVED list,
kept separate from CONDITIONALLY_EXPOSED (metrics gated off on a stock server)
because the reason differs, and guarded by
test_known_unresolved_still_do_not_resolve so the entries cannot outlive the fix.
Part of kubernetes-sigs#669.
The drift checks decided whether a declared name resolves by restating the
exposition's naming conventions ("a counter matches X or X_total") in the test
utils, a second copy of what Metric.get_queries already implements. The copies
drifted: CounterMetric("vllm:prompt_tokens") queries the bare name only, while
the check accepted the declaration because v0.26.0 exposes
vllm:prompt_tokens_total. The check passed on a metric whose query selects
nothing, which is the exact failure mode it exists to catch.
Metric.candidate_names() now reports the series a metric's queries select, as
groups that are OR'd with the names within a group AND'd, and both drift checks
ask the metric rather than restating the rules. The test utils keep only the
series to family/type mapping, which is genuinely fixture-format knowledge.
Against the v0.26.0 golden this reds exactly vllm:prompt_tokens and
vllm:generation_tokens, both fixed by kubernetes-sigs#568. They go on a KNOWN_UNRESOLVED list,
kept separate from CONDITIONALLY_EXPOSED (metrics gated off on a stock server)
because the reason differs, and guarded by
test_known_unresolved_still_do_not_resolve so the entries cannot outlive the fix.
Part of kubernetes-sigs#669.
The drift checks decided whether a declared name resolves by restating the
exposition's naming conventions ("a counter matches X or X_total") in the test
utils, a second copy of what Metric.get_queries already implements. The copies
drifted: CounterMetric("vllm:prompt_tokens") queries the bare name only, while
the check accepted the declaration because v0.26.0 exposes
vllm:prompt_tokens_total. The check passed on a metric whose query selects
nothing, which is the exact failure mode it exists to catch.
Metric.candidate_names() now reports the series a metric's queries select, as
groups that are OR'd with the names within a group AND'd, and both drift checks
ask the metric rather than restating the rules. The test utils keep only the
series to family/type mapping, which is genuinely fixture-format knowledge.
Against the v0.26.0 golden this reds exactly vllm:prompt_tokens and
vllm:generation_tokens, both fixed by kubernetes-sigs#568. They go on a KNOWN_UNRESOLVED list,
kept separate from CONDITIONALLY_EXPOSED (metrics gated off on a stock server)
because the reason differs, and guarded by
test_known_unresolved_still_do_not_resolve so the entries cannot outlive the fix.
Part of kubernetes-sigs#669.
The drift checks decided whether a declared name resolves by restating the
exposition's naming conventions ("a counter matches X or X_total") in the test
utils, a second copy of what Metric.get_queries already implements. The copies
drifted: CounterMetric("vllm:prompt_tokens") queries the bare name only, while
the check accepted the declaration because v0.26.0 exposes
vllm:prompt_tokens_total. The check passed on a metric whose query selects
nothing, which is the exact failure mode it exists to catch.
Metric.candidate_names() now reports the series a metric's queries select, as
groups that are OR'd with the names within a group AND'd, and both drift checks
ask the metric rather than restating the rules. The test utils keep only the
series to family/type mapping, which is genuinely fixture-format knowledge.
Against the v0.26.0 golden this reds exactly vllm:prompt_tokens and
vllm:generation_tokens, both fixed by kubernetes-sigs#568. They go on a KNOWN_UNRESOLVED list,
kept separate from CONDITIONALLY_EXPOSED (metrics gated off on a stock server)
because the reason differs, and guarded by
test_known_unresolved_still_do_not_resolve so the entries cannot outlive the fix.
Part of kubernetes-sigs#669.
3 participants
What
Query counters by both exact name forms (
name_total or name) instead of a__name__regex matcher.Why
Fixes #567, but the bug is broader than GMP. A counter's stored series name depends on the exporter: modern
prometheus_clientappends_totalto every counter sample, while older exporters and OTel re-exports keep the bare family name. Any query that commits to a single name form silently reads 0 whenever the deployment stores the other one:=~on__name__with HTTP 400 (=~ is an unsupported matchtype for the __name__ label), so every counter declared via{__name__=~"name(_total)?"}(e.g.prefix_cache_hit_percent) reads 0 on GMP.prompt_tokensandgeneration_tokensquery the bare names, but the v0.26.0 exposition golden shows a stock vLLM only storesvllm:prompt_tokens_total/vllm:generation_tokens_total, soprompt_lenandoutput_lenread 0 against stock vLLM on any Prometheus, not just GMP.CounterMetricnow emits both exact forms joined withor(e.g.sum(increase(vllm:request_success_total{...}[60s]) or increase(vllm:request_success{...}[60s]))), which GMP accepts, keeps legacy bare names (pre-v0.7.0) resolving, and makes theis_exposed"bare or_total" assumption in the drift check (#697) true at query time.orunions series with distinct label sets, so mixed fleets still sum correctly. Names that cannot carry_total(_count/_sum/_bucketseries) keep a single exact leg.The
{__name__=~...}selector support is removed fromCounterMetricand now rejected up front likeGaugeMetric/HistogramMetricalready do. Query goldens regenerated withUPDATE_QUERY_GOLDENS=1.e2e/tests/test_metrics_fallback.pynow also exposes a prompt-tokens counter from its mock in both name forms and assertsprompt_len.rate > 0, so CI holds a non-zero Prometheus token counter for each leg of theor. (The assertion cannot live intest_prometheus.py: llm-d-inference-sim v0.6.1 as pinned in flake.nix does not exportvllm:prompt_tokens/vllm:generation_tokensat all, only therequest_prompt_tokenshistogram, soprompt_lenis always 0 against the sim.)Verification
e2e/tests/test_metrics_fallback.py(legacy bare and_total-suffixed expositions, including the newprompt_len.rate > 0assertions) passes locally against a real Prometheus.pdm run test,pdm run validate, and the serverless golden checks (test_declared_names_resolve_against_goldens) pass.