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

Changed TraceIdRatioBasedSampler.GetLowerLong to use the correct bytes for sampling decision #5707

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AsakerMohd
Copy link

@AsakerMohd AsakerMohd commented Jun 20, 2024

Changes

Changed TraceIdRatioBasedSampler.GetLowerLong to use the correct bytes for sampling decision. Previously it was using the first 8 bytes to base the sampling decision on. Byte 0 in this case is the most significant byte not the least significant byte. As a result, changed it to use bytes 8->16 as those are the actual LowerLong. I discovered this while testing the with XrayIds where the first 4 bytes aren't random (based on current timestamp). Looking at this issue, open-telemetry/opentelemetry-specification#1413 (comment), seems like we're supposed to use the last 8 bytes to maintain compatibility with 64-bit TraceIds since those are random. I also looked at the Python Implementation and the Java Implementaiton of the TraceIdRatioBasedSampler and they both also seem to be using the last 8 bytes (least significant 8 bytes to byte 8->15 where byte 0 is the most significant byte).

Testing:
I created a sample app and copied over the sampling code to verify that the bytes being used currently are the most significant bytes and this was the result:

This is traceID= 667493231b8d224048149519ddfa8382 (xray trace id)
this is byte 0 = 102
this is byte 1 = 116
this is byte 2 = 147
this is byte 3 = 35
this is byte 4 = 27
this is byte 5 = 141
this is byte 6 = 34
this is byte 7 = 64
This is traceID= 66749388460c471ecba96ae40d3bf1c9 (xray trace id)
this is byte 0 = 102
this is byte 1 = 116
this is byte 2 = 147
this is byte 3 = 136
this is byte 4 = 70
this is byte 5 = 12
this is byte 6 = 71
this is byte 7 = 30

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@AsakerMohd AsakerMohd requested a review from a team as a code owner June 20, 2024 21:26
@github-actions github-actions bot added the pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package label Jun 20, 2024
Copy link

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

@cijothomas
Copy link
Member

This could be a breaking change, even though the algorithm in TraceIdRatioSampler was never fixed and not spec-ed out either. Do you know if the spec issue for the same is worked on by anyone actively?

@AsakerMohd
Copy link
Author

This could be a breaking change, even though the algorithm in TraceIdRatioSampler was never fixed and not spec-ed out either. Do you know if the spec issue for the same is worked on by anyone actively?

Not that I'm aware of.

@vishweshbankwar
Copy link
Member

vishweshbankwar commented Jun 26, 2024

This could be a breaking change, even though the algorithm in TraceIdRatioSampler was never fixed and not spec-ed out either. Do you know if the spec issue for the same is worked on by anyone actively?

Not that I'm aware of.

@AsakerMohd - Looks like the spec already has TODO for this. I think we could just wait for that rather than changing it now only to adjust it again later. (if the final state is different than proposed in this PR).

@vishweshbankwar
Copy link
Member

This could be a breaking change, even though the algorithm in TraceIdRatioSampler was never fixed and not spec-ed out either. Do you know if the spec issue for the same is worked on by anyone actively?

Not that I'm aware of.

@AsakerMohd - Looks like the spec already has TODO for this. I think we could just wait for that rather than changing it now only to adjust it again later (if the final state is different than proposed in this PR).

I have added this for discussion for our next SIG meeting. https://docs.google.com/document/d/1yjjD6aBcLxlRazYrawukDgrhZMObwHARJbB9glWdHj8/edit

Feel free to join.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants