-
Notifications
You must be signed in to change notification settings - Fork 513
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
feat(propagator/aws-xray): Extract X-Ray header in a case-insensitive fashion #1328
Conversation
propagators/opentelemetry-propagator-aws-xray/src/AWSXRayPropagator.ts
Outdated
Show resolved
Hide resolved
b1b0e1a
to
451f626
Compare
const headerKeys = getter.keys(carrier); | ||
const relevantHeaderKey = headerKeys.find((e) => { | ||
return e.toLowerCase() === AWSXRAY_TRACE_ID_HEADER; | ||
}); | ||
if (!relevantHeaderKey) { | ||
return INVALID_SPAN_CONTEXT; | ||
} | ||
const traceHeader = getter.get(carrier, relevantHeaderKey); | ||
|
||
if (!traceHeader || typeof traceHeader !== 'string') { | ||
return INVALID_SPAN_CONTEXT; | ||
} |
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's better to implement that logic in a new proprietary TextMapGetter
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.
@osherv I don't think that's advisable.
If, as a platform engineer, I use the SDK to register an AWS XRay propagator, or a B3 propagator, that's my problem. It should be invisible to the API consumer.
The developer who then uses the API to extract or inject the context should only concern himself with the serialisation/deserialisation of the context into/from the payload. It should not be concerned whether or not the propagator is expecting a lowercase string or not. It can't know if the expected key should be lower case, upper case, camel-case, kebab-case, etc. That's the responsibly of the propagator.
If you stand your point, then the AWS Lambda instrumentation has to be modified to use case-independent header keys, because at the moment, using both appears to be incompatible.
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.
LGTM - the header should be case-insensitive
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1328 +/- ##
==========================================
- Coverage 96.08% 95.65% -0.43%
==========================================
Files 14 16 +2
Lines 893 1105 +212
Branches 191 230 +39
==========================================
+ Hits 858 1057 +199
- Misses 35 48 +13
|
@NPellet can you please fix the lint issue so CI is green and we can merge? Thanks |
Which problem is this PR solving?
We observed that we receive API Gateway triggers where the X-Ray header is in camel case (despite having injected() with the same propagator).
This PR allows the X-Ray propagator to extract from a object with a case-insensitive key.
Short description of the changes