-
Notifications
You must be signed in to change notification settings - Fork 16
[APMSVLS-197] Add check to extract trace context present within event.request.headers #1011
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a fallback mechanism to extract distributed tracing context from event.request.headers for AppSync integration scenarios. This enhancement eliminates the need for custom extractors when RUM injects trace context into AppSync → Lambda resolver flows.
Changes:
- Added nested extraction logic to check
event.request.headersafter primary headers extraction fails
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f267045 to
5e936c4
Compare
5e936c4 to
e4d03a2
Compare
…request.headers Add automatic trace context extraction from event.request.headers for AppSync integration scenarios. This eliminates the need for customers to use customized extractors for RUM → AppSync → Lambda resolver flows where RUM-injected trace context is nested under event["request"]["headers"].
4043feb to
8a2baca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some unit testing and show an example trace in the PR description? Just to see what were we missing
96d4cb4 to
b0619c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b0619c2 to
391f38c
Compare
| } | ||
|
|
||
| if let Some(request_obj) = payload_value.get("request") { | ||
| if let Some(request_headers) = request_obj.get("headers") { | ||
| if let Some(sc) = propagator.extract(request_headers) { | ||
| debug!("Extracted trace context from event.request.headers"); | ||
| return Some(sc); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you decided that this should be last priority?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed there was some reason for the original order, so I didn't take a stance on where exactly it should go and added it as the last case..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on other projects which support AppSync extraction, it seems that they are considered in the same order as inferred spans would do it – I'd suggest to follow that same order
https://github.com/DataDog/datadog-lambda-js/blob/main/src/trace/context/extractor.ts#L92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! That's a good point.
…or service-specific traces
f76c2a8 to
6d228b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert_eq!( | ||
| context.trace_id, 555, | ||
| "Should prioritize event.request.headers as service-specific extraction" | ||
| ); |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name test_extract_span_context_priority_order and assertion message claim that event.request.headers should be prioritized, but the implementation shows it's actually checked after event.headers (line 1075). The trace_id assertion expects 555 (from event.request.headers) but based on the extraction order, it should extract 333 from event.headers first. Either the test assertion is incorrect, or the implementation order needs adjustment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is expected behavior?
Extraction from managed services (including Appsync) is higher priority than trace context within event.headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it just bugged due to the changes
| if let Some(request_obj) = payload_value.get("request") { | ||
| if let Some(request_headers) = request_obj.get("headers") { | ||
| if let Some(sc) = propagator.extract(request_headers) { | ||
| debug!("Extracted trace context from event.request.headers"); | ||
| return Some(sc); | ||
| } | ||
| } | ||
| } |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new extraction from event.request.headers is positioned before event.headers (line 1075), which changes the extraction priority. Based on the PR description stating this is a 'fallback', it should be placed after event.headers extraction, not before. This ordering makes it higher priority than top-level event headers, which contradicts the fallback concept.
| if let Some(request_obj) = payload_value.get("request") { | ||
| if let Some(request_headers) = request_obj.get("headers") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be simplified instead of having two nested if let somes to and_then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe even with the third one, depending on what clippy says
| } | ||
|
|
||
| #[test] | ||
| fn test_extract_span_context_appsync_real_world_example() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad naming with "real_world_example" instead, either set all trace context values to be something useful
| let payload = json!({ | ||
| "arguments": { | ||
| "userId": "12345" | ||
| }, | ||
| "identity": { | ||
| "sub": "user-sub-id" | ||
| }, | ||
| "request": { | ||
| "headers": { | ||
| "x-datadog-trace-id": "123456789012345", | ||
| "x-datadog-parent-id": "98765432109876", | ||
| "x-datadog-sampling-priority": "2", | ||
| "x-datadog-origin": "rum", | ||
| "x-datadog-tags": "_dd.p.dm=-0", | ||
| "user-agent": "Mozilla/5.0", | ||
| "content-type": "application/json" | ||
| } | ||
| }, | ||
| "info": { | ||
| "fieldName": "getUser", | ||
| "parentTypeName": "Query" | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be part of the payloads we have in testing as a file
duncanista
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some more comments
…sts. Removed redundant tests that would be caught by others. Priority and functionality is entirely covered within test_extract_span_context_priority_order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| context.trace_id, 555, | ||
| "Should prioritize event.request.headers as service-specific extraction" |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test assertion comment claims event.request.headers should be prioritized, but according to the implementation, event.request.headers is checked after trigger-specific headers and before event.headers. With the test data showing headers containing trace_id=111, the actual priority would favor the trigger-specific headers first. The test should either verify that trace_id=111 is extracted (if headers are checked first), or the test data should be adjusted to only include event.request.headers to properly validate this extraction path.
APMSVLS-197 feat: Add check to extract trace context from event.request.headers
Overview
This PR adds automatic trace context extraction from
event.request.headersfor AppSync integration scenarios. This eliminates the need for customers to use customized extractors for RUM → AppSync → Lambda resolver flows where RUM-injected trace context is nested underevent["request"]["headers"].Problem
Previously, trace context from RUM was not propagated through AppSync to Lambda resolvers because the extension only checked:
event.headers(top-level event headers)For AppSync, the trace context is nested under
event.request.headers, which was not checked, causing traces to break at the AppSync boundary.Solution
Added a step that checks
event.request.headersafter all other trace context sources have been attempted.Trace Examples
Before
Without this change, trace context is lost at the AppSync boundary:
Trace showing broken propagation through AppSync
Log showing the passed header value for x-datadog-trace-id does not align with the given trace_id
Flow:
RUM → AppSync → Lambda(trace ID not propagated)After
With this change, trace context is properly extracted and propagated:
Trace showing successful propagation through AppSync
Flow:
RUM → AppSync → Lambda resolver(trace continues using value within x-datadog-trace-id)Testing
Unit Tests
Added 10 unit tests covering:
event.request.headerswith Datadog headersevent.request.headerswith W3C TraceContext headersRun tests with:
cargo test test_extract_span_context -- --nocapture