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

Clarify TraceIdRatioBased Sampler description #1524

Closed
MrAlias opened this issue Mar 9, 2021 · 4 comments · Fixed by #1536
Closed

Clarify TraceIdRatioBased Sampler description #1524

MrAlias opened this issue Mar 9, 2021 · 4 comments · Fixed by #1536
Labels
area:sampling Related to trace sampling area:sdk Related to the SDK spec:trace Related to the specification/trace directory

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Mar 9, 2021

From the specification:

Description MUST be TraceIdRatioBased{0.000100}

Does this mean this sampler should always return the static string "TraceIdRatioBased{0.000100}" even if the ratio is not 0.000100?

Or should it return "TraceIdRatioBased{$RATIO}" with $RATIO replaced with a decimal representation of the sampling ratio? If so, what precision should this ratio be?

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 9, 2021

Survey of Existing Solutions

Go

"TraceIdRatioBased{$RATIO}" with $RATIO replaced with a decimal representation of the sampling ratio with the "smallest number of digits necessary to identify the value uniquely" level of precision.

https://github.com/open-telemetry/opentelemetry-go/blob/90bd4ab50c6e85743f2b8f3a1768a5afceeb1fc0/sdk/trace/sampling.go#L105

Java

"TraceIdRatioBased{$RATIO}" with $RATIO replaced with a decimal representation of the sampling ratio with 6 digits of precision.

https://github.com/open-telemetry/opentelemetry-java/blob/5c0ea253a53719774b16933191e8f8b1c46ff73d/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/samplers/TraceIdRatioBasedSampler.java#L84

Python

"TraceIdRatioBased{$RATIO}" with $RATIO replaced with a decimal representation of the sampling ratio with up to 17 digits of precision (I think?).

https://github.com/open-telemetry/opentelemetry-python/blob/489a81e5d33690ec10f2d7606c7137168f85c17a/opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py#L261

JS

"TraceIdRatioBased{$RATIO}" with $RATIO replaced with a decimal representation of the sampling ratio. I'm not sure on the precision.

https://github.com/open-telemetry/opentelemetry-js/blob/bf99144ad4e4fa38120e896d70e9e5bcfaf27054/packages/opentelemetry-core/src/trace/sampler/TraceIdRatioBasedSampler.ts#L40

.NET

"TraceIdRatioBased{$RATIO}" with $RATIO replaced with a decimal representation of the sampling ratio with 6 digits of precision (I think?).

https://github.com/open-telemetry/opentelemetry-dotnet/blob/21c440a284e2efe533a4fdf7c97c8978fbb6ecae/src/OpenTelemetry/Trace/TraceIdRatioBasedSampler.cs#L47

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 9, 2021

Based on what SIGs are doing (especially the ones that have a released a 1.0.0 version) it would follow the specification needs to be updated to reflect the description should be dynamic. E.g.

Description MUST return a string of the form `"TraceIdRatioBased{RATIO}"` with `RATIO` replaced with the Sampler instances' trace sampling ratio represented as a decimal number.
The precision of the number SHOULD follow language standards and SHOULD be high enough to identify when Samplers have different ratios.
For example, if a TraceIdRatioBased Sampler had a sampling ratio of 1 to every 10,000 spans it could return `"TraceIdRatioBased{0.000100}"` as its description.

@arminru
Copy link
Member

arminru commented Mar 11, 2021

I'm glad no one interpreted it as "it has to be this constant string regardless of the actual ratio" 😄

Thanks for digging up the status-quo and presenting it here, Tyler!
Your proposal sounds good to me. Could you please create a PR for that?

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 11, 2021

Your proposal sounds good to me. Could you please create a PR for that?

Sure thing 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sampling Related to trace sampling area:sdk Related to the SDK spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants