Repository navigation
fix(worker): query engine-specific HTTP load endpoint for vLLM/SGLang - #1867
Conversation
📝 WalkthroughWalkthroughAdds a ChangesAbsolute Token Data Gating and Prometheus Load Scraping
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant WorkerMonitor
participant fetch_http_load
participant authed_get
participant PromScrape
participant Worker
WorkerMonitor->>fetch_http_load: poll worker load
fetch_http_load->>authed_get: GET /metrics or /v1/loads
authed_get->>Worker: HTTP request
Worker-->>authed_get: response body
authed_get-->>fetch_http_load: raw body
fetch_http_load->>PromScrape: parse(body)
PromScrape-->>fetch_http_load: metric map
fetch_http_load-->>WorkerMonitor: WorkerLoadResponse
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
protocols
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fallback mechanism to fetch worker load metrics from Prometheus /metrics endpoints for vLLM and SGLang runtimes when native /v1/loads endpoints are unavailable. It adds a minimal Prometheus text-format parser, normalizes the scraped metrics into a single-rank load response, and ensures that ratio-only metrics do not poison the absolute token-based DP-rank routing cache. The reviewer feedback is highly constructive, pointing out that a dedicated parsing library like prometheus-parse should be preferred over custom string manipulation, and identifying metric name changes in newer versions of vLLM and SGLang that require backward-compatible handling.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-designed PR. The engine-specific dispatch, ratio-vs-absolute-token distinction, and DP-cache eviction logic are all sound. Good test coverage for the new PromScrape parser and the has_absolute_token_data discriminator. One minor nit on the Prometheus parser's timestamp robustness — not a current bug but worth hardening.
e20b125 to
8f89a1f
Compare
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
8f89a1f to
33570e2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/worker/manager.rs (1)
952-971: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
successful/failedcounts now conflate fetch outcome with token-data availability.
successful/failedare derived purely froml.load >= 0(line 970-971). Since ratio-only vLLM/SGLang/metricsresponses now legitimately reportload: -1(via the newhas_absolute_token_data()gate) even thoughfetch_http_loadsucceeded and returned real data, these workers will now be counted asfailedinWorkerLoadsResult. Any dashboard/alerting or CLI output built onsuccessful/failedwill misreport healthy vLLM/SGLang fleets as having fetch failures.Consider deriving
successful/failedfrom whetherdetailsisSome/None(actual fetch outcome) rather than fromload >= 0(token-data availability), since these are now two independent signals.💡 Proposed fix
let loads = future::join_all(futures).await; - let successful = loads.iter().filter(|l| l.load >= 0).count(); - let failed = loads.iter().filter(|l| l.load < 0).count(); + let successful = loads.iter().filter(|l| l.details.is_some()).count(); + let failed = loads.iter().filter(|l| l.details.is_none()).count();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/worker/manager.rs` around lines 952 - 971, The `successful` and `failed` counters in `WorkerLoadsResult` are currently based on `l.load >= 0`, which mixes fetch success with whether absolute token data was present. Update the aggregation near the `join_all` result in `manager.rs` so it counts success/failure from the actual fetch outcome (`details` present vs absent in `WorkerLoadInfo`), while keeping `load` as the separate token-data indicator returned by `fetch_http_load`/`has_absolute_token_data()`. This will keep `successful`/`failed` accurate for ratio-only vLLM/SGLang responses that legitimately set `load` to `-1`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@model_gateway/src/worker/manager.rs`:
- Around line 952-971: The `successful` and `failed` counters in
`WorkerLoadsResult` are currently based on `l.load >= 0`, which mixes fetch
success with whether absolute token data was present. Update the aggregation
near the `join_all` result in `manager.rs` so it counts success/failure from the
actual fetch outcome (`details` present vs absent in `WorkerLoadInfo`), while
keeping `load` as the separate token-data indicator returned by
`fetch_http_load`/`has_absolute_token_data()`. This will keep
`successful`/`failed` accurate for ratio-only vLLM/SGLang responses that
legitimately set `load` to `-1`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d49ce8de-8cf3-4909-98e3-e9362daa77cc
📒 Files selected for processing (3)
crates/protocols/src/worker.rsmodel_gateway/src/worker/manager.rsmodel_gateway/src/worker/monitor.rs
![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: cf918720ac
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Some(Self::single_rank(SchedulerLoadSnapshot { | ||
| num_running_reqs: m.sum("vllm:num_requests_running") as i32, | ||
| num_waiting_reqs: m.sum("vllm:num_requests_waiting") as i32, | ||
| token_usage: m.mean(kv_usage), |
There was a problem hiding this comment.
For vLLM 0.x multi-process/multi-GPU servers, vllm:gpu_cache_usage_perc is exported with Prometheus multiprocess_mode="sum" (see the vLLM 0.7.3 metrics docs: https://docs.vllm.ai/en/v0.7.3/serving/metrics.html), so this value can exceed 1.0 when multiple engine processes report the same ratio. Publishing m.mean(kv_usage) directly as the 0–1 token_usage signal makes load-aware policies treat multi-GPU workers as overloaded at normal utilization and skews comparisons against single-GPU workers; please normalize the summed gauge by the engine/GPU count or otherwise derive an average before updating routing state.
Useful? React with 👍 / 👎.
2 participants
Problem
/get_loadsreportedload: -1for every HTTP worker, and load-aware routing (power_of_two / cache_aware) had no signal to act on.WorkerMonitor::fetch_http_loadunconditionally calledGET /v1/loads?include=…. Verified against the dev ORDcross-region-routerfleet, stock vLLM v0.7.3 and SGLang images 404 that path — it's an SGLang-custom/mock endpoint. So every fetch failed and load fell back to-1.Change
Dispatch
fetch_http_loadonruntime_typeand query the endpoint each engine actually serves:GET /metrics→vllm:gpu_cache_usage_perc→token_usage,num_requests_running/waitingGET /v1/loadsfirst (custom builds), fall back toGET /metrics(sglang:token_usage,num_running_reqs,num_queue_reqs, …)GET /v1/loads(mock worker, custom engines)A small
PromScrapeparser reads the flat Prometheus gauge lines (sumfor counts,meanfor ratios). Both metric fetchers require the KV-usage gauge and returnNoneotherwise, sotoken_usageis never a fake0.0.Ratio vs. absolute tokens
vLLM/SGLang
/metricsexpose the KV-usage ratio, not an absolute used-token count. Metric-derived snapshots therefore carrytoken_usagebut leavenum_used_tokens/max_total_num_tokensat0. NewWorkerLoadResponse::has_absolute_token_data()(any rank withmax_total_num_tokens > 0) gates the two consumers that assume absolute tokens:/get_loadsscalar reports-1(unavailable) instead of a misleading0; the ratio is still available indetails.loads[].token_usage./v1/loadsto ratio-only are evicted so stale per-rank loads can't keep drivingMinimumTokensPolicy.power_of_two/cache_awareare unaffected — they readtoken_usagefrom the watch-channel group loads.Testing
cargo test -p smg --lib worker::monitor→ 17 pass (6 new: parser, engine mappings, absolute-token discriminator).cargo clippy -p smg -p openai-protocol --lib -- -D warnings→ clean.cross-region-router.Notes / follow-ups
vllm:cache_config_infolabels (num_gpu_blocks × block_size × ratio) — deliberately not included here to keep the parser simple and avoid version-fragile label parsing. Could populate the/get_loadsabsolute scalar for vLLM in a follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes