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

Add the requirement for the probability sampler #570

Closed
wants to merge 3 commits into from
Closed

Add the requirement for the probability sampler #570

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f84c55
Add the requirement for the probability sampler to include a span att…
Apr 20, 2020
e833c3c
Update specification/trace/sdk.md
jkwatson Apr 21, 2020
efb7376
shorten line lengths to 80
Apr 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
4 changes: 4 additions & 0 deletions specification/trace/sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@ These are the default samplers implemented in the OpenTelemetry SDK:
that are root spans (no parent) and Spans with remote parent. However there
should be configuration to change this to "root spans only", or "all spans".
* Description MUST be `ProbabilitySampler{0.000100}`.
* If the probability is applied (as opposed to using the parent's
Copy link
Member

Choose a reason for hiding this comment

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

It's surprising that this is a MUST since downstream tracers don't typically need to know the parent's sampling procedure, just its decision. This looks more like a SHOULD-level semantic convention to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, we want to have the Probability Sampler work the same everywhere. So, if we want to go forward with this change (and I'm relying on @pavolloffay for the justification/need), then I think that we should require them to all behave the same way. Hence, the use of MUST.

Copy link
Member

Choose a reason for hiding this comment

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

@c24t this has no effect on downstream tracers, since the probability is merely recorded as span attribute, out of band.

`SampledFlag` value), the set of span Attributes returned in the result MUST
include an attribute with the key `sampling.probability` and a value of the
probability being used.
Copy link

Choose a reason for hiding this comment

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

sampling.probability would have the value that is calculated by the algorithm? It is not very clear what is value of the probability being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the concern. A probability-based sampler has a probability that it is configured to use. This is the sampling probability.


#### Probability Sampler algorithm

Expand Down