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

Updating handling of sample priority extraction #51

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

jlawrienyt
Copy link
Contributor

@jlawrienyt jlawrienyt commented Jun 26, 2024

This change updates the sample priority extraction logic to match what is provided in Datadog's SDKs. I.e. any positive integer value is treated as a true value, any 0 or negative integer value is treated as a false value. Non-integer values cause context extraction to fail with an error.

This partially addresses #17 in that extraction will now work correctly for values > 1; it does not propagate the non 0/1 values to any outgoing trace context.

@jlawrienyt jlawrienyt requested a review from tonglil as a code owner June 26, 2024 13:11
This change updates the sample priority extraction logic to match what
is provided in Datadog's SDKs.  I.e. any positive integer value is
treated as a true value, any 0 or negative integer value is treated as a
false value.  Non-integer values cause context extraction to fail with
an error.
@jlawrienyt
Copy link
Contributor Author

jlawrienyt commented Jun 26, 2024

For reference, here's how Datadog's SDK in go handles extraction of the sampling priority header

Copy link
Owner

@tonglil tonglil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a future refactor can update the constants to ints and call itoa where necessary.

@tonglil tonglil merged commit f97dc3d into tonglil:main Jun 26, 2024
1 check 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