Skip to content

[Frontend][Metrics] Unregister only PrometheusStatLogger-owned collectors - #10

Open
Ronald1995 wants to merge 1 commit into
codex/rl-weight-operation-metricsfrom
pr-55781-ownership-fix
Open

Ronald1995 wants to merge 1 commit into
codex/rl-weight-operation-metricsfrom
pr-55781-ownership-fix

Conversation

@Ronald1995

Copy link
Copy Markdown
Owner

Why

PrometheusStatLogger.__init__ calls unregister_vllm_metrics(), which removes every vllm:* collector from the default registry. That also deletes frontend-owned metrics whose lifetime has nothing to do with the engine:

When a logger is rebuilt (e.g. Elastic EP), those collectors are unregistered from REGISTRY, so their series stop being exported from /metrics for the rest of the process lifetime. Measured on this branch:

SINGLE PROCESS (default `vllm serve`)
  before unregister, exposed: True
  after  unregister, exposed: False      <-- series gone from /metrics
  metric object still usable: 6.0        <-- object survives, export does not

MULTIPROCESS (api_server_count > 1)
  before unregister, exposed: True
  after  unregister, exposed: True       <-- survives via the mmap file

What

Track ownership per collector object instead of by metric name, and release only what the previous logger registered:

  • PrometheusStatLoggerRegistry is the registration hook that records ownership.
  • unregister_prometheus_stat_logger_metrics() unregisters just those collectors. It is still called before a new logger is constructed, because the logger's metric names are fixed and the previous instance must release them first.
  • Connector / speculative-decoding / perf metric factories already receive their metric classes from the logger, so they inherit the same ownership. Their signatures gain an optional injection point (metric_types / counter_cls) and keep the existing class-attribute defaults for direct use.

unregister_vllm_metrics() remains available for test and global cleanup. RayPrometheusStatLogger._unregister_vllm_metrics was a no-op hook that lost its only caller when the call site was removed, and is dropped.

How this is different from vllm-project#55781

vllm-project#55781 both adds the weight-operation metrics and fixes this ownership problem, which mixes an RL feature with a logger-lifetime bugfix. This PR extracts only the bugfix so it can be reviewed on its own. It contains no RL/metrics-feature changes, and vllm-project#55781 rebases cleanly on top of it (the two changes touch disjoint files).

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 and the repo's requirements/common.txt installed, against the real vllm import chain (no stubs):

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

That run covers this PR together with the two RL metric test files (13 tests total). tests/v1/metrics/test_prometheus_logger_ownership.py itself asserts:

  • an independently owned vllm:* metric survives a logger rebuild and keeps its accumulated value,
  • late cache_config_info registration is owned too,
  • engine-index expansion after a rebuild,
  • the real EC-connector metric factory path, including that the previous logger's connector counter is no longer registered.

Rust is unaffected by this PR. As a side check, cargo test -p vllm-metrics was run on the same cluster:

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

AI assistance

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

`PrometheusStatLogger.__init__` called `unregister_vllm_metrics()`, which
removed *every* `vllm:*` collector from the default registry. That also wiped
frontend-owned metrics, whose lifetime does not depend on the engine: the RL
weight-operation metrics, the parser metrics and the WebSocket connection
metrics all live in `REGISTRY` for the whole process.

Track ownership per collector object instead, and release only what the previous
logger registered. `PrometheusStatLoggerRegistry` is the registration hook that
records ownership; `unregister_prometheus_stat_logger_metrics()` is still called
before a new logger is constructed, because metric names are fixed and the
previous instance has to release them first.

Connector, speculative-decoding and perf metric factories already receive their
metric classes from the logger, so they now inherit that ownership. Their
signatures gain an optional injection point (`metric_types` / `counter_cls`) and
keep the existing class-attribute defaults for direct use.

`unregister_vllm_metrics()` stays for test and global cleanup, and the
`RayPrometheusStatLogger._unregister_vllm_metrics` no-op hook, dead since the
call site was removed, is dropped.

Tests: tests/v1/metrics/test_prometheus_logger_ownership.py covers an
independently owned metric surviving a logger rebuild, late `cache_config_info`
registration, engine-index expansion and the real EC connector factory path.

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 the PR 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

tests/v1/metrics/test_prometheus_logger_ownership.py (owned by this PR) asserts that an independently owned vllm:* metric survives a logger rebuild with its value intact, that late cache_config_info registration and engine-index expansion are handled, and that the previous logger's EC-connector counter is unregistered through the real connector factory path.

Regression measurement for the bug this PR fixes

SINGLE PROCESS (default `vllm serve`)
  before unregister, exposed: True
  after  unregister, exposed: False     <-- series disappears from /metrics
MULTIPROCESS (api_server_count > 1)
  before unregister, exposed: True
  after  unregister, exposed: True      <-- survives via the mmap file

So the impact is specific to single-process deployments, which is the default.

Rust side

This PR does not touch Rust. As a side check on the same cluster, cargo test -p vllm-metrics passes (3 passed, 0 failed).

Not covered by this run

  • cargo test -p vllm-server 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), and the account has no sudo/apt. This PR does not change Rust, so it is not expected to matter here, but the server crate is unverified on this image in general.
  • tests/v1/metrics/test_histogram_buckets.py was not run (it needs a full GPU environment); it exercises PrometheusStatLogger construction, so it is worth including in CI.

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

@Ronald1995

Copy link
Copy Markdown
Owner Author

Heads-up: the base branch was reverted

codex/rl-weight-operation-metrics just got commit 9368e3a4b2, which removes all of the logger-ownership code from that branch (the RL PR now touches only its 7 weight-metric files). That means the ownership files this PR is based on are now back at their main state.

This PR's diff is unchanged and still correct: 2 files / +12 / -14

vllm/v1/metrics/prometheus.py
vllm/v1/metrics/ray_wrappers.py

It remains based on codex/rl-weight-operation-metrics, so the diff is exactly these two files. Once the RL PR is rebased/retargeted onto main, this PR can be rebased onto main too — no content change needed.

One thing worth flagging for review: this PR is the smaller half of the ownership fix. The rest of the ownership plumbing (_use_prometheus_registry, the connector / speculative-decoding / perf metric_types / counter_cls injection points, and the ownership test covering cache_config_info, engine-index expansion and the EC-connector factory path) currently lives in the RL PR's branch history and will disappear from main when that branch is retargeted — i.e. when this PR is all that remains, the logger will register through PrometheusStatLoggerRegistry, but the connector metric factories will go back to registering through their own class attributes, and the ownership test file will not exist on main.

Where you want that split is your call. Two options:

  1. This PR takes the whole ownership change (the previous revision of the branch had exactly that, at commit dec23429b1), so main keeps the complete fix and the test; or
  2. This PR stays at the two files it has now, and the connector/perf/spec_decode plumbing plus the ownership test get added here as a follow-up commit before merge.

If you prefer option 1 or 2, say which and I will push it — I did not want to expand this PR's scope without asking, since vllm-project#55781 was just narrowed on purpose.

@Ronald1995
Ronald1995 force-pushed the codex/rl-weight-operation-metrics branch from 5f9d442 to bc7991b Compare September 24, 2026 15:12
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