feat: request lifecycle logging via InflightGuard

[nnshah1]

Summary

• Add request_id field to InflightGuard with structured "request received" (INFO) and "request completed" (INFO/ERROR) lifecycle logs
• Add accessor methods and Display impls for RequestType/ErrorType
• Change cancellation logging from trace\! to warn\! with structured fields in disconnect.rs
• Add worker lifecycle logs ("request received"/"request completed") in push_handler.rs
• All create_inflight_guard call sites now pass request_id

Test plan

• cargo check --workspace passes
• cargo clippy -p dynamo-llm -p dynamo-runtime --no-deps clean
• CI checks pass

Part 2 of 4 for DIS-1643: Consistent Error Tracing
Depends on #7814

🤖 Generated with Claude Code

Summary by CodeRabbit

• Improvements
  • Enhanced request tracing with unique identifiers throughout the system for improved debugging and observability.
  • Added structured logging for request lifecycle events (received, completed, cancelled) with detailed metadata.
  • Improved visibility into request metrics with accessible data about models, endpoints, and timing information.
  • Elevated disconnect and error logging to provide better visibility into connection and cancellation events.

[coderabbitai]

Walkthrough

The pull request extends request tracking and observability throughout the LLM service stack. Changes update the metrics infrastructure to store and expose request identifiers, add lifecycle logging (request received/completed), and thread request IDs through service handlers across HTTP and gRPC protocols to service endpoints including OpenAI, Anthropic, and Tensor. Disconnect monitoring logs are elevated from trace to warning level.

Changes

Cohort / File(s)	Summary
Metrics Infrastructure lib/llm/src/http/service/metrics.rs	Extended InflightGuard to store request_id; added public accessor methods (request_id(), model(), endpoint(), request_type(), error_type(), elapsed_ms()); introduced logging in constructor ("request received") and drop ("request completed") with structured metadata; added Display trait impls for RequestType and ErrorType; updated ResponseMetricCollector to include and store ISL.
HTTP Service Endpoints lib/llm/src/http/service/openai.rs, lib/llm/src/http/service/anthropic.rs	Updated create_inflight_guard() invocations in multiple handlers (completions_single, completions_batch, chat_completions, embeddings, images, videos, audio_speech, anthropic_messages) to pass request identifier (request_id.clone() or ctx.id().to_string()) as fourth argument; no control flow or error handling changes.
gRPC Service Endpoints lib/llm/src/grpc/service/openai.rs, lib/llm/src/grpc/service/tensor.rs	Updated create_inflight_guard() calls in completion_response_stream and tensor_response_stream to include request_id.clone() as additional parameter.
Disconnect Observability lib/llm/src/http/service/disconnect.rs	Upgraded disconnect-related logs from trace to warning level in connection_monitor; enhanced monitor_for_disconnects to emit structured warning log when request is cancelled by client, including metadata from inflight_guard (request_id, model, endpoint, request_type, elapsed_ms).
Runtime Logging lib/runtime/src/pipeline/network/ingress/push_handler.rs	Added info-level logging at request lifecycle boundaries in handle_payload: "request received" after decoding control message and "request completed" after successful processing.
Test Updates lib/llm/tests/http_metrics.rs	Updated test cases to pass empty String as new request_id parameter to create_inflight_guard() signature.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The PR description covers the Summary and Test plan sections but lacks the required Details, Where should the reviewer start, and Related Issues sections from the template.	Add missing template sections: expand Details with specific implementation notes, identify key files for review (especially lib/llm/src/http/service/metrics.rs and lib/llm/src/http/service/disconnect.rs), and explicitly list related GitHub issue #DIS-1643 using Relates to keyword.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: adding request lifecycle logging via InflightGuard, which aligns with the primary objective of the PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

lib/llm/src/http/service/disconnect.rs (1)

138-153: Add structured fields to unexpected-close warnings.

Lines 138 and 153 currently log plain strings; include request_id, model, endpoint, and request_type for consistent correlation with other lifecycle logs.

Proposed refactor

-            tracing::warn!("Connection closed unexpectedly; issuing cancellation");
+            tracing::warn!(
+                request_id = %engine_context.id(),
+                model = %cancellation_labels.model.as_str(),
+                endpoint = %cancellation_labels.endpoint.as_str(),
+                request_type = %cancellation_labels.request_type.as_str(),
+                "connection closed unexpectedly; issuing cancellation"
+            );
...
-            tracing::warn!("Stream closed unexpectedly; issuing cancellation");
+            tracing::warn!(
+                request_id = %engine_context.id(),
+                model = %cancellation_labels.model.as_str(),
+                endpoint = %cancellation_labels.endpoint.as_str(),
+                request_type = %cancellation_labels.request_type.as_str(),
+                "stream closed unexpectedly; issuing cancellation"
+            );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/llm/src/http/service/disconnect.rs` around lines 138 - 153, Update the
two tracing::warn calls that log unexpected closures to include structured
fields for correlation: replace the plain string warnings in the
ConnectionStatus::ClosedUnexpectedly match arm (the tracing::warn! that follows
metrics.inc_client_disconnect()/metrics.inc_cancellation() and
engine_context.kill()) and the tracing::warn! inside the match handling
stream_rx.await | Ok(ConnectionStatus::ClosedUnexpectedly) to pass request_id,
model, endpoint, and request_type as fields (e.g., tracing::warn!(request_id =
%request_id, model = %model, endpoint = %endpoint, request_type = %request_type,
"Connection closed unexpectedly; issuing cancellation") so the logs align with
other lifecycle logs and preserve the existing message and behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/llm/src/http/service/metrics.rs`:
- Around line 1050-1052: The current mapping for ErrorType::Cancelled in the
metrics/error_detail logic only says "client disconnected before completion"
which mislabels timeout-driven cancellations; update the ErrorType::Cancelled
branch (where `let detail = match self.error_type { ... }` is defined) to use a
broader description such as "request cancelled or timed out before completion"
(or similar wording that covers both client disconnect and timeout) so lifecycle
logs correctly reflect both cancellation causes; leave other ErrorType variants
unchanged.

In `@lib/runtime/src/pipeline/network/ingress/push_handler.rs`:
- Around line 366-369: The unconditional tracing::info!("request completed")
incorrectly logs success even for publish-failure/cancellation paths; change it
so completion is only logged on the successful completion path (e.g., after the
publish/send succeeds) and instead log failure/cancellation with appropriate
tracing::error or tracing::warn in the send-failure/cancel branches; locate the
tracing::info call in push_handler.rs (the spot using context.id()) and
move/remove it from the unconditional location and add targeted logs inside the
success branch and inside the send failure/cancellation branches so each outcome
is recorded accurately.

---

Nitpick comments:
In `@lib/llm/src/http/service/disconnect.rs`:
- Around line 138-153: Update the two tracing::warn calls that log unexpected
closures to include structured fields for correlation: replace the plain string
warnings in the ConnectionStatus::ClosedUnexpectedly match arm (the
tracing::warn! that follows
metrics.inc_client_disconnect()/metrics.inc_cancellation() and
engine_context.kill()) and the tracing::warn! inside the match handling
stream_rx.await | Ok(ConnectionStatus::ClosedUnexpectedly) to pass request_id,
model, endpoint, and request_type as fields (e.g., tracing::warn!(request_id =
%request_id, model = %model, endpoint = %endpoint, request_type = %request_type,
"Connection closed unexpectedly; issuing cancellation") so the logs align with
other lifecycle logs and preserve the existing message and 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: Pro

Run ID: 5c711e7a-3a08-4006-8af1-56bda5c90952

📥 Commits

Reviewing files that changed from the base of the PR and between 13058cb and 215ab79.

📒 Files selected for processing (8)

• lib/llm/src/grpc/service/openai.rs
• lib/llm/src/grpc/service/tensor.rs
• lib/llm/src/http/service/anthropic.rs
• lib/llm/src/http/service/disconnect.rs
• lib/llm/src/http/service/metrics.rs
• lib/llm/src/http/service/openai.rs
• lib/llm/tests/http_metrics.rs
• lib/runtime/src/pipeline/network/ingress/push_handler.rs

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Don't label every Cancelled request as a client disconnect.

ErrorType::Cancelled is documented as covering client cancellation and timeout cases, but the new error_detail only describes the client side. That will mislabel timeout paths in the lifecycle logs.

📝 Suggested wording

-                    ErrorType::Cancelled => "client disconnected before completion",
+                    ErrorType::Cancelled => "request cancelled or timed out before completion",

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	let detail = match self.error_type {
	ErrorType::Cancelled => "client disconnected before completion",
	ErrorType::Internal => "internal server error during processing",
	let detail = match self.error_type {
	ErrorType::Cancelled => "request cancelled or timed out before completion",
	ErrorType::Internal => "internal server error during processing",

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/llm/src/http/service/metrics.rs` around lines 1050 - 1052, The current
mapping for ErrorType::Cancelled in the metrics/error_detail logic only says
"client disconnected before completion" which mislabels timeout-driven
cancellations; update the ErrorType::Cancelled branch (where `let detail = match
self.error_type { ... }` is defined) to use a broader description such as
"request cancelled or timed out before completion" (or similar wording that
covers both client disconnect and timeout) so lifecycle logs correctly reflect
both cancellation causes; leave other ErrorType variants unchanged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Avoid unconditional "request completed" logging.

At Line 366, this log is emitted even after publish-failure/cancellation paths (e.g., send failure branch), which can misreport failed/cancelled flows as successful completions.

Proposed fix

-        tracing::info!(
-            request_id = %context.id(),
-            "request completed"
-        );
+        let elapsed_ms = start_time.elapsed().as_millis();
+        if send_complete_final {
+            tracing::info!(
+                request_id = %context.id(),
+                status = "success",
+                elapsed_ms = %elapsed_ms,
+                "request completed"
+            );
+        } else if context.is_stopped() {
+            tracing::warn!(
+                request_id = %context.id(),
+                status = "cancelled",
+                elapsed_ms = %elapsed_ms,
+                "request completed"
+            );
+        } else {
+            tracing::error!(
+                request_id = %context.id(),
+                status = "error",
+                elapsed_ms = %elapsed_ms,
+                "request completed"
+            );
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/runtime/src/pipeline/network/ingress/push_handler.rs` around lines 366 -
369, The unconditional tracing::info!("request completed") incorrectly logs
success even for publish-failure/cancellation paths; change it so completion is
only logged on the successful completion path (e.g., after the publish/send
succeeds) and instead log failure/cancellation with appropriate tracing::error
or tracing::warn in the send-failure/cancel branches; locate the tracing::info
call in push_handler.rs (the spot using context.id()) and move/remove it from
the unconditional location and add targeted logs inside the success branch and
inside the send failure/cancellation branches so each outcome is recorded
accurately.

[nnshah1]

Superseded by combined PR #7840