fix: guard inflight_requests and request_duration from early returns. #2576
Conversation
WalkthroughIntroduces a RAII-based RequestMetricsGuard in Ingress push_handler to manage inflight gauge decrement and duration observation automatically. Metrics are incremented at request start, and the guard’s Drop handles cleanup on all exits, replacing the prior manual end-of-function metrics update. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant IngressHandler as Ingress::handle_payload
participant Metrics as Metrics (counter/gauge/hist)
participant Guard as RequestMetricsGuard
Client->>IngressHandler: handle_payload(payload)
IngressHandler->>Metrics: increment request_counter
IngressHandler->>Metrics: inc inflight_requests
IngressHandler->>Metrics: observe request_bytes
IngressHandler->>Guard: create(start_time, inflight, duration)
Note right of Guard: RAII guard ensures cleanup on any exit
alt normal processing
IngressHandler-->>Client: return result
else early return / error
IngressHandler-->>Client: return error
end
Note over Guard,Metrics: On drop
Guard->>Metrics: dec inflight_requests
Guard->>Metrics: observe request_duration(elapsed)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/runtime/src/pipeline/network/ingress/push_handler.rs (4)
110-113: Minor style consistency: use imported type names instead of fully-qualified ones.You already import IntGauge and Histogram (Line 18). Using those here improves consistency and readability.
- inflight_requests: prometheus::IntGauge, - request_duration: prometheus::Histogram, + inflight_requests: IntGauge, + request_duration: Histogram,
137-147: Prefer explicit control flow over Option::map when doing side effects.Using map for side effects is slightly non-idiomatic and can be misread. An if let reads clearer and keeps the guard’s lifetime explicit. Functionally identical.
- let _inflight_guard = self.metrics().map(|m| { - m.request_counter.inc(); - m.inflight_requests.inc(); - m.request_bytes.inc_by(payload.len() as u64); - RequestMetricsGuard { - inflight_requests: m.inflight_requests.clone(), - request_duration: m.request_duration.clone(), - start_time, - } - }); + let _inflight_guard = if let Some(m) = self.metrics() { + m.request_counter.inc(); + m.inflight_requests.inc(); + m.request_bytes.inc_by(payload.len() as u64); + Some(RequestMetricsGuard { + inflight_requests: m.inflight_requests.clone(), + request_duration: m.request_duration.clone(), + start_time, + }) + } else { + None + };Optional note: if you care about avoiding the negligible Instant creation when metrics are disabled, move Instant::now() inside the if-let. It will shift the measured start by a few microseconds; probably not worth it unless you want absolute minimal overhead.
21-21: Tiny consistency nit: you import Instant but call std::time::Instant::now().Use the imported alias for consistency, or drop the import and keep the fully-qualified path. I’d go with the alias.
Outside this selected range (Line 135), update the call site:
let start_time = Instant::now();
137-147: Add a regression test to prove the guard covers typical early-return sites.Given the prior leaks, a focused test will prevent backslides. Table-test these cases and assert inflight returns to baseline and duration is observed at least once:
- decode_message failure path
- invalid TwoPartMessageType path
- response stream creation failure path
- generate() error path
- publish send error path (mid-stream)
I can help scaffold a test using a test registry and a fake Ingress that forces these exits.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/runtime/src/pipeline/network/ingress/push_handler.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/runtime/src/pipeline/network/ingress/push_handler.rs (1)
lib/llm/src/perf.rs (1)
start_time(125-127)
🔇 Additional comments (1)
lib/runtime/src/pipeline/network/ingress/push_handler.rs (1)
108-121: RAII guard cleanly fixes the metrics leak on early returns — nice.Creating RequestMetricsGuard and letting Drop handle inflight dec + duration observe guarantees correctness across all exit paths, including errors and early returns. This aligns perfectly with the PR goal (DIS-496).
…#2576) Signed-off-by: Hannah Zhang <[email protected]>
…#2576) Signed-off-by: Krishnan Prashanth <[email protected]>
…#2576) Signed-off-by: nnshah1 <[email protected]>
Overview:
guard inflight_requests and request_duration from early returns.
Details:
guard inflight_requests and request_duration from early returns.
Where should the reviewer start?
lib/runtime/src/pipeline/network/ingress/push_handler.rs
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
DIS-496 some metrics leaks on early-returns
Summary by CodeRabbit
Bug Fixes
Refactor