Skip to content
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

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

viveksing
Copy link
Contributor

@viveksing viveksing commented Sep 20, 2024

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
Screenshot from 2024-09-20 16-48-11

@viveksing viveksing requested a review from a team as a code owner September 20, 2024 15:11
Copy link

linux-foundation-easycla bot commented Sep 20, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.06%. Comparing base (de13ff1) to head (30fb76b).
Report is 10 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@viveksing
Copy link
Contributor Author

hi @jack-berg can you please review this one. Thank you :)

String spanId =
incomingSpanId == null
? SpanId.getInvalid()
: StringUtils.padLeft(incomingSpanId, MAX_SPAN_ID_LENGTH);
Copy link
Member

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.

Copy link
Contributor Author

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.

@breedx-splk
Copy link
Contributor

Hey @viveksing --
First, lemme just say that I appreciate this contribution and the fact that this will attempt to improve interop with other tracing systems that might not be behaving entirely correctly.

OtTracePropagator drops the parent context if spanId is shorter than 16 bits

I assume that you just meant bytes here and not bits. :)

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 SHOULD be padded with zeros if generated by something with less precision...but I didn't see anything specifically mentioned about span ids (or parent span 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.

@viveksing
Copy link
Contributor Author

viveksing commented Sep 26, 2024

@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

Similar transformations are expected when tracing system converts other distributed trace context propagation formats to W3C Trace Context. 

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?

Copy link
Contributor

@breedx-splk breedx-splk left a 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.

@breedx-splk
Copy link
Contributor

@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.

@jack-berg jack-berg merged commit 46b28eb into open-telemetry:main Sep 30, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants