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

Assume random trace ID and set th-field only for sampled spans #1278

Merged
merged 1 commit into from
May 8, 2024

Conversation

oertl
Copy link
Contributor

@oertl oertl commented Apr 16, 2024

This PR reflects the latest agreements of the sampling SIG:

  • assume least significant bits of trace ID are random by default
  • remove th-field if not sampled

Description:

< Describe what is being changed or added.
Ex. Bug fix - Describe the bug and how this fixes it.
Ex. Feature addition - Describe what this provides and why. >

Existing Issue(s):

< Link any applicable issues. >

Testing:

< Describe what testing was performed and any tests were added. >

Documentation:

< Describe the documentation added.
Please be sure to modify relevant existing documentation if needed. >

Outstanding items:

< Anything that these changes are intentionally missing
that will be needed or can be helpful in the future. >

@oertl oertl requested a review from a team April 16, 2024 19:11
@github-actions github-actions bot requested a review from PeterF778 April 16, 2024 19:12
Copy link
Contributor

@PeterF778 PeterF778 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!

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.

This is cool and really helps to clean a few things up along the way. Thanks! I do wonder if it's worth mentioning in the README that, under rare circumstances where someone has plugged in their own ID generator, they could possibly break consistent sampling.

There have been some fringe cases where users needed to bridge to an existing telemetry system while they were migrating to otel.

@laurit laurit merged commit 55ec98b into open-telemetry:main May 8, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants