-
Notifications
You must be signed in to change notification settings - Fork 840
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
Fix ottracepropagation for short span ids #6734
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6734 +/- ##
=========================================
Coverage 90.06% 90.06%
- Complexity 6457 6464 +7
=========================================
Files 718 718
Lines 19511 19533 +22
Branches 1922 1924 +2
=========================================
+ Hits 17572 17593 +21
Misses 1350 1350
- Partials 589 590 +1 ☔ View full report in Codecov by Sentry. |
hi @jack-berg can you please review this one. Thank you :) |
String spanId = | ||
incomingSpanId == null | ||
? SpanId.getInvalid() | ||
: StringUtils.padLeft(incomingSpanId, MAX_SPAN_ID_LENGTH); |
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 you add unit tests for this? Thanks.
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.
done. added unit tests. Thanks for review.
Hey @viveksing --
I assume that you just meant I looked at the w3c spec again and now I'm wondering what the scenario is in which you witnessed or somehow created 15 character (15 hex nibbles) span IDs? Do you have any idea of how that was generated? The w3c spec is pretty clear about saying that trace IDs This 15 smells funny to me, and leads me to believe that something out there is purposefully trimming leading zeros from the string representation...and it'd be nice to understand what that something is. |
@breedx-splk thanks for your comments :) So far I have seen this issue while using skipper proxy before Java applications. It may be the case that skipper is trimming the leading zeros from spanId before propagating them via headers. I tried testing the issue locally and adding trimmed zero's back in spanId fixes the problem of losing span context. Thanks for sharing the w3c specs. There was one statement in the section Interoperating with existing systems which use shorter identifiers
After reading the above line I felt maybe we can try to fix the spanId's too? or sorry if I may have misinterpreted it and the transformations only apply for traceId's? |
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.
Looks good to me.
@viveksing nah I think you're good...I failed to realize that the "OT" here is for OpenTracing, which I guess doesn't use the same W3C propagator, but it's own thing. Sorry if I've added any confusion, this change seems fine to me. Thanks. |
OtTracePropagator drops the parent context if spanId is shorter than 16 bytes while checking spanContext.isValid() . There can be cases when the spanId is 15 bytes and these id's should be padded with 0's to avoid losing parent trace/span. The pr applies this fix.
Example screenshot