perf(metrics-endpoint): encode the registry on a dedicated thread#3535
perf(metrics-endpoint): encode the registry on a dedicated thread#3535chet wants to merge 1 commit into
Conversation
|
@coderabbitai full_review, thanks! |
|
ʬʬ (｀・ω・´)ゞ ✅ Action performedFull review finished. |
Summary by CodeRabbit
WalkthroughThe metrics endpoint now queues ChangesMetrics endpoint request handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant service_fn
participant handle_metrics_request
participant encoder_thread
Client->>service_fn: GET /metrics
service_fn->>handle_metrics_request: await request
handle_metrics_request->>encoder_thread: enqueue oneshot reply channel
encoder_thread->>encoder_thread: gather and encode metrics
encoder_thread-->>handle_metrics_request: encoded bytes or EncodeError
handle_metrics_request-->>service_fn: HTTP 200, 503, or 500
service_fn-->>Client: metrics response
🚥 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
🧹 Nitpick comments (1)
crates/metrics-endpoint/src/lib.rs (1)
285-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table-driven regression coverage for the new response contract.
Cover successful
/metricsresponses byte-for-byte, including status and headers, plus the blocking-task failure path returning HTTP 500 while the listener remains usable. As per coding guidelines, Rust tests for observable results should use table-driven cases. As per path instructions, prioritize behavioral and missing-test coverage.🤖 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/metrics-endpoint/src/lib.rs` around lines 285 - 309, Add table-driven Rust regression tests for handle_metrics_request covering successful GET /metrics responses byte-for-byte, including status and headers, and the spawn_blocking failure path returning HTTP 500. Ensure the failure test verifies the listener or server remains usable afterward, reusing existing test helpers and observable response assertions.Sources: Coding guidelines, Path instructions
🤖 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/metrics-endpoint/src/lib.rs`:
- Around line 285-309: Bound concurrent metrics encoding in
handle_metrics_request by adding a shared semaphore around the spawn_blocking
work, or reuse a shared rendered snapshot to avoid duplicate encoding. Ensure
each /metrics request acquires the shared limit before encoding and returns HTTP
503 or 429 immediately when saturated, while preserving the existing successful
response and encoding-error handling.
---
Nitpick comments:
In `@crates/metrics-endpoint/src/lib.rs`:
- Around line 285-309: Add table-driven Rust regression tests for
handle_metrics_request covering successful GET /metrics responses byte-for-byte,
including status and headers, and the spawn_blocking failure path returning HTTP
500. Ensure the failure test verifies the listener or server remains usable
afterward, reusing existing test helpers and observable response assertions.
🪄 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: 8ba85c34-6036-4b7c-bbf7-a4b8207d73e8
📒 Files selected for processing (1)
crates/metrics-endpoint/src/lib.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/metrics-endpoint/src/lib.rs (1)
285-309: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPropagate encoder errors instead of using panic as the error channel.
encode_metricsstill callsunwrap()at Line 280, so an encoding failure is converted into aJoinErrorand the original Prometheus error is lost. Return aResultfromencode_metricsand handleOk(Err(error))separately from task panics or cancellation.As per coding guidelines, library code should use error propagation/conversions rather than panic-based handling.
🤖 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/metrics-endpoint/src/lib.rs` around lines 285 - 309, The metrics encoding flow should preserve encoder errors instead of panicking and hiding them inside JoinError. Update encode_metrics to return a Result and propagate the underlying Prometheus encoding error, then adjust handle_metrics_request to handle Ok(Err(error)) separately from task panics or cancellation while retaining distinct logging and HTTP failure handling.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@crates/metrics-endpoint/src/lib.rs`:
- Around line 285-309: The metrics encoding flow should preserve encoder errors
instead of panicking and hiding them inside JoinError. Update encode_metrics to
return a Result and propagate the underlying Prometheus encoding error, then
adjust handle_metrics_request to handle Ok(Err(error)) separately from task
panics or cancellation while retaining distinct logging and HTTP failure
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ad9f7143-77fb-4471-ab08-e22f294b4baa
📒 Files selected for processing (1)
crates/metrics-endpoint/src/lib.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
chet
changed the title
|
@poroh thanks -- reworked exactly to your suggestion. There's now one dedicated |
|
Thanks @coderabbitai -- |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes.
This resolves the earlier concern about hidden encoder errors. Good work on the encoder-thread rework overall — bounding the CPU cost to a single core and ensuring clean shutdown independent of keep-alive connections are the right characteristics for a metrics endpoint under load. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/metrics-endpoint/src/lib.rs`:
- Around line 250-254: Update spawn_encoder_thread to return io::Result instead
of calling expect on thread creation, and propagate the result through both
metrics endpoint entry points covering the referenced startup paths. Preserve
the existing encoder channel/thread setup on success, while converting detached
endpoint startup failures into logged errors rather than task panics.
- Around line 843-855: Update the keep-alive setup in the test around the held
TcpStream to send an HTTP /health request and read its response fully before
calling cancel_token.cancel(). Preserve the connection until after server
shutdown so the accepted connection task retains its encoder_tx clone and the
test deterministically verifies the shutdown behavior.
- Around line 446-451: The metrics endpoint’s 500 responses must distinguish
encoding errors, caught panics, and encoder task/thread failures instead of
sharing “Failed to encode metrics.” Update the response construction in the
encoder error and panic branches, including the TrySendError::Disconnected path
near the visible match, to use stable, distinct bodies for each failure class.
Add table-driven tests covering each class and asserting its mapped status/body
response.
- Around line 368-370: Update the reply transmission in the shown
requester-disconnect handling block to call `.ok()` on the result of
`reply_tx.send(result)`, explicitly discarding the expected send failure while
preserving the existing behavior.
🪄 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: 4a48348c-d1ec-41a6-8519-6a56520e5260
📒 Files selected for processing (1)
crates/metrics-endpoint/src/lib.rs
| Err(TrySendError::Disconnected(_)) => { | ||
| tracing::error!("metrics encoder thread is gone; cannot encode metrics"); | ||
| return Ok(Response::builder() | ||
| .status(500) | ||
| .body(Full::new(Bytes::from("Failed to encode metrics"))) | ||
| .unwrap()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return distinct responses for each 500 failure class.
Encoding errors, caught panics, and encoder task/thread failures all currently return "Failed to encode metrics", contrary to the PR objective. Give each class a stable, distinct body and add table-driven assertions for the mapping.
As per coding guidelines, use table-driven tests for functions mapping inputs to errors or observable results.
Also applies to: 462-483
🤖 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/metrics-endpoint/src/lib.rs` around lines 446 - 451, The metrics
endpoint’s 500 responses must distinguish encoding errors, caught panics, and
encoder task/thread failures instead of sharing “Failed to encode metrics.”
Update the response construction in the encoder error and panic branches,
including the TrySendError::Disconnected path near the visible match, to use
stable, distinct bodies for each failure class. Add table-driven tests covering
each class and asserting its mapped status/body response.
Source: Coding guidelines
The shared `/metrics` handler ran `registry.gather()` and the text encode inline on the tokio task serving the connection, so a large scrape could stall a worker -- and every binary serves `/metrics` through this crate. Spawning each encode on the blocking pool would cap one encode's latency but not the aggregate CPU: N concurrent scrapes would run N encoders on N cores. Instead this hands the encode to one dedicated thread behind a bounded queue, capping encode CPU to a single core no matter how many scrapes arrive. - `run_metrics_endpoint_with_listener` spawns one `metrics-encoder` thread that owns the registry and prefix and serves encode requests off a small bounded channel; the `/metrics` handler sends a request and awaits the result. The thread exits when the server shuts down and the sender drops. - A full queue returns 503 (backpressure); a failed encode returns 500 carrying the Prometheus error, kept distinct from a dead-thread 500. - `encode_metrics` now returns the Prometheus encode error instead of unwrapping it. The success-path bytes are unchanged, so `/metrics` output stays byte-identical. This supports NVIDIA#3524
|
Thanks @coderabbitai -- all three addressed:
|
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "--- spawn_encoder_thread signature ---"
rg -n 'fn spawn_encoder_thread' -A5 crates/metrics-endpoint/src/lib.rs || true
echo "--- entry point signatures ---"
rg -n 'pub (async )?fn (run|serve|start)' -A3 crates/metrics-endpoint/src/lib.rs || true
echo "--- EncodeError enum ---"
rg -n 'enum EncodeError' -A10 crates/metrics-endpoint/src/lib.rs || true
echo "--- bmc-proxy caller ---"
rg -n 'metrics_endpoint|spawn_encoder_thread|metrics-endpoint' -g '*bmc-proxy*' -A3 -B3 -r . 2>/dev/null || true
fd -t f . -x echo {} | rg -i 'bmc-proxy' | head -20
echo "--- shutdown test with keep-alive ---"
rg -n 'GET /health|keep-alive|keep_alive' crates/metrics-endpoint/src/lib.rs -A5 -B5 || trueLength of output: 2610 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "--- find bmc-proxy dir ---"
fd -t d bmc-proxy
echo "--- bmc-proxy usage of metrics endpoint ---"
rg -n 'metrics_endpoint|run_metrics_endpoint' -C4 --glob '*bmc-proxy*' . || true
echo "--- shutdown test ---"
rg -n 'fn.*shutdown|GET /health|keep_alive|keep-alive' crates/metrics-endpoint/src/lib.rsLength of output: 1544 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n 'run_metrics_endpoint' -C5 crates/bmc-proxy/src/**/*.rs 2>/dev/null || rg -n 'run_metrics_endpoint' -C5 -r crates/bmc-proxyLength of output: 467 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n 'run_metrics_endpoint' crates/bmc-proxy -C5Length of output: 1039
All three points check out. This looks solid — nice work tightening up the failure modes around thread startup and shutdown. ヽ(=^･ω･^=)丹 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/metrics-endpoint/src/lib.rs (1)
443-489: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCount scrape failures instead of only logging them. At
crates/metrics-endpoint/src/lib.rs:443-489, the busy, disconnected, encode-error, panic, and dropped-reply paths are bounded operational outcomes; emit a singlecarbide_instrument::Eventsuch ascarbide_metrics_scrape_failures_totalwith an enum outcome label.🤖 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/metrics-endpoint/src/lib.rs` around lines 443 - 489, The metrics scrape failure branches in the request handling flow must emit a carbide_instrument::Event named carbide_metrics_scrape_failures_total instead of only logging. Add one event per failure outcome, labeled distinctly for the TrySendError::Full, TrySendError::Disconnected, EncodeError::Encode, EncodeError::Panicked, and reply_rx.await error paths, while preserving their existing responses and logs.Source: Coding guidelines
🤖 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/metrics-endpoint/src/lib.rs`:
- Around line 292-304: Update the connection-serving flow around
serve_connection to track each spawned task in a JoinSet instead of detaching
it. During shutdown, cancel the active connection tasks, await their completion,
and only then return from the endpoint after stopping and joining the encoder;
extend the relevant test to assert that the held connection is closed.
---
Nitpick comments:
In `@crates/metrics-endpoint/src/lib.rs`:
- Around line 443-489: The metrics scrape failure branches in the request
handling flow must emit a carbide_instrument::Event named
carbide_metrics_scrape_failures_total instead of only logging. Add one event per
failure outcome, labeled distinctly for the TrySendError::Full,
TrySendError::Disconnected, EncodeError::Encode, EncodeError::Panicked, and
reply_rx.await error paths, while preserving their existing responses and logs.
🪄 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: ca8c028e-bedc-4fd1-b22b-8b9c53c8b1f2
📒 Files selected for processing (2)
crates/bmc-proxy/src/metrics.rscrates/metrics-endpoint/src/lib.rs
| // The accept loop only returns once the token is cancelled. Signal the encoder | ||
| // thread to stop and join it so shutdown is clean. The thread exits within one | ||
| // `ENCODER_RECV_TIMEOUT` (plus any encode already in progress), independent of | ||
| // connection tasks that may still hold a queue sender. Join on a blocking thread so | ||
| // this brief wait does not stall an async worker. | ||
| should_stop.store(true, Ordering::SeqCst); | ||
| match tokio::task::spawn_blocking(move || encoder_thread.join()).await { | ||
| Ok(Ok(())) => {} | ||
| Ok(Err(_panic)) => tracing::error!("metrics encoder thread panicked"), | ||
| Err(err) => tracing::error!(error = %err, "failed to join metrics encoder thread"), | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Cancel and join active connection tasks before returning.
Cancellation stops the accept loop and encoder, but detached serve_connection tasks can continue serving open keep-alive sockets after this function returns. Track them in a JoinSet, terminate them during shutdown, and extend the test to assert that held is closed.
As per coding guidelines, “Do not spawn background tasks without joining them”; as per path instructions, prefer joined and cancellable background tasks.
Also applies to: 890-900
🤖 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/metrics-endpoint/src/lib.rs` around lines 292 - 304, Update the
connection-serving flow around serve_connection to track each spawned task in a
JoinSet instead of detaching it. During shutdown, cancel the active connection
tasks, await their completion, and only then return from the endpoint after
stopping and joining the encoder; extend the relevant test to assert that the
held connection is closed.
Sources: Coding guidelines, Path instructions
|
Thanks @coderabbitai -- two dispositions here:
|
|
Tip For best results, initiate chat on the files or code changes.
Nice work landing the encoder-thread rework — this PR is in good shape. (=^･ω･^=) ノ ✏️ Learnings added
|
2 participants
The shared
/metricshandler ranregistry.gather()and the text encode inline on the tokio task serving the connection, so a large scrape could stall a worker -- and every binary serves/metricsthrough this crate. Spawning each encode on the blocking pool would cap one encode's latency but not the aggregate CPU: N concurrent scrapes would run N encoders on N cores. Instead this hands the encode to one dedicated thread behind a bounded queue, capping encode CPU to a single core no matter how many scrapes arrive.run_metrics_endpoint_with_listenerspawns onemetrics-encoderthread that owns the registry and prefix and serves encode requests off a small bounded channel; the/metricshandler sends a request and awaits the result.gather()(a misbehaving collector or callback) is caught, so it returns 500 for that one scrape and the encoder survives to serve the next -- the per-request isolation the inline/pool approach had.encode_metricsnow returns the Prometheus encode error instead of unwrapping it. The success-path bytes are unchanged, so/metricsoutput stays byte-identical.io::Errorfrom the endpoint entry points instead of panicking;bmc-proxy(the one direct caller ofrun_metrics_endpoint_with_listener) logs that error rather than discarding the server's exit outcome.This supports #3524