Conversation
Signed-off-by: datron <Datron@users.noreply.github.com>
Changed Files
|
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR updates request ID handling to prioritize the ChangesRequest ID Header-First Resolution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates request correlation handling so that, when an upstream proxy supplies x-request-id, Superposition reuses it for observability and propagation instead of always relying on an internally generated request ID.
Changes:
- Add
x-request-idextraction to the custom Actix root span builder so logs can include the inbound request ID. - Update the request/response logging middleware to prefer the inbound
x-request-idheader and fall back to the generatedtracing_actix_web::RequestId, then setx-request-idon the response.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/superposition/src/log_span.rs | Includes inbound x-request-id in the tracing root span when present. |
| crates/service_utils/src/middlewares/request_response_logging.rs | Prefers inbound x-request-id (fallback to generated) and returns it on the response header. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let request_id = header_extractor(headers, "x-request-id"); | ||
| let method = request.method().to_string(); | ||
| tracing_actix_web::root_span!(request, workspace, org, method, path,) | ||
| if let Some(request_id) = request_id { | ||
| tracing_actix_web::root_span!(request, request_id = %request_id, workspace, org, method, path,) | ||
| } else { | ||
| tracing_actix_web::root_span!(request, workspace, org, method, path,) |
There was a problem hiding this comment.
This is a good one - should we move the sanitization ahead - so that what we log and what we use / send back are the same ?
| // Check for x-request-id header first, fallback to generated one from extensions | ||
| let request_id = req.headers().get("x-request-id").cloned().or_else(|| { | ||
| req.extensions() | ||
| .get::<tracing_actix_web::RequestId>() | ||
| .and_then(|req_id| { | ||
| header::HeaderValue::from_str(&req_id.to_string()).ok() | ||
| }) | ||
| }); |
There was a problem hiding this comment.
The header_extractor enforces a 256 char limit which will address this as well - so moving the header_extraction here will solve both issues I believe.
Problem
Superposition is ignoring x-request-id from proxy
Solution
Read the header and use in logs, if missing then generate a request ID and use that
Summary by CodeRabbit