fix(daemon): decouple vLLM Prometheus scraping from enable_docker

[michaelroy-amd]

Summary

• VllmPrometheusCollector (the vLLM /metrics HTTP scraper) was only constructed when opts.enable_docker && !opts.disable_vllm_metrics, even though the scraper has zero Docker dependency — it's a plain HTTP GET against a vLLM instance's /metrics endpoint.
• apps/rocm/src/dash.rs::maybe_spawn_embedded_daemon always calls runner_options(config, paths, false), hardcoding enable_docker = false. Combined with the gate above, this made the vLLM Prometheus scraper permanently dead for the embedded daemon — the common no-Docker / managed-vLLM path.
• Fix: gate vllm construction in run_loop on !opts.disable_vllm_metrics alone. enable_docker continues to gate only DockerDiscovery, which is the only genuinely Docker-dependent piece.

Root cause

Managed (native) vLLM instances are already discovered independently of Docker via services_dir, so the scraper never needed enable_docker as a precondition. The original gate conflated "Docker discovery is enabled" with "scrape metrics from any known vLLM instance," silently disabling metrics for every embedded-daemon deployment.

Relates to EAI-7359

Test Plan

• cargo build clean
• cargo clippy --all-targets --all-features -- -D warnings clean
• cargo test -p rocm-dash-daemon runner:: — 11 passed, including new vllm_metrics_enabled_is_independent_of_docker regression test
• cargo test -p rocm dash:: -- --test-threads=1 — 8 passed, including new runner_options_keeps_vllm_metrics_enabled_without_docker regression test

[rominf]

LGTM. The gating change is correct: enable_docker && !disable_vllm_metrics → !disable_vllm_metrics (not inverted), so the embedded-daemon path — which always passes enable_docker = false — now constructs the vLLM collector as intended.

No wasted per-tick work or error spam when nothing is configured: the scrape block is still guarded by !instances.is_empty() and parallel_scrape early-returns on empty targets, so a no-Docker host with an empty services_dir issues zero scrape requests; constructing the collector when idle is just a cheap reqwest::Client build, and a 1.5s timeout is present. enable_docker now gates only DockerDiscovery; grep confirms nothing else relied on the old coupling, and the empty-instances guard (not enable_docker) is the real invariant preventing pointless scraping. Tests match the shipped logic.

Approving.

[volen-silo]

🔴 Automated review · pr-review-watcher · 8018fe4

Summary

• Change type/scope: Bug fix in the dashboard telemetry daemon — decouples the vLLM Prometheus scraper from enable_docker (2 files, +81/-3, 2 commits).
• Overall assessment: Approve. The fix is correct, well-scoped, and its central premise holds up under source verification. Build and both test suites pass locally exactly as the PR claims.
• Blocking findings: 0. Non-blocking: 2 (both about one weak test). Tradeoffs: 1.

What the fix does and why it's correct

run_loop previously constructed VllmPrometheusCollector only when opts.enable_docker && !opts.disable_vllm_metrics. The one production caller path — maybe_spawn_embedded_daemon → runner_options(config, paths, false) — hardcodes enable_docker=false, so the scraper was permanently dead for the embedded daemon. The fix gates construction on !opts.disable_vllm_metrics alone.

Verified in context:

• Scrape targets (runner.rs:389-396) are built from runner.state.instances, which is populated by both DockerDiscovery (runner.rs:249-295) and the services_dir managed-service registry (runner.rs:345-379) — the latter independent of Docker. So once the collector is constructed, it genuinely scrapes managed/native instances. The PR's core claim is sound.
• enable_docker still gates only DockerDiscovery::detect(...) (runner.rs:172), unchanged — no regression for a (hypothetical) Docker-enabled path.
• The doc-comment claim that disable_vllm_metrics is an internal-only seam is accurate: workspace-wide grep shows it is written to false at exactly two sites (runner.rs:87 default, dash.rs:52) and has no CLI flag, config field (DaemonConfig has none), or env var. enable_docker likewise is never set true anywhere in production code — reinforcing that the embedded daemon is the only runner path today.

Verification performed

• cargo test -p rocm-dash-daemon runner:: → 11 passed (incl. new vllm_metrics_enabled_is_independent_of_docker).
• cargo test -p rocm dash:: -- --test-threads=1 → 8 passed (incl. new runner_options_keeps_vllm_metrics_enabled_without_docker).
• cargo clippy -p rocm-dash-daemon --all-targets → clean (pedantic + nursery enabled workspace-wide).
• Blast radius mapped: all callers of runner_options / run_loop and all references to enable_docker / disable_vllm_metrics reviewed; nothing outside the diff relied on the old coupling.

Non-blocking suggestions

1. apps/rocm/src/dash.rs:468-477 — the new dash.rs test is a near-tautology. runner_options unconditionally sets disable_vllm_metrics: false (dash.rs:52), with zero data dependency on enable_docker or config. The test just asserts that hardcoded literal back at itself; it can only fail if someone directly edits line 52. It does not guard the actual regression, which lived in run_loop's construction gate in runner.rs and is already covered there by vllm_metrics_enabled_is_independent_of_docker. Harmless, but low-value dressed up as a regression test. Consider dropping it in favor of the explanatory comment alone, or repurposing it to assert what dash.rs actually owns — e.g. that maybe_spawn_embedded_daemon's hardcoded false argument doesn't drift.

2. apps/rocm/src/dash.rs:460-467 — doc comment overclaims what that test proves. Framing this assertion as the "EAI-7359 regression" test implies it would have caught the bug pre-fix. It would not have: the bug was run_loop's conditional collector construction, not any field in runner_options. The background/context in the comment is accurate and useful ("why"); only the "this test is the regression guard" implication is misleading. Softening the framing would remove the comment-rot-by-association.

Tradeoffs (deliberate choice, noted for visibility)

• Two-test split across crates. The real, well-targeted coverage is entirely in runner.rs's vllm_metrics_enabled_is_independent_of_docker (exercises all four enable_docker×disable_vllm_metrics combinations against the extracted predicate). The dash.rs test is redundant on top of it. Documenting the constraint at the runner_options call site is reasonable; codifying it as a second "regression" test is what overstates its value (see suggestion 1). Author's call.
• Extracted const fn vllm_metrics_enabled vs inlining !opts.disable_vllm_metrics. Reasonable as a named seam that the unit test can target without async run_loop plumbing; not dead-code-adjacent (used at runner.rs:138 and in the test). Fine either way.

Coverage gap (informational, not a defect)

No test exercises maybe_spawn_embedded_daemon → run_loop end-to-end to assert the collector is actually constructed and mutates instance state (gen_tps, kv_cache_usage_pct, …) when enable_docker=false with a managed instance present. That needs a fake vLLM /metrics server and is reasonably out of scope for a focused bugfix; the two unit tests plus the existing end_to_end.rs (which runs run_loop with default enable_docker=false opts) are a pragmatic substitute. Not blocking; worth a follow-up ticket if this path is regression-sensitive.

Positive signals

• Root cause is correctly identified and the minimal fix targets it precisely, rather than papering over symptoms.
• Excellent commit hygiene: two atomic commits, conventional-commit subjects, bodies that explain the why, DCO sign-offs, no AI footers. PR description accurately matches the diff.
• Good instinct in the follow-up commit to soften disable_vllm_metrics docs to reflect that it is not a shipped off-switch — avoids misleading future readers about a live flag.

Deployment notes

None. No schema/migration/config changes; no breaking changes; no new dependencies. Behavior change is purely additive: vLLM metrics now scrape on the embedded (no-Docker) daemon path where they were previously silently absent.