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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c819f17
Add requirements for probability sampler
bogdandrutu May 18, 2020
3ce2497
Merge branch 'master' into probrequirements
bogdandrutu Aug 21, 2020
3a70c51
Merge branch 'master' into probrequirements
bogdandrutu Aug 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions specification/trace/sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ It produces an output called `SamplingResult` which contains:

Returns the sampler name or short description with the configuration. This may
be displayed on debug pages or in the logs. Example:
`"ProbabilitySampler{0.000100}"`.
`"TraceIdRatioBased{0.000100}"`.

Description MUST NOT change over time and caller can cache the returned value.

Expand All @@ -109,16 +109,29 @@ The default sampler is `ParentBased(root=AlwaysOn)`.
* Returns `NOT_RECORD` always.
* Description MUST be `AlwaysOffSampler`.

#### Probability
#### TraceIdRatioBased

* The `ProbabilitySampler` MUST ignore the parent. To respect the parent
`SampledFlag`, the `ProbabilitySampler` should be used as a delegate of the
`ParentBased` sampler specified below.
* Description MUST be `ProbabilitySampler{0.000100}`.
* 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.
* Description MUST be `TraceIdRatioBased{0.000100}`.

TODO: Add details about how the `ProbabilitySampler` is implemented as a function
TODO: Add details about how the `TraceIdRatioBased` is implemented as a function
of the `TraceID`.

##### Requirements for `TraceIdRatioBased` sampler algorithm

* The sampling algorithm MUST be deterministic. A trace identified by a given
`TraceId` is sampled or not independent of language, time, etc. To achieve this,
implementations MUST use a deterministic hash of the `TraceId` when computing
the sampling decision. By ensuring this, running the sampler on any child `Span`
will produce the same decision.
* 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.
Comment on lines +127 to +131
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.


#### ParentBased

* This is a composite sampler. `ParentBased` helps distinguished between the
Expand Down