refactor(api): serve /metrics through the shared metrics-endpoint crate#3527
Conversation
api-core served `/metrics` from its own copy of the metrics server -- a fork of the shared `metrics-endpoint` crate carrying two things the shared one lacked: mirroring metrics under an alternate name prefix, and stopping the listener on a cancellation token. Both now live in the shared crate, so this deletes the fork and points api-core at it.
- `run.rs` builds the shared `MetricsEndpointConfig` over the API's registry and serves it with `run_metrics_endpoint_with_cancellation`, turning the configured `alt_metric_prefix` into a `PrefixMigration { old: "carbide_", new }`.
- `crates/api-core/src/logging/metrics_endpoint.rs`, the forked server, is deleted along with its module declaration.
- `create_metrics` and the meter setup, with their custom histogram views, are untouched.
The shared endpoint serves the same registry, so `/metrics` stays byte-identical: the integration test's catalogue regenerates with no diff, and the `alt_metric_` mirroring test still passes through the shared crate.
Tests pass!
This supports NVIDIA#3180
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
Summary by CodeRabbit
Walkthroughapi-core replaces its local HTTP metrics server with the shared ChangesMetrics endpoint migration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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.
Inline comments:
In `@crates/api-core/src/run.rs`:
- Around line 169-172: Update the MetricsEndpointConfig construction in the run
flow to pass metrics.health_controller.clone() instead of None. Preserve the
shared health controller so health and readiness endpoints reflect api-core’s
actual state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b276d872-a9dd-4ecd-b729-fe4df2c8d014
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/api-core/Cargo.tomlcrates/api-core/src/logging/metrics_endpoint.rscrates/api-core/src/logging/mod.rscrates/api-core/src/run.rs
💤 Files with no reviewable changes (2)
- crates/api-core/src/logging/mod.rs
- crates/api-core/src/logging/metrics_endpoint.rs
| &metrics_endpoint::MetricsEndpointConfig { | ||
| address: metrics_address, | ||
| registry: metrics.registry, | ||
| health_controller: None, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== crates/api-core/src/run.rs (around target lines) ==\n'
nl -ba crates/api-core/src/run.rs | sed -n '145,195p'
printf '\n== search health_controller usages in api-core and metrics-endpoint ==\n'
rg -n "health_controller" crates/api-core crates/metrics-endpoint -S
printf '\n== crates/metrics-endpoint/src/lib.rs (relevant section) ==\n'
nl -ba crates/metrics-endpoint/src/lib.rs | sed -n '1,180p'Repository: NVIDIA/infra-controller
Length of output: 257
🏁 Script executed:
Inspect the relevant code:
- crates/api-core/src/run.rs around lines 145-195
- crates/metrics-endpoint/src/lib.rs around the health controller defaults
- any health_controller wiring in crates/api-core and crates/metrics-endpointRepository: NVIDIA/infra-controller
Length of output: 311
🏁 Script executed:
set -euo pipefail
printf '\n== crates/api-core/src/run.rs (excerpt) ==\n'
sed -n '145,195p' crates/api-core/src/run.rs
printf '\n== health_controller references ==\n'
rg -n "health_controller" crates/api-core crates/metrics-endpoint -S || true
printf '\n== crates/metrics-endpoint/src/lib.rs (excerpt) ==\n'
sed -n '1,180p' crates/metrics-endpoint/src/lib.rsRepository: NVIDIA/infra-controller
Length of output: 10159
🏁 Script executed:
set -euo pipefail
printf '\n== api-core health controller usage ==\n'
rg -n "set_ready|set_healthy|health_controller" crates/api-core -S
printf '\n== metrics-endpoint setup and tests around health defaults ==\n'
sed -n '200,240p' crates/metrics-endpoint/src/lib.rs
printf '\n---\n'
sed -n '400,540p' crates/metrics-endpoint/src/lib.rsRepository: NVIDIA/infra-controller
Length of output: 7165
Preserve the shared health controller. health_controller: None creates a new controller that starts healthy and ready, so /health and /ready can succeed before api-core is actually ready. Pass metrics.health_controller.clone() through here.
🤖 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 `@crates/api-core/src/run.rs` around lines 169 - 172, Update the
MetricsEndpointConfig construction in the run flow to pass
metrics.health_controller.clone() instead of None. Preserve the shared health
controller so health and readiness endpoints reflect api-core’s actual state.
|
Thanks @coderabbitai -- declining this one: there's no health controller to preserve. api-core's forked Wiring api-core's real readiness into the shared endpoint's |
|
Tip For best results, initiate chat on the files or code changes.
Agreed that wiring real readiness into the shared endpoint's ᕮ(´• ω •`)ᕮ ✏️ Learnings added
|
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
2 participants
api-core served
/metricsfrom its own copy of the metrics server -- a fork of the sharedmetrics-endpointcrate carrying two things the shared one lacked: mirroring metrics under an alternate name prefix, and stopping the listener on a cancellation token. Both now live in the shared crate (#3474), so this deletes the fork and points api-core at it.run.rsbuilds the sharedMetricsEndpointConfigover the API's registry and serves it withrun_metrics_endpoint_with_cancellation, turning the configuredalt_metric_prefixinto aPrefixMigration { old: "carbide_", new }.crates/api-core/src/logging/metrics_endpoint.rs, the forked server, is deleted along with its module declaration.create_metricsand the meter setup, with their custom histogram views, are untouched.The shared endpoint serves the same registry, so
/metricsstays byte-identical:test_integration's catalogue regenerates with no diff, and thealt_metric_mirroring test (test_metrics_integration) still passes through the shared crate.This supports #3180