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

Rename ProbabilitySampler to TraceIdRatioBasedSampler and add requirements #611

Merged
merged 3 commits into from
Aug 21, 2020

Conversation

bogdandrutu
Copy link
Member

In #331 there were questions about why different requirements are important when deciding on the probability sampling algorithm.

@reyang
Copy link
Member

reyang commented May 21, 2020

Do we want to mention security consideration (e.g what's the trust boundary)? E.g. a malicious guy could generate trace ids that always result in the trace being sampled even at extremely low rate.

This might be solved by starting a new trace across trust boundary, there are many other solutions, too.

@bogdandrutu
Copy link
Member Author

@reyang that is a good point, but I don't think it is in scope of this sampler. I think that is a more general problem and not only for having things sampled (because another way to do that is to always set the traceflag option to 1). There is a special section in w3c about security concerns.

specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
@jmacd jmacd added the area:sampling Related to trace sampling label May 29, 2020
@carlosalberto carlosalberto added spec:trace Related to the specification/trace directory area:sdk Related to the SDK labels Jun 19, 2020
@carlosalberto carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 7, 2020
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@bogdandrutu
Copy link
Member Author

will work on this soon, commenting to not close it

@lizthegrey
Copy link
Member

Thanks Bogdan & Yuri!

@bogdandrutu
Copy link
Member Author

@yurishkuro @lizthegrey PR updated.

@bogdandrutu bogdandrutu changed the title Add requirements for probability sampler Rename ProbabilitySampler to TraceIdRatioBasedSampler and add requirements Aug 19, 2020
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
Comment on lines +129 to +133
* A `TraceIdRatioBased` sampler with a given sampling rate MUST also sample all
traces that any `TraceIdRatioBased` sampler with a lower sampling rate would
sample. This is important when a backend system may want to run with a higher
sampling rate than the frontend system, this way all frontend traces will
still be sampled and extra traces will be sampled on the backend only.
Copy link
Member

Choose a reason for hiding this comment

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

This is only relevant for requests that cross trust boundaries because otherwise the sampled flag in the incoming TraceState is honored by the parent-based sampler, right? This could mentioned to avoid confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, we decided that the samplers like this will not honor any flag or TraceState, is the user responsibility to configure the ParentBased:

The TraceIdRatioBased MUST ignore the parent SampledFlag. To respect the parent SampledFlag, the TraceIdRatioBased should be used as a delegate of the ParentBased sampler specified below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Re-read, yes in general if parent-based is set it is not very important, but even if parent-based is not set this should be the same case.

@bogdandrutu bogdandrutu merged commit 69f1f68 into open-telemetry:master Aug 21, 2020
@bogdandrutu bogdandrutu deleted the probrequirements branch August 21, 2020 16:01
arminru added a commit to dynatrace-oss-contrib/opentelemetry-specification that referenced this pull request Aug 21, 2020
bogdandrutu pushed a commit that referenced this pull request Aug 21, 2020
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
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 release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants