-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Sampling should recommend ParentBasedSampler with TraceIDRatioBasedSampler #5884
Comments
Thanks for raising this issue, @thomaschaaf. @open-telemetry/javascript-approvers, PTAL. Thanks! |
I wrote this originally. At the time, I chose not to focus on the combination of the two, but I think now it's fine to do both. Unfortunately, there is a downside to both behaviors:
The best way to have really coherent traces with sampling is to use tail-based sampling. But SDKs don't do that. At any rate, I think including ParentBased is probably less...weird? Surprising? Less common to have weird behavior? |
As per spec, the default sampler is |
Yes, JS does that by default if you don't change sampling in any way. However, since this is for documenting adjusting the sampling rate at the head, the suggestion is to do ParentBased(TraceIdRatioBased=foo)) |
Thanks for clarifying. yes, that makes sense. |
What needs to be changed?
The sampling page https://github.com/open-telemetry/opentelemetry.io/blob/main/content/en/docs/languages/js/sampling.md currently does not mention ParentBasedSampler. In my opinion the default should be ParentBasedSampler with TraceIDRatioBasedSampler as that is what most newcomers are looking for.
What is the name + path of the page that needs changed? content/en/docs/languages/js/sampling.md
The text was updated successfully, but these errors were encountered: