Skip to content

[Frontend][RL] Instrument HTTP weight operations in both frontends - #11

Merged
Ronald1995 merged 1 commit into
codex/rl-weight-operation-metricsfrom
pr-55781-weight-metrics
Sep 22, 2026
Merged

Ronald1995 merged 1 commit into
codex/rl-weight-operation-metricsfrom
pr-55781-weight-metrics

Conversation

@Ronald1995

Copy link
Copy Markdown
Owner

Purpose

Add bounded-label telemetry around dispatched frontend HTTP weight operations: init, start, start_draft, update, finish and set_version. Counters report success/error (including cancellation), histograms measure one dispatched frontend operation, and an in-flight gauge tracks concurrent operations without one request clearing another. Input validation stays outside the recorder.

Both frontends expose the same names and labels:

  • Python RL HTTP routes use an independently owned WeightOperationMetrics, created on first use on get_prometheus_registry().
  • Rust HTTP weight-transfer routes use a drop-safe RAII recorder.

Rust gRPC control calls are intentionally not counted as HTTP operations.

Changes since the previous revision of this PR

  1. finish and the weight-version handshake are now separate operations. Previously the Rust recorder spanned both finish_weight_update() and set_weight_version(), so a version failure was reported as a failed finish with a duration that included the version round-trip, while the Python route counted them as a single finish. That made the same metric name mean different things in the two frontends. Now finish records success as soon as the engine finishes, and the version handshake is counted as set_version on both frontends. The Python /finish_weight_update route therefore calls finish_weight_update() and update_weight_version() separately, matching the Rust flow.
  2. /update_weight_version is instrumented on both frontends (it was previously not counted at all on either side, even though the Rust route existed).
  3. The frontend recorder binds to the served registry. The module-level singleton was created at import time on the default REGISTRY, while /metrics serves get_prometheus_registry(). That only worked by accident, because prometheus_client picks the mmap-backed value class when PROMETHEUS_MULTIPROC_DIR is already set at creation time. The recorder is now created on first use on get_prometheus_registry(), with a regression test that would fail if it were bound at import time.
  4. Test gaps closed: rejected input now asserts that the in-flight gauge is never touched (Python and Rust), the Rust lifecycle test asserts the finish / set_version counts, and a new Rust test covers "finish succeeds, version fails" (500 response, finish = success, set_version = error).
  5. Documentation of the metrics states that a cancelled operation is recorded as status="error".

Relationship to vllm-project#55781's ownership fix

This PR is the RL product-observability part only: it no longer modifies vllm/v1/metrics/{prometheus,loggers,ray_wrappers}.py or the connector metric factories. Those ownership changes are split into a separate PR (pr-55781-ownership-fix), because the same bug also silently drops the parser, WebSocket and HTTP instrumentator metrics. The two PRs touch disjoint files and can land in either order; the context in this PR was rewritten from origin/main instead of touching the default registry.

Why this is not duplicating an existing PR

Test commands and results

Executed on the H20 cluster (vllm-h20-head -> Slurm h20 -> vllm-h20-02, H20-3e), Python 3.12.14 venv with torch 2.14.0+cpu plus the repo's requirements/common.txt, against the real vllm import chain (no stubs):

cd ~/pr55781/wt-weight
~/pr55781/.venv/bin/python -m pytest -q \
  tests/entrypoints/unit_tests/test_rl_weight_operation_metrics.py \
  tests/entrypoints/unit_tests/test_rl_weight_operation_routes.py \
  tests/v1/metrics/test_prometheus_logger_ownership.py
13 passed, 1 warning in 0.83s

Covered: recorder success/error/cancellation, overlapping requests not clearing each other's gauge, registry binding on first use, recorder created once, route-level in-flight and success accounting, rejected input touching neither counter nor gauge, finish success when the version handshake fails, unversioned finish not recording set_version, and the /update_weight_version route.

ruff check and ruff format --check pass on all changed Python files, and python tools/pre_commit/check_test_tethering.py confirms the new test files are collected by Buildkite jobs.

Rust: not yet executed. rust/src/server/src/routes/tests.rs gains four assertion groups (lifecycle finish = 2 and set_version = 2, rejection leaving the gauge at zero, and a new finish-succeeds/version-fails 500 test). cargo test -p vllm-metrics passes on the cluster, including the new weight_operation_recorder_tracks_success_and_error test:

running 3 tests
test api_server::tests::weight_operation_recorder_tracks_success_and_error ... ok
test result: ok. 3 passed; 0 failed

