fix(engine): propagate W3C traceparent from incoming HTTP requests#1769
fix(engine): propagate W3C traceparent from incoming HTTP requests#1769mikkel-arturo wants to merge 1 commit into
Conversation
Replace the OtelContext::attach() + info_span!() pattern with set_parent() (via with_parent_headers) so incoming traceparent headers are honoured even for non-contextual spans at the top of the HTTP handler. Affected sites: - views.rs: HTTP handler span (primary) - engine/mod.rs: handle_invocation, enqueue_action spans
|
@mikkel-arturo is attempting to deploy a commit to the motia Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR refactors distributed tracing context propagation across the engine by introducing a unified ChangesDistributed Tracing Context Propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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
🤖 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 `@engine/src/workers/rest_api/views.rs`:
- Around line 312-319: The code extracts a "baggage" header into bg and later
logs headers with sanitize_headers_for_logging but "baggage" isn't treated as
sensitive, so raw values can be emitted; update the extraction or sanitization
so baggage is redacted: either set bg to a redacted placeholder (e.g.,
"[REDACTED]") when reading the header in the same block that sets tp and bg, or
update sanitize_headers_for_logging to treat the "baggage" header as sensitive
so any later header logging will mask it; refer to the tp and bg variables and
the sanitize_headers_for_logging function when making the change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 378efcbb-ad6e-45cb-b088-9e2cf85fa7d7
📒 Files selected for processing (3)
engine/src/engine/mod.rsengine/src/telemetry.rsengine/src/workers/rest_api/views.rs
| let tp = headers | ||
| .get("traceparent") | ||
| .and_then(|v| v.to_str().ok()) | ||
| .map(|s| s.to_string()); | ||
| let bg = headers | ||
| .get("baggage") | ||
| .and_then(|v| v.to_str().ok()) | ||
| .map(|s| s.to_string()); |
There was a problem hiding this comment.
Redact baggage before header logging.
Line 316 introduces explicit baggage ingestion, but sanitize_headers_for_logging does not classify it as sensitive, so Line 350 can emit raw baggage values (often user/session metadata) into logs.
🔧 Suggested patch
const SENSITIVE_HEADERS: &[&str] = &[
"authorization",
+ "baggage",
"cookie",
"set-cookie",
"x-api-key",🤖 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 `@engine/src/workers/rest_api/views.rs` around lines 312 - 319, The code
extracts a "baggage" header into bg and later logs headers with
sanitize_headers_for_logging but "baggage" isn't treated as sensitive, so raw
values can be emitted; update the extraction or sanitization so baggage is
redacted: either set bg to a redacted placeholder (e.g., "[REDACTED]") when
reading the header in the same block that sets tp and bg, or update
sanitize_headers_for_logging to treat the "baggage" header as sensitive so any
later header logging will mask it; refer to the tp and bg variables and the
sanitize_headers_for_logging function when making the change.
|
Hey @akashMishraX , thanks for your contribution. I reviewed it, and it looks good, but checking the W3C docs, i noticed it also recommends including trace-context: |
|
Also can you accept the Agreement Check? |
|
@mikkel-arturo We'd be happy to add this (possibly with modifications) but first we need you to include the following acknowledgement verbatim by editing your original PR message |
License agreement recorded@mikkel-arturo, your agreement has been recorded and you have been added to contributors.md. All future PRs from your account will pass this check automatically. |
|
Hey @anthonyiscoding @guibeira no problem. I added the agreement As for adding tests, I'm unsure what to do, as a) It's not clear to me what tests to add Overall, as I said, as someone that doesn't know anything about either Rust or Otel and just spent time with Claude until it had the right behavior, I'm not confident in the code and would treat this PR more as a PoC that someone else will need to polish to ensure it's conformant. That said, I have been using it in my base for a few weeks now and it's been working very well. I have been able to diagnose a large range of issues by having e2e observability. |
3 participants
Replace the OtelContext::attach() + info_span!() pattern with set_parent() (via with_parent_headers) so incoming traceparent headers are honoured even for non-contextual spans at the top of the HTTP handler.
Affected sites:
What
Inject traceparent http header into span
Why
I want end to end observability but there was no way to get it when a service not connected to iii was the initiator of a trace.
Notes
I know nothing about either OTel or Rust, so have no clue what the effects of this may be, other than it is working for me.
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor