Map x-ray key/value pairs to baggage. #3106
Conversation
anuraaga
requested review from
arminru,
bogdandrutu,
carlosalberto,
jkwatson,
Oberon00,
pavolloffay,
thisthat and
tylerbenson
as code owners
|
@willarmiros Please review too :) |
extensions/aws/src/main/java/io/opentelemetry/extension/aws/AwsXrayPropagator.java
Show resolved
Hide resolved
extensions/aws/src/main/java/io/opentelemetry/extension/aws/AwsXrayPropagator.java
Outdated
Show resolved
Hide resolved
extensions/aws/src/test/java/io/opentelemetry/extension/aws/AwsXrayPropagatorTest.java
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #3106 +/- ##
===========================================
+ Coverage 0 90.86% +90.86%
- Complexity 0 2884 +2884
===========================================
Files 0 329 +329
Lines 0 9024 +9024
Branches 0 907 +907
===========================================
+ Hits 0 8200 +8200
- Misses 0 544 +544
- Partials 0 280 +280
Continue to review full report at Codecov.
|
| @@ -190,14 +210,21 @@ public static AwsXrayPropagator getInstance() { | |||
| + "' with value " | |||
| + traceHeader | |||
| + "'. Returning INVALID span context."); | |||
There was a problem hiding this comment.
Is this log message still correct to say we're returning INVALID context? It is returning an invalid context, but is it returning the invalid context?
There was a problem hiding this comment.
Thanks, just removed that bit since it's sort of an implementation detail, logging the actual problem is what matters.
| if (baggage == null) { | ||
| baggage = Baggage.builder(); | ||
| } | ||
| baggage.put(trimmedPart.substring(0, equalsIndex), value.trim()); |
There was a problem hiding this comment.
So interestingly the X-Ray Trace header spec has a limit of 256 bytes on incoming arbitrary key-value pairs after the defined fields (baggage). This is not publicly documented, and is only implemented in the Node SDK. So clearly it's not critical to matching the behavior of the X-Ray SDKs, but I think it's still worth calling out since a limit on the baggage size is a safe thing to do to prevent malicious headers from stalling a program. Would be interested to hear your thoughts/if there are any existing considerations for this in OTel.
There was a problem hiding this comment.
Thanks for calling this out! The interesting point is to compare to w3c baggage
https://w3c.github.io/baggage/#limits
It's limit is 8192 - it's different but I don't see a big reason for that to affect how xray propagator itself works given it's a different spec. And smaller can be increased to larger later if we ever needed to. Will add the limit.
There was a problem hiding this comment.
Awesome! Good to know about the w3c limits. LGTM.
| @@ -190,14 +210,21 @@ public static AwsXrayPropagator getInstance() { | |||
| + "' with value " | |||
| + traceHeader | |||
| + "'. Returning INVALID span context."); | |||
There was a problem hiding this comment.
Thanks, just removed that bit since it's sort of an implementation detail, logging the actual problem is what matters.
No description provided.