fix(metrics): wire /metrics to the engine the server actually serves from (#277) - #297
Conversation
bb1705f to
e8a7624
Compare
`/metrics` is dead on the production server. The synchronous `LLMEngine` folds `IterationStats` into the logger 1:1 with `llm_engine.py:306-332`; `AsyncLLM` — which is what the shipped server actually serves from — does not. A real deployment therefore scrapes a well-formed `vllm:*` catalog whose series never move, which reads as "idle" rather than as "missing". Spec only, committed before any implementation: scope (the AsyncLLM step site, the batch-queue stat stamp, the recorder mutex, the server attach), the pinned upstream anchors at `555967922`, the port map, the RED-first async gate that asserts the same invariants `test_llm_engine.cpp` case 6 asserts, and the explicit exclusion of the config-gated families (spec-decode / kv-connector / mm / LoRA), which are the sibling residual on the same row. Also links #277 from the roadmap open-issue table and records the claim. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: ClaudeCode:claude-opus-5 [ClaudeCode]
…from (#277) `/metrics` was dead on the production server. The synchronous `LLMEngine` folds `IterationStats` into the logger 1:1 with `llm_engine.py:306-332`; the shipped server does not use it — every route is served from `AsyncLLM`, whose output handler folded nothing into any logger. So a real deployment scraped a well-formed `vllm:*` catalog whose series never moved, which is worse than an absent endpoint: a scraper reads it as "idle". Three defects, each with its own gate: 1. NO FOLD. `AsyncLLM` gains the attach point `LLMEngine` already has, mirroring upstream's `logger_ref[0]` (`async_llm.py:648-652`) as an atomic pointer so an attach is visible to the already-running handler thread. `RunOutputHandler` now builds one `IterationStats` per step under a non-null logger (`:664-665`), threads it through `process_outputs` (`:676-678`), and folds it plus `scheduler_stats` into `Record()` (`:697-702`) — deliberately OUTSIDE `output_processor_mutex_`, so the logger's mutex is a leaf lock. 2. NO `scheduler_stats`/`timestamp` ON THE DEPTH-2 PATH. `EngineCore::step_with_batch_queue` stamped neither, while `step()` stamped both. Upstream stamps them in the path BOTH step functions share — `scheduler_stats` in `Scheduler.update_from_output` (`scheduler.py:1938-1951`) and `timestamp` in `EngineCoreOutputs.__post_init__` (`engine/__init__.py:249-251`) — which is exactly why upstream's `step_with_batch_queue` stamps nothing extra. Unstamped, the async-scheduling serving path (what `LoadedEngine` resolves to whenever the runner supports it) publishes all-zero gauges and reports TTFT as `-arrival_time`. The former `VT_TTFT_DUMP`-only timestamp stamp is subsumed; the diagnostic reads the same value it always did. 3. NOT THREAD-SAFE. `PromRegistry` is documented as not thread-safe, and upstream never needs it to be — `prometheus_client` mutates under the GIL. Once the output-handler thread records while an HTTP worker scrapes, one recorder and N readers genuinely overlap, so `Record`, `Expose` and `SetCacheConfigInfo` take one mutex. `server_main.cpp` attaches the single logger it already builds to both frontends. Two stale residual comments claiming AsyncLLM has no live logger are corrected in `api_server.{h,cpp}`. Additive and opt-in throughout: with no logger attached the handler takes the same no-stats `process_outputs` call as before, which the token-stream identity case asserts byte-for-byte. RED-first, verbatim in `.agents/specs/async-metrics.md`: * `test_llm_engine -tc="async_llm*"` — 19 of 63 assertions read 0 on a stack that had just generated 8 tokens (counters, request_success absent, every TTFT/ITL/e2e/TPOT count, every timing `_sum`). * `test_async_llm -tc="*depth-2*"` — the running-gauge poll times out (the gauge never leaves 0 with a request in flight for 20 s), and the TTFT/e2e `_sum` assertions fail at 0. A count-only assertion would have passed the timestamp defect, so the sign of the `_sum` is the discriminator. GREEN: `test_llm_engine` 15/291, `test_async_llm` 10/325, `test_prometheus_metrics` 4/81, full CPU `ctest -j 6` **366/366 passed, 0 failed** (780.19 s) — none of the four known-flaky binaries failed, so no serial re-run was owed. Records: `SERVE-METRICS` + `SERVE-RESPONSE-METRICS` engine-matrix and feature-matrix rows, the `ROAD-V1-C8` residual in the roadmap and in `roadmap-v1-completion.md` (punch-list item 7 closed), `NOW.md`, and the `docs/STATUS.md` claim that metrics "lack live async backing", which had become false. The STATUS edit is a net deletion (-30) and the ratchet is re-pinned byte-tight downward in the same change. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: ClaudeCode:claude-opus-5 [ClaudeCode]
e8a7624 to
9c55e3d
Compare
CI note:
|
| main revision | DSR sites | |
|---|---|---|
abdc3a26 |
32 | baseline |
848d4a87 |
32 | |
84fc736d |
32 | |
7d45913a |
33 | ← regression enters |
e17e8f8a |
33 | |
24306364 |
33 | this PR's base |
7d45913a is the landing of #213. Diffing the checker's own site list across
84fc736d..7d45913a yields exactly one new entry —
src/vllm/model_executor/models/qwen3_5.cpp: [vt_ifdef] #ifdef VT_MARLIN_NVFP4 —
a device-specific branch in the shared layer with no DSR-ALLOW and no
deliberate baseline change.
Filed as #302 rather than fixed here: only the #213 author knows whether that
#ifdef is genuinely the platform leg (→ DSR-ALLOW with a reason) or should be
asking the Platform capability table instead, and the checker explicitly forbids
raising the baseline to make it pass.
This branch adds zero DSR sites: none of the 33 are in any file it touches.
The one local test failure, also pre-existing
test_async_llm intermittently fails async_llm test_abort and test_multi_abort leave other requests healthy (:325) — a case this PR does not touch. Measured
paired, same box, 40 concurrent copies × 3 rounds, alternating builds in one
worktree: main 31/120 (25.8%) vs main + this PR 31/120 (25.8%).
Identical, so it is neither caused nor worsened here. Filed as #294. This
PR's own new cases are 8/8 under the same contention, and serially on an idle box
test_async_llm is 10/10, test_openai_conformance 23/23,
test_openai_api_server 50/50.
Closes #277. Row
SERVE-METRICS(+SERVE-RESPONSE-METRICS), punch-list item 7(
ROAD-V1-C8). Spec:.agents/specs/async-metrics.md,committed alone before any implementation.
The bug
/metricswas dead on the production server. The synchronousLLMEnginefoldsIterationStatsinto the logger 1:1 withllm_engine.py:306-332. The shippedserver does not use it — every route is served from
AsyncLLM, whose outputhandler folded nothing into any logger. A real deployment therefore scraped a
well-formed
vllm:*catalog whose series never moved, which is worse than anabsent endpoint: a scraper reads a motionless, correctly-named catalog as
"the server is idle".
Three defects, three discriminators
1. No fold.
AsyncLLMgains the attach pointLLMEnginealready had,mirroring upstream's
logger_ref[0](async_llm.py:648-652) as an atomicpointer so an attach is visible to the already-running handler thread.
RunOutputHandlerbuilds oneIterationStatsper step under a non-null logger(
:664-665), threads it throughprocess_outputs(:676-678), and folds itplus
scheduler_statsintoRecord()(:697-702) — deliberately outsideoutput_processor_mutex_, which keeps the logger's mutex a leaf lock.2. No
scheduler_stats/timestampon the depth-2 path.EngineCore::step_with_batch_queuestamped neither, whilestep()stampedboth. Upstream stamps them in the path both step functions share —
scheduler_statsinScheduler.update_from_output(scheduler.py:1938-1951),timestampinEngineCoreOutputs.__post_init__(engine/__init__.py:249-251)— which is precisely why upstream's
step_with_batch_queuestamps nothingextra. Unstamped, the async-scheduling serving path (what
LoadedEngineresolves to whenever the runner supports it) publishes all-zero gauges and
reports TTFT as
-arrival_time. The oldVT_TTFT_DUMP-only timestamp stamp issubsumed; the diagnostic reads the same value it always did.
3. Not thread-safe.
PromRegistryis documented as not thread-safe, andupstream never needs it to be —
prometheus_clientmutates under the GIL. Oncethe output-handler thread records while an HTTP worker scrapes, one recorder
and N readers genuinely overlap, so
Record/Expose/SetCacheConfigInfotake one mutex.
server_main.cppattaches the single logger it already builds to bothfrontends. Two stale comments in
api_server.{h,cpp}claiming AsyncLLM has nolive logger are corrected.
Additive and opt-in throughout: with no logger attached the handler takes the
same no-stats
process_outputscall as before, asserted byte-for-byte by atoken-stream identity case.
RED first
Verbatim in the spec's Evidence section.
The gauge assertion had to be a poll against a deliberately slowed runner:
every gauge is legitimately 0 once the batch drains, so an end-of-run scrape
could not see defect 2. And a
_count-only assertion would have passed defect3 — only the sign of the
_sumdistinguishes-arrival_timefrom a real TTFT.GREEN
test_llm_enginetest_async_llmtest_prometheus_metricsctest --test-dir build-cpu -j 6None of the four known-flaky binaries failed in that run. From-scratch CPU build
under
-Werror: 1131/1131 clean.Two failures on the re-run, and what they actually were
One was not a failure at all. A post-rebase re-run appeared to report
test_openai_conformancefailing 13 of 23 cases. The log said otherwise beforeany test was re-run: the assertions cited
/home/mudler/_git/vllm.cpp-lora-w2/…/test_conformance.cpp— a differentworktree, while
stringson the binary ctest actually ran has 152 referencesto this worktree and zero to that one; the file was 48,261 NUL bytes out
of 60,238 (a sparse hole, i.e. two writers at independent offsets); and its
own progress lines counted
1/368while the summary saidout of 369. Aconcurrent session was running
ctestinvllm.cpp-lora-w2(pid 218469,confirmed via
/proc/<pid>/cwd) and its output landed in this session's log —while our own run was still executing. Gate logs now go to
.gatelogs/insidethe worktree, and a failing log gets validated (cited paths vs the binary, NUL
runs, self-consistent counts) before it is believed.
One was real, pre-existing, and now has its own issue. On an uncontaminated
re-run
test_async_llmgenuinely failed — inasync_llm test_abort and test_multi_abort leave other requests healthy(:325,Drain(engine, reused) == 3observing 4), a case this PR does not touch. Thatbinary is squarely in this PR's blast radius, so it was measured rather than
dismissed: 40 concurrent copies of the single case, three rounds each,
alternating the two builds in one worktree on the same box.
main@848d4a87main+ this PRIdentical rate — pre-existing on
main, neither caused nor worsened here. Theunderlying defect is real (
AsyncLLM::abortqueues the core abortasynchronously, while the case's precondition reads only frontend state, so a
frame from a reused id's previous incarnation can reach the new collector) and
is filed as #294 rather than silently fixed in this PR. Serially on an idle
box both binaries are green:
test_async_llm10/10,test_openai_conformance23/23.
Recipe:
cmake -S . -B build-cpu -G Ninja -DCMAKE_BUILD_TYPE=Release \ -DVLLM_CPP_CUDA=OFF -DVLLM_CPP_VULKAN=OFF -DVLLM_CPP_METAL=OFF cmake --build build-cpu -j 18 ctest --test-dir build-cpu -j 6 --output-on-failureScope
Out, and untouched: the config-gated metric families (spec-decode,
kv-connector, multimodal cache, LoRA), which are the sibling residual on the
same row;
update_scheduler_stats(LoRA-only upstream); theVLLM_V1_OUTPUT_PROC_CHUNK_SIZEchunking loop, which our synchronousprocess_outputsdoes not have on either engine; DP/multi-engine aggregation.Records
SERVE-METRICSandSERVE-RESPONSE-METRICSin the engine and feature matrices,the
ROAD-V1-C8residual inroadmap_v1.mdandroadmap-v1-completion.md(punch-list item 7 closed), the coordination claim,
NOW.md(net zero againstits 6000-char budget), and
docs/STATUS.md, whose claim that metrics "lack liveasync backing" had become false. The STATUS edit is a net deletion (-30) and
the ratchet is re-pinned byte-tight downward in the same change, which
tests/scripts/test_check_public_doc_tables.py::test_the_rebased_character_ratchet_is_byte_tightalready enforces.
docs/USAGE.mdanddocs/FEATURES.mdupdated for theserving-surface change.
No performance axis is claimed: the wiring is inert without a logger, and with
one it costs one
IterationStatsper step on the output-handler thread — thesame cost the synchronous engine already paid.
🤖 Generated with Claude Code