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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe metrics endpoint now queues ChangesMetrics endpoint request handling
Estimated code review effort: 4 (Complex) | ~45 minutes 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 scrape and oneshot reply
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
|
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
|
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
|
| /// scrapes arrive at once. This small queue lets a couple of scrapes wait behind an | ||
| /// in-flight encode; a scrape that arrives with the queue already full is shed with | ||
| /// `503` rather than allowed to pile up. | ||
| const ENCODER_QUEUE_CAPACITY: usize = 4; |
There was a problem hiding this comment.
I would increase it to some bigger number. Something that represent of 0.5 second of work on modern core otherwise it can be annoyingly flaky.
| if should_stop.load(Ordering::SeqCst) { | ||
| break; | ||
| } | ||
| match encoder_rx.recv_timeout(ENCODER_RECV_TIMEOUT) { |
There was a problem hiding this comment.
You can consider channel with type enum Item { Work(reply_tx), Stop } to avoid polling.
poroh
left a comment
poroh
left a comment
There was a problem hiding this comment.
Couple improvements possible but both not critical
|
@poroh thanks -- both adopted:
|
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 800-803: Update the lifecycle test’s reply receive around
reply_rx.blocking_recv() to use the existing thread-exit deadline, polling until
a reply arrives or the deadline expires. Preserve the current encoder-reply and
encode-success assertions while ensuring an encoder regression cannot hang the
test indefinitely.
- Around line 299-303: The shutdown path around the encoder thread join must
enforce a bounded timeout, since send(Item::Stop) cannot be processed while
encode_metrics() is blocked in gather(). Update the tokio::task::spawn_blocking
flow to apply the established scrape/shutdown timeout mechanism or otherwise
bound the wait for encoder_thread.join(), while preserving normal completion and
panic isolation 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: 012e96c1-fbed-4cac-af61-c5e0c22e59de
📒 Files selected for processing (2)
crates/bmc-proxy/src/metrics.rscrates/metrics-endpoint/src/lib.rs
|
Thanks @coderabbitai -- both 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 "--- try_recv polling in test ---"
rg -n 'try_recv|blocking_recv' crates/metrics-endpoint/src/lib.rs -B3 -A3
echo "--- shutdown join with timeout ---"
rg -n 'timeout|join' crates/metrics-endpoint/src/lib.rs -B3 -A3 | head -80Length of output: 5106 Both points check out against the current code,
Both changes are correctly implemented and consistent with the connection-independent shutdown design. Nice work. (=^･ω･^=)ﾉ |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/metrics-endpoint/src/lib.rs (1)
747-1046: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of success/panic paths; consider adding the queue-full and disconnected-encoder branches.
TrySendError::Full(503) andTrySendError::Disconnected(500) inhandle_metrics_requestaren't exercised by any test. Both are unit-testable directly againstMetricsHandlerState— build async_channeland either fill it without draining, or drop the receiver — without needing a real server or thread.As per coding guidelines: "Use table-driven tests for functions mapping inputs to outputs, errors, or observable results."
🤖 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 747 - 1046, The test suite lacks coverage for the queue-full and disconnected-encoder branches in handle_metrics_request. Add a table-driven unit test that constructs MetricsHandlerState with a sync_channel, fills the queue to trigger TrySendError::Full and drops the receiver to trigger TrySendError::Disconnected, then verifies the handler returns 503 and 500 respectively without requiring a live server or encoder thread.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 308-312: Replace the `let _ =` Result-discard patterns in the
encoder-thread shutdown and reply paths with `.ok()`: update
`shutdown_tx.send(Item::Stop)` inside the `spawn_blocking` closure and
`reply_tx.send(result)` near the reply handling. Preserve the existing behavior
of intentionally ignoring these expected send failures.
---
Nitpick comments:
In `@crates/metrics-endpoint/src/lib.rs`:
- Around line 747-1046: The test suite lacks coverage for the queue-full and
disconnected-encoder branches in handle_metrics_request. Add a table-driven unit
test that constructs MetricsHandlerState with a sync_channel, fills the queue to
trigger TrySendError::Full and drops the receiver to trigger
TrySendError::Disconnected, then verifies the handler returns 503 and 500
respectively without requiring a live server or encoder thread.
🪄 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: bbf76dd0-c5cf-436a-9108-b2626ab96713
📒 Files selected for processing (2)
crates/bmc-proxy/src/metrics.rscrates/metrics-endpoint/src/lib.rs
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 -- both done: the two intentional fire-and-forget |
|
@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 "--- .ok() usages on send ---"
rg -n '\.send\(.*\)\.ok\(\)' crates/metrics-endpoint/src/lib.rs -B3 -A1
echo "--- backpressure test ---"
rg -n 'test_handle_metrics_request_queue_backpressure' -A40 crates/metrics-endpoint/src/lib.rsLength of output: 2725 Both changes check out,
Nice, minimal way to pin down both edge cases. This PR is in good shape. (=^･ω･^=)ﾉ |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/metrics-endpoint/src/lib.rs (1)
308-323: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThe five-second timeout does not bound the blocking shutdown task.
Once
spawn_blockingstarts, timing out itsJoinHandleonly detaches it. Ifsend(Item::Stop)blocks on a full queue orencoder_thread.join()waits on a hung collector, that blocking task remains alive and can still stall Tokio runtime shutdown. Use a shutdown mechanism that does not leave an unbounded blocking task—for example, bounded stop delivery plusis_finished()polling, joining only after completion—and test a permanently blocked encoder.#!/bin/bash set -euo pipefail rg -n -A3 -B3 'name = "tokio"|spawn_blocking|ENCODER_SHUTDOWN_JOIN_TIMEOUT|shutdown_tx\.send|encoder_thread\.join' \ Cargo.lock crates/metrics-endpoint/src/lib.rsAs per coding guidelines, “Do not spawn background tasks without joining them.” As per path instructions, prioritize resource lifetimes and joined/cancellable background tasks.
🤖 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 308 - 323, The shutdown path around the spawn_blocking task must not leave an unbounded blocking task after the timeout. Replace the blocking stop-send and unconditional encoder_thread.join flow with bounded stop delivery and completion polling via is_finished(), only awaiting or joining once completion is confirmed; preserve the timeout behavior while ensuring all spawned tasks are joined before runtime shutdown. Add coverage for a permanently blocked encoder to verify shutdown returns without retaining an unjoined blocking task.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.
Duplicate comments:
In `@crates/metrics-endpoint/src/lib.rs`:
- Around line 308-323: The shutdown path around the spawn_blocking task must not
leave an unbounded blocking task after the timeout. Replace the blocking
stop-send and unconditional encoder_thread.join flow with bounded stop delivery
and completion polling via is_finished(), only awaiting or joining once
completion is confirmed; preserve the timeout behavior while ensuring all
spawned tasks are joined before runtime shutdown. Add coverage for a permanently
blocked encoder to verify shutdown returns without retaining an unjoined
blocking task.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 49d23c0f-af7b-4148-8807-bf550e279acd
📒 Files selected for processing (2)
crates/bmc-proxy/src/metrics.rscrates/metrics-endpoint/src/lib.rs
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