but cargo test -p vllm-server --lib weight_transfer cannot be built in the available environment: the H20 compute-node image has no OpenSSL development headers or .pc files (Failed to find OpenSSL development headers, SERVER_RC=101), the account has no sudo/apt, and the CUDA devel container lacks libssl-dev too. Those route assertions still need a run on a machine with pkg-config libssl-dev before merge.

AI assistance

This change was prepared with AI assistance (code analysis, patch drafting, remote test execution). The submitting human has reviewed every changed line and ran the tests reported above.

Add bounded-label telemetry around dispatched frontend weight operations: init,
start, start_draft, update, finish and set_version. Counters report
success/error, histograms measure one dispatched operation, and an in-flight
gauge tracks concurrency without one request clearing another. Input validation
stays outside the recorder.

Both frontends expose the same names and labels: the Python RL HTTP routes use
an independently owned `WeightOperationMetrics` created on first use on
`get_prometheus_registry()`, and the Rust HTTP weight-transfer routes use a
drop-safe RAII recorder.

`finish` and the weight-version handshake are recorded as separate operations so
that a version failure is not reported as a failed finish; the Python route now
calls `finish_weight_update()` and `update_weight_version()` separately to match
the Rust frontend, and `/update_weight_version` is instrumented on both.

Tests: recorder outcome/overlap tests, route-level in-flight and rejected-input
tests, a registry-binding regression test, and Rust recorder plus HTTP
lifecycle and rejection metric assertions.

Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
@github-actions

Copy link
Copy Markdown

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

@Ronald1995

Copy link
Copy Markdown
Owner Author

Test evidence (H20 cluster)

Environment: vllm-h20-head -> Slurm h20 -> node vllm-h20-02 (H20-3e).
Python 3.12.14 venv on this branch, with torch 2.14.0+cpu and the repo's requirements/common.txt; the real vllm import chain is used, no stubs:

cd ~/pr55781/wt-weight
~/pr55781/.venv/bin/python -m pytest -q \
  tests/entrypoints/unit_tests/test_rl_weight_operation_metrics.py \
  tests/entrypoints/unit_tests/test_rl_weight_operation_routes.py \
  tests/v1/metrics/test_prometheus_logger_ownership.py
vllm imported: /home/ronald/pr55781/wt-weight/vllm/__init__.py
13 passed, 1 warning in 0.83s

What the 13 tests cover for this PR:

  • recorder outcomes for success / RuntimeError / asyncio.CancelledError (cancellation is recorded as status="error"), with the gauge released in every case;
  • overlapping requests not clearing each other's in-flight gauge (2 -> 1 -> 0);
  • the recorder is created on first use on get_prometheus_registry() — this test fails if the collector is created at import time on the default registry, which was the previous behaviour;
  • the recorder is created exactly once;
  • route-level accounting: gauge is 1 while the engine call is awaited, success counter and duration observed after it returns;
  • rejected input never enters the recorder (no counter, no gauge), and the fake engine raises if it is reached at all;
  • finish records success even when the version handshake raises, with set_version recorded as error and its gauge released;
  • finish without a version does not record set_version;
  • /update_weight_version is recorded as set_version.

Static checks on this branch: ruff check and ruff format --check pass on all changed Python files; python tools/pre_commit/check_test_tethering.py confirms the new test files are collected by Buildkite jobs.

Rust: recorder test passed, route tests not yet executed

cargo test -p vllm-metrics on the cluster (cargo/rustc 1.95.0 from rust-toolchain.toml):

running 3 tests
test api_server::tests::weight_operation_recorder_tracks_success_and_error ... ok
test scheduler::tests::log_stats_accumulator_drains_interval_data ... ok
test scheduler::tests::perf_counters_render_with_a_single_total_suffix ... ok
test result: ok. 3 passed; 0 failed

cargo test -p vllm-server --lib weight_transfer remains unverified, and it is the part of this PR that most needs a run:

Failed to find OpenSSL development headers.
SERVER_RC=101

Reason: the H20 compute-node image has no OpenSSL headers, .pc files or libssl.so, and the account has no sudo/apt; nvcr.io#nvidia/cuda:12.4.1-devel-ubuntu22.04 also lacks libssl-dev. The four assertion groups added in rust/src/server/src/routes/tests.rs (lifecycle finish = 2 / set_version = 2, rejected payloads leaving requests_in_flight at zero, and the new weight_transfer_routes_record_finish_success_when_version_fails 500-path test) will be confirmed on a machine with libssl-dev before this leaves draft.

Logs: /home/ronald/pr55781/logs/py3-3678.log, /home/ronald/pr55781/logs/rust-3677.log, /home/ronald/pr55781/logs/rustctr-3687.log.

@Ronald1995
Ronald1995 merged commit ae020f6 into codex/rl-weight-operation-metrics Sep 22, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant