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.

Sign up for GitHub

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

Jump to bottom

Rename ProbabilitySampler to TraceIdRatioBasedSampler and add requirements #611

Merged
merged 3 commits into from
Aug 21, 2020
Merged

Rename ProbabilitySampler to TraceIdRatioBasedSampler and add requirements #611

merged 3 commits into from
Aug 21, 2020
+20 −7

Conversation

bogdandrutu
Copy link
Member

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

This was referenced May 19, 2020
Closed
Closed
@garthk garthk mentioned this pull request May 20, 2020
Open
@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.

yurishkuro
yurishkuro reviewed May 21, 2020
View reviewed changes
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
@lmolkova lmolkova mentioned this pull request Jul 30, 2020
Closed
@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Aug 12, 2020
@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
arminru
arminru approved these changes Aug 21, 2020
View reviewed changes
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
@bogdandrutu
Add requirements for probability sampler
c819f17 
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
arminru
arminru reviewed Aug 21, 2020
View reviewed changes
specification/trace/sdk.md
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
@arminru
Add changelog entry for open-telemetry#611
84e484b
@dyladan dyladan mentioned this pull request Aug 21, 2020
Closed
bogdandrutu pushed a commit that referenced this pull request Aug 21, 2020
@arminru
Add changelog entry for #611 (#853)
3e564a1
@jtescher jtescher mentioned this pull request Aug 21, 2020
Closed
@jkwatson jkwatson mentioned this pull request Aug 21, 2020
Closed
@dengliming dengliming mentioned this pull request Aug 22, 2020
Closed
@fbogsany fbogsany mentioned this pull request Aug 24, 2020
Merged
@reyang reyang mentioned this pull request Aug 24, 2020
Closed
@MrAlias MrAlias mentioned this pull request Aug 24, 2020
Closed
@lzchen lzchen mentioned this pull request Aug 25, 2020
Closed
@evantorrie evantorrie mentioned this pull request Sep 2, 2020
Merged
@bobstrecansky bobstrecansky mentioned this pull request Sep 15, 2020
Closed
@prondubuisi prondubuisi mentioned this pull request Sep 19, 2020
Merged
@Oberon00 Oberon00 mentioned this pull request Nov 4, 2020
Closed
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@bogdandrutu
Add requirements for probability sampler (open-telemetry#611)
18d6714 
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@arminru
Add changelog entry for open-telemetry#611 (open-telemetry#853)
a9c7d0f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arminru arminru arminru approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

@lizthegrey lizthegrey lizthegrey approved these changes

@yurishkuro yurishkuro yurishkuro approved these changes

@c24t c24t Awaiting requested review from c24t

@iredelmeier iredelmeier Awaiting requested review from iredelmeier

@jmacd jmacd Awaiting requested review from jmacd

@reyang reyang Awaiting requested review from reyang

@SergeyKanzhelev SergeyKanzhelev Awaiting requested review from SergeyKanzhelev

@tedsuo tedsuo Awaiting requested review from tedsuo

@tigrannajaryan tigrannajaryan Awaiting requested review from tigrannajaryan

Assignees

@yurishkuro yurishkuro

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@bogdandrutu @reyang @lizthegrey @carlosalberto @yurishkuro @arminru @SergeyKanzhelev @jmacd