feat: trace infrastructure — spans, targets, request ID propagation

[nnshah1]

Summary

• Split HTTP router into system (debug-level) and inference (info-level) trace layers with separate on_response callbacks
• Add make_inference_request_span with always-on request_span target and empty span fields for downstream recording
• get_or_create_request_id now returns String (warns on invalid UUID instead of 400)
• Rename span field x_dynamo_request_id → request_id for consistency
• Add echo_request_id_header middleware to copy x-request-id from request to response headers
• Add request_span=trace filter directive so request context is always visible

Test plan

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

Part 1 of 4 for DIS-1643: Consistent Error Tracing

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Request IDs are now echoed in response headers for improved request tracing.

• Improvements

  • Request ID handling now prefers distributed tracing context and falls back to generating a UUID when needed, improving trace reliability.
  • Logging and tracing reorganized with separate system vs. inference spans and unified response-level logging for clearer diagnostics.
  • Trace output now uses a consolidated request_id field for consistency.

[coderabbitai]

Walkthrough

Refactors request ID resolution and tracing: get_or_create_request_id now takes only headers and prefers distributed trace context; renamed trace fields from x_dynamo_request_id to request_id; split HTTP routers and span factories for system vs inference; updated trace header handling, logging, and span targets.

Changes

Cohort / File(s)	Summary
OpenAI request-id logic & API lib/llm/src/http/service/openai.rs	Changed get_or_create_request_id signature to accept only &HeaderMap; prefer distributed trace-context request_id; validate deprecated x-dynamo-request-id as UTF‑8 + UUID; fall back to new UUID; updated all local call sites.
Anthropic call-site update lib/llm/src/http/service/anthropic.rs	Call site updated to invoke new get_or_create_request_id(headers) (removed prior explicit None/user arg).
Router split & middleware lib/llm/src/http/service/service_v2.rs	Split router into system_router and inference_router; introduced separate span factories (make_system_request_span, make_inference_request_span); unified response logging behavior; added echo_request_id_header middleware; merged OpenAPI into system routes.
Runtime logging & trace context renames lib/runtime/src/logging.rs	Renamed serialized/struct field x_dynamo_request_id → request_id; replaced make_request_span with make_inference_request_span and added make_system_request_span; trace header injection now uses request-id; JSON logs emit request_id; adjusted log filter defaults.
Span target changes (ingress/push & system server) lib/runtime/src/pipeline/network/ingress/push_endpoint.rs, lib/runtime/src/system_status_server.rs	Updated span target to target: "request_span" in push_endpoint; switched system_status_server to use make_system_request_span for HTTP TraceLayer.
HTTP ingress trace parsing & tests lib/runtime/src/pipeline/network/ingress/http_endpoint.rs	TraceParent parsing now reads request-id (falls back to deprecated x-dynamo-request-id) and stores it in traceparent.request_id; adjusted tests to assert request_id rather than x_dynamo_request_id.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description covers the Summary and Test plan sections, but omits required template sections: Details (specifics of changes), Where should the reviewer start (file guidance), and Related Issues.	Add missing template sections: expand Details with specific change breakdown, specify key files to review, and link the related GitHub issue DIS-1643.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: trace infrastructure improvements including spans, targets, and request ID propagation across multiple files.
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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

lib/runtime/src/pipeline/network/ingress/push_endpoint.rs (1)

96-106: ⚠️ Potential issue | 🟡 Minor

Generate a fallback request_id when headers are absent.

This branch now creates an always-on request_span, but it still records no request_id. Any NATS payload without headers will therefore run with no distributed request ID, so the fallback ingress path still falls outside the new correlation scheme.

Suggested fix

                 } else {
-                    tracing::info_span!(target: "request_span", "handle_payload")
+                    tracing::info_span!(
+                        target: "request_span",
+                        "handle_payload",
+                        request_id = %uuid::Uuid::new_v4(),
+                    )
                 };

🤖 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_endpoint.rs` around lines 96 -
106, When headers are missing, create and attach a generated request_id so the
fallback tracing span participates in the correlation scheme: generate a unique
ID (e.g. uuid::Uuid::new_v4().to_string()) and include it as the request_id
field on the fallback span instead of the current plain
tracing::info_span!(target: "request_span", "handle_payload"); mirror the same
span field names used by make_handle_payload_span and keep component_name,
endpoint_name, namespace, and instance_id in the span so the fallback path
records the same metadata.

🧹 Nitpick comments (1)

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

529-530: Consider logging client errors (4xx) at warn level for inference routes.

Currently, both 4xx and 5xx responses are logged at error level. Client errors (e.g., 400 Bad Request, 401 Unauthorized, 422 Unprocessable Entity) typically indicate issues with the client's request rather than server failures. Logging these as errors could cause alert fatigue and obscure genuine server issues.

♻️ Suggested change to differentiate client vs server errors

         let on_response_inference = |response: &Response<Body>,
                                      latency: Duration,
                                      _span: &tracing::Span| {
             let status = response.status();
             let latency_ms = latency.as_millis();
-            if status.is_server_error() || status.is_client_error() {
+            if status.is_server_error() {
                 tracing::error!(status = %status.as_u16(), latency_ms = %latency_ms, "http response sent");
+            } else if status.is_client_error() {
+                tracing::warn!(status = %status.as_u16(), latency_ms = %latency_ms, "http response sent");
             } else {
                 tracing::info!(status = %status.as_u16(), latency_ms = %latency_ms, "http response sent");
             }
         };

🤖 Prompt for AI Agents

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

In `@lib/llm/src/http/service/service_v2.rs` around lines 529 - 530, The current
tracing::error! call logs both client (4xx) and server (5xx) responses as
errors; change the conditional to log client errors using tracing::warn! and
server errors using tracing::error!—use the existing status and latency_ms
variables (the status.is_client_error() and status.is_server_error() checks) to
branch and emit tracing::warn!(status = %status.as_u16(), latency_ms =
%latency_ms, "http response sent") for 4xx and tracing::error!(...) for 5xx so
client-side request issues don't generate error-level alerts.

🤖 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/openai.rs`:
- Around line 294-330: get_or_create_request_id currently relies on
get_distributed_tracing_context (the logging layer) so in READABLE setups it
will ignore a valid x-dynamo-request-id header and mint a new UUID; instead,
move the canonical request id into request extensions set by a
pre-handler/middleware (the same place make_inference_request_span should
read/write) and have get_or_create_request_id read from that extension first,
then fall back to validating DYNAMO_REQUEST_ID_HEADER from headers, and only
generate a new UUID if neither the extension nor a valid header exists;
update/introduce the middleware to populate the extension and remove dependence
on get_distributed_tracing_context in get_or_create_request_id.

In `@lib/runtime/src/logging.rs`:
- Around line 341-349: make_system_request_span currently creates a new Span
without preserving incoming trace context; update make_system_request_span(req:
&Request<B>) to extract the incoming "traceparent" (and related W3C headers)
from req.headers(), convert to a tracing::SpanContext or equivalent the project
uses (same approach as make_inference_request_span), and call
span.set_parent(...) (or use tracing::Span::current with the extracted context)
before returning so system routes join the caller's trace; reference the
existing make_inference_request_span implementation for the exact header parsing
and set_parent usage to replicate here.

---

Outside diff comments:
In `@lib/runtime/src/pipeline/network/ingress/push_endpoint.rs`:
- Around line 96-106: When headers are missing, create and attach a generated
request_id so the fallback tracing span participates in the correlation scheme:
generate a unique ID (e.g. uuid::Uuid::new_v4().to_string()) and include it as
the request_id field on the fallback span instead of the current plain
tracing::info_span!(target: "request_span", "handle_payload"); mirror the same
span field names used by make_handle_payload_span and keep component_name,
endpoint_name, namespace, and instance_id in the span so the fallback path
records the same metadata.

---

Nitpick comments:
In `@lib/llm/src/http/service/service_v2.rs`:
- Around line 529-530: The current tracing::error! call logs both client (4xx)
and server (5xx) responses as errors; change the conditional to log client
errors using tracing::warn! and server errors using tracing::error!—use the
existing status and latency_ms variables (the status.is_client_error() and
status.is_server_error() checks) to branch and emit tracing::warn!(status =
%status.as_u16(), latency_ms = %latency_ms, "http response sent") for 4xx and
tracing::error!(...) for 5xx so client-side request issues don't generate
error-level alerts.

🪄 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: 9995c547-125e-4341-8651-c6c7bd5c6c04

📥 Commits

Reviewing files that changed from the base of the PR and between 1bbdff2 and 40910ed.

📒 Files selected for processing (6)

• lib/llm/src/http/service/anthropic.rs
• lib/llm/src/http/service/openai.rs
• lib/llm/src/http/service/service_v2.rs
• lib/runtime/src/logging.rs
• lib/runtime/src/pipeline/network/ingress/push_endpoint.rs
• lib/runtime/src/system_status_server.rs

[nnshah1]

@coderabbitai review

[nnshah1]

@coderabbitai resolve

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

lib/runtime/src/pipeline/network/ingress/http_endpoint.rs (1)

289-320: ⚠️ Potential issue | 🟠 Major

Reuse the shared TraceParent parser here.

This helper still copies the full traceparent header into trace_id and never fills parent_id, so the spawned handle_payload span records an invalid trace ID and drops the caller relationship. It also skips the UUID validation you just added for request-id. Since http::HeaderMap already implements GenericHeaders, delegating to Self::from_headers(headers) keeps the HTTP ingress path consistent.

🔧 Proposed fix

 impl TraceParent {
     pub fn from_axum_headers(headers: &HeaderMap) -> Self {
-        let mut traceparent = TraceParent::default();
-
-        if let Some(value) = headers.get("traceparent")
-            && let Ok(s) = value.to_str()
-        {
-            traceparent.trace_id = Some(s.to_string());
-        }
-
-        if let Some(value) = headers.get("tracestate")
-            && let Ok(s) = value.to_str()
-        {
-            traceparent.tracestate = Some(s.to_string());
-        }
-
-        if let Some(value) = headers.get("x-request-id")
-            && let Ok(s) = value.to_str()
-        {
-            traceparent.x_request_id = Some(s.to_string());
-        }
-
-        // Read request-id from internal headers, with fallback to deprecated x-dynamo-request-id
-        if let Some(value) = headers
-            .get("request-id")
-            .or_else(|| headers.get("x-dynamo-request-id"))
-            && let Ok(s) = value.to_str()
-        {
-            traceparent.request_id = Some(s.to_string());
-        }
-
-        traceparent
+        Self::from_headers(headers)
     }
 }

🤖 Prompt for AI Agents

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

In `@lib/runtime/src/pipeline/network/ingress/http_endpoint.rs` around lines 289 -
320, The from_axum_headers implementation is duplicating parsing and incorrectly
putting the full traceparent into trace_id while never setting parent_id and
skipping the new request-id UUID validation; replace its body to delegate to the
shared parser by calling Self::from_headers(headers) (http::HeaderMap already
implements GenericHeaders) so trace_id/parent_id are populated correctly and
request_id validation is reused instead of manual header copying in
TraceParent::from_axum_headers.

🧹 Nitpick comments (1)

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

509-517: Don’t log all 4xx responses at error!.

That will skew error-rate dashboards and page on normal control flow like 400/404/409/429. Reserve error! for 5xx and downgrade 4xx to warn! or info!.

🔧 Suggested change

-            if status.is_server_error() || status.is_client_error() {
+            if status.is_server_error() {
                 tracing::error!(status = %status.as_u16(), latency_ms = %latency_ms, "http response sent");
+            } else if status.is_client_error() {
+                tracing::warn!(status = %status.as_u16(), latency_ms = %latency_ms, "http response sent");
             } else {
                 tracing::info!(status = %status.as_u16(), latency_ms = %latency_ms, "http response sent");
             }

🤖 Prompt for AI Agents

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

In `@lib/llm/src/http/service/service_v2.rs` around lines 509 - 517, The shared
on_response closure currently treats all 4xx as errors; change its logging level
so only 5xx responses use tracing::error! and 4xx use tracing::warn! (or
tracing::info! if you prefer less verbosity). Specifically, update the
on_response closure (the lambda taking response: &Response<Body>, latency:
Duration, _span: &tracing::Span) to check status.is_server_error() ->
tracing::error!, else if status.is_client_error() -> tracing::warn!, else ->
tracing::info!, keeping the same fields (status and latency_ms) and message
text.

🤖 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/runtime/src/logging.rs`:
- Around line 474-477: The TCP/header path currently clones "request-id" /
"x-dynamo-request-id" verbatim into request_id; instead validate those values as
UUIDs (same validation used by TraceParent::from_headers) before recording or
propagating them. Update the logic that sets the request_id variable to attempt
parsing the header string with the project's UUID parser (e.g., Uuid::parse_str
or the same helper used by TraceParent::from_headers) and only assign/cloned
value when parsing succeeds; likewise apply the same validation for the
alternate "x-dynamo-request-id" header and for the similar logic around the
487-504 block so malformed/non-UUID IDs are rejected and not injected into spans
or outgoing headers.
- Around line 1116-1120: The code unconditionally forces request_span=trace via
filter_layer.add_directive("request_span=trace".parse().unwrap()), which causes
DistributedTraceIdLayer::on_enter() to run for all requests and triggers the
fail-fast OtelData panic; change this so the add_directive call is only executed
when the DYN_LOGGING_SPAN_EVENTS env flag is set (e.g., check
DYN_LOGGING_SPAN_EVENTS or similar config before calling
filter_layer.add_directive), leaving the default behavior unchanged when the
flag is absent; ensure references to OtelData/DistributedTraceIdLayer::on_enter
remain protected by that gate so the panic path remains opt-in.

---

Outside diff comments:
In `@lib/runtime/src/pipeline/network/ingress/http_endpoint.rs`:
- Around line 289-320: The from_axum_headers implementation is duplicating
parsing and incorrectly putting the full traceparent into trace_id while never
setting parent_id and skipping the new request-id UUID validation; replace its
body to delegate to the shared parser by calling Self::from_headers(headers)
(http::HeaderMap already implements GenericHeaders) so trace_id/parent_id are
populated correctly and request_id validation is reused instead of manual header
copying in TraceParent::from_axum_headers.

---

Nitpick comments:
In `@lib/llm/src/http/service/service_v2.rs`:
- Around line 509-517: The shared on_response closure currently treats all 4xx
as errors; change its logging level so only 5xx responses use tracing::error!
and 4xx use tracing::warn! (or tracing::info! if you prefer less verbosity).
Specifically, update the on_response closure (the lambda taking response:
&Response<Body>, latency: Duration, _span: &tracing::Span) to check
status.is_server_error() -> tracing::error!, else if status.is_client_error() ->
tracing::warn!, else -> tracing::info!, keeping the same fields (status and
latency_ms) and message text.

🪄 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: a611060a-dcf5-49d1-81c0-e320dd75631e

📥 Commits

Reviewing files that changed from the base of the PR and between 40910ed and ad8c70c.

📒 Files selected for processing (4)

• lib/llm/src/http/service/openai.rs
• lib/llm/src/http/service/service_v2.rs
• lib/runtime/src/logging.rs
• lib/runtime/src/pipeline/network/ingress/http_endpoint.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• lib/llm/src/http/service/openai.rs

[coderabbitai]

✅ Actions performed

Comments resolved. Approval is disabled; enable reviews.request_changes_workflow to allow explicit top-level @coderabbitai resolve or @coderabbitai approve commands.

[nnshah1]

@coderabbitai resolve

Re: CodeRabbit comment on request_span=trace (line 1120)

This is by design — request_span=trace controls span creation only, not span events.

• Span creation (request_span=trace): Ensures the info_span with request_id/model/trace_id fields is always created, so all log events within the request inherit this context. This is unconditional and intentional.
• Span events (enter/exit): Separately gated by span_events_enabled() (line 918) and FmtSpan::CLOSE (line 980-983). These only fire when DYN_LOGGING_SPAN_EVENTS=1.
• DistributedTraceIdLayer: Has its own per-layer filter and processes spans independently of the base EnvFilter. The request_span=trace directive does not widen its processing scope or the panic path.

Re: TCP request-id UUID validation (line 477) — Fixed in latest commit, added .filter(|id| uuid::Uuid::parse_str(id).is_ok()).

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@nnshah1 — thanks for the clear explanation on both points!

request_span=trace (line 1120): Understood — the directive only governs span creation (ensuring the info_span with request_id/model/trace_id fields is unconditionally instantiated so downstream log events inherit that context), while span events (enter/exit) remain separately gated behind span_events_enabled() / FmtSpan::CLOSE and only fire when DYN_LOGGING_SPAN_EVENTS=1. The DistributedTraceIdLayer is also independently filtered and is unaffected by this directive. I've noted this for future reviews.

TCP request-id UUID validation (line 477): Thanks for the fix — .filter(|id| uuid::Uuid::parse_str(id).is_ok()) looks correct.

[resolve]

---

✏️ Learnings added

Learnt from: nnshah1
URL: https://github.com/ai-dynamo/dynamo/pull/7814

Timestamp: 2026-04-02T19:18:34.366Z
Learning: In `lib/runtime/src/logging.rs`, the `request_span=trace` filter directive added to the distributed logging configuration is intentional. It controls span *creation* only — ensuring `info_span` spans with `request_id`/`model`/`trace_id` fields are always created so downstream log events inherit request context. It does NOT widen span *events* (enter/exit), which are separately gated by `span_events_enabled()` (line 918) and `FmtSpan::CLOSE` (lines 980-983) and only fire when `DYN_LOGGING_SPAN_EVENTS=1`. The `DistributedTraceIdLayer` has its own per-layer filter and is unaffected by this directive. Do not flag `request_span=trace` as a potential log-level widening risk in future reviews.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

🧠 Learnings used

Learnt from: ryanolson
Repo: ai-dynamo/dynamo PR: 6649
File: lib/velo-messenger/src/events/handlers.rs:20-54
Timestamp: 2026-03-21T17:10:27.409Z
Learning: In `lib/velo-messenger/src/events/handlers.rs`, the three internal event system handlers (`_event_subscribe`, `_event_trigger`, `_event_trigger_request`) intentionally swallow errors by logging via `tracing::warn!` and returning `Ok(())`. This is a deliberate design choice by ryanolson (see commit 6e2558353e for rationale). Do not flag the `Ok(())` return on error in these handlers as a bug in future reviews.

---

Learnt from: nnshah1
Repo: ai-dynamo/dynamo PR: 5400
File: lib/runtime/src/logging.rs:0-0
Timestamp: 2026-01-15T08:25:44.096Z
Learning: In lib/runtime/src/logging.rs, the on_new_span method intentionally panics (via expect()) when trace_id or span_id is not available from OtelData. This is fail-fast behavior by design to catch configuration bugs during development. The layer ordering explicitly ensures tracing_opentelemetry::layer() runs first to populate OtelData. Silent fallback is avoided to prevent masking configuration issues. This code path only activates when DYN_LOGGING_SPAN_EVENTS=1 is explicitly enabled.

---

Learnt from: PeaBrane
Repo: ai-dynamo/dynamo PR: 7593
File: lib/mocker/src/replay/collector.rs:365-377
Timestamp: 2026-03-24T04:23:18.746Z
Learning: In `lib/mocker/src/replay/collector.rs`, the `TraceCollector::snapshots()` test-only method intentionally returns `Vec<TraceRequestStatsSnapshot>` in nondeterministic (HashMap iteration) order. PeaBrane confirmed there is no order-sensitive consumer of this method today, so adding a deterministic sort would be unnecessary defensive churn. Do not flag the lack of ordering in this method in future reviews.

[nnshah1]

Superseded by follow-up PR on top of #7733 which was merged first.