feat(observability): report real prefix-cache query/hit counters in /metrics - #953
Conversation
…metrics Thread prefix-cache query/hit counters from the qwen3 scheduler through SchedulerMetrics into the vLLM SchedulerStats.prefix_cache_stats surface, so Prometheus /metrics no longer reads zeros for prefix cache hit rate. - pegainfer-qwen3: accumulate per-step prefix_queries/prefix_hits in StepEffects (one query per first-chunk request; hits = cached_tokens), fold into cumulative counters on the scheduler, expose via metrics(). - pegainfer-frontend: add prefix_cache_queries/hits to SchedulerMetrics and map them to PrefixCacheStats in the vLLM bridge. This complements the cached_tokens usage path (TokenEvent::Scheduled) that upstream already landed for pegainfer-project#603; it covers the /metrics consumer only. Verified: pegainfer-frontend --lib tests pass (65); cargo clippy and cargo check --workspace --lib clean. qwen3 A100 e2e blocked locally by a rdma-mummy-sys bindgen environment issue unrelated to this change.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab0234f87f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| queries: snapshot.prefix_cache_queries, | ||
| hits: snapshot.prefix_cache_hits, |
There was a problem hiding this comment.
For Qwen3's stepped bridge, SchedulerMetrics carries monotonic totals and dispatch_step sends scheduler_stats_from(&self.scheduler.metrics()) on every output batch; vLLM's Prometheus logger increments prefix_cache_queries/hits by the values in each SchedulerStats. Once any cached request has run, every subsequent token batch re-adds the same cumulative totals, so /metrics overcounts prefix-cache traffic until the process restarts. Please compute per-send deltas here, like the existing spec-decode path does.
Useful? React with 👍 / 👎.
| effects.prefix_queries += 1; | ||
| effects.prefix_hits += result.cached_tokens as u64; |
There was a problem hiding this comment.
For any prompt longer than one token, this makes the query denominator request-granularity (+1) while prefix_hits is token-granularity (cached_tokens). vLLM's prefix-cache counters are meant to be compared as hit tokens / queried tokens, so a repeated ~1900-token prompt would report roughly 1888 hits over 1 query and produce impossible hit rates above 100%. Increment queries by the queried/cacheable prompt token count instead.
Useful? React with 👍 / 👎.
|
Please fix the token-vs-request query-unit mismatch and cumulative-counter double counting, then provide an exact-head, multi-batch scheduler-to-/metrics E2E result showing stable non-zero deltas across scrapes; the PR currently reports that Qwen3 E2E was blocked, so no E2E result has been provided. |
…r-send deltas Two review bugs on the prefix-cache `/metrics` surface, both fixed: 1. Unit mismatch (queries vs hits). Previously `prefix_queries += 1` (a request) while `prefix_hits += cached_tokens` (a token count), so `hit_rate = hits/queries` could exceed 100%. Now both are TOKEN-granular, matching vLLM's `PrefixCacheStats`: `prefix_queries` counts the prompt tokens looked up in the cache and `prefix_hits` counts the cached tokens. Because cached <= prompt, `hits <= queries` and the rate stays in [0, 1]. Guarded by the existing `prefill_pos == 0` check so each request is counted exactly once (no double counting across chunked prefill). 2. Cumulative overcount in Prometheus. The scheduler holds running totals, but `dispatch_step` / `publish_scheduler_stats` shipped that running total on *every* token batch, and the frontend adds each `SchedulerStats` value into its `prefix_cache_*_total` counters — so a cached request re-added the whole history on every subsequent batch until restart. Now the bridge ships per-send DELTAS (cur - last), mirroring the existing spec-decode path: `prefix_cache_delta()` in bridge.rs, with `last_prefix_*` state in both the legacy `publish_scheduler_stats` loop and the stepped bridge (AtomicU64, since `dispatch_step` takes `&self`). Adds a `FakeExecutor` prefix-hit hook, a multi-batch/multi-scrape qwen3 scheduler test (`prefix_cache_metrics_stable_across_batches_and_scrapes`, token-granular assertions), and a frontend test (`prefix_cache_stats_are_per_interval_deltas_not_running_totals`) that proves the bridge ships the interval delta, not the running total. Signed-off-by: yuntaonie <1571859588@qq.com>
SummaryFixes two issues flagged on the prefix-cache /metrics counters: Token-vs-request query-unit mismatch (pegainfer-qwen3/src/scheduler/resolve.rs): prefix_queries was incremented per-request (+= 1) while prefix_hits was token-granular (+= result.cached_tokens), so repeated prompts reported >100% hit rates. Both are now token-granular: prefix_queries += req.prompt_tokens.len(), prefix_hits += result.cached_tokens (cached_tokens ≤ prompt_tokens ⇒ rate ∈ [0,1]). Verificationcargo test --release -p pegainfer-frontend --lib vllm::bridge → 14/14 passed, including prefix_cache_stats_are_per_interval_deltas_not_running_totals (interval 1 ships (100,37); interval 2 after totals reach (200,74) ships delta (100,37) not (200,74); idle ships (0,0)). Note on E2EThe live A100 /metrics multi-batch scrape requested in review could not be produced in this environment: the qwen3 build chain pulls rdma-mummy-sys, whose bindgen (0.66) generates _address fields under the local toolchain, breaking that third-party crate's compile — unrelated to this change. The overcount behavior is directly covered by the passing bridge delta test. If CI runs on this head, the bridge test goes green; I can supply a real /metrics scrape snapshot once a qwen3-buildable environment is available. |
Summary
Threads prefix-cache query/hit counters from the qwen3 scheduler through
SchedulerMetricsinto the vLLMSchedulerStats.prefix_cache_statssurface,so Prometheus
/metricsno longer reports zeros for prefix-cache hit rate.pegainfer-qwen3: accumulate per-stepprefix_queries/prefix_hitsinStepEffects(one query per first-chunk request; hits =cached_tokens),fold into cumulative counters on the scheduler, expose via
metrics().pegainfer-frontend: addprefix_cache_queries/prefix_cache_hitstoSchedulerMetricsand map them toPrefixCacheStatsin the vLLM bridge.Relation to #814 / #669 / #603
This supersedes #814 (which was opened from
feat/sim-frontend-prefix-cache-metrics,head
7f24658e, against the pre-rename tree and went CONFLICTING due to#841PegaInfer rename +#824kv-store refactor). Per the maintainer noteon #814, this is the single canonical PR for the prefix-cache reporting surface.
Scope split to avoid two parallel implementations:
cached_tokens→ OpenAI usage): already landed upstreamvia
TokenEvent::Scheduled(issue metrics: report real prefix-cache query/hit counters from the qwen3 scheduler #603 line). Not touched here./metricssurface (prefix_cache_queries/hits→ Prometheus): this PR.Verified
cargo test -p pegainfer-frontend --lib— 65 passed (incl. bridgeprefix_cache_statsmapping).cargo clippy+cargo check --workspace --lib— clean.Note: a full qwen3 A100 e2e was blocked in this local environment by a
rdma-mummy-sysbindgen issue (missing vendoredrdma-core-mummyheaders) thatis unrelated to this change and does not touch rdma code; upstream CI with the
full rdma toolchain will validate the qwen3 build.