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

[AsakerMohd]

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, https://github.com/open-telemetry/opentelemetry-specification/issues/1413#issuecomment-1247145651, 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

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

[cijothomas]

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]

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]

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]

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.

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions[bot]]

Closed as inactive. Feel free to reopen if this PR is still being worked on.