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

Probability Sampler #136

Merged
merged 34 commits into from
Jul 14, 2020
Merged

Conversation

nholbrook
Copy link
Contributor

@nholbrook nholbrook commented Jun 30, 2020

Implements ProbabilitySampler based on the sampler interface in #118.

Relevant snippet from specification:

Probability
The default behavior should be to trust the parent SampledFlag. However there should be configuration to change this.
The default behavior is to apply the sampling probability only for Spans 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}.
TODO: Add details about how the probability sampler is implemented as a function of the TraceID. TODO: Split out the parent handling.

@nholbrook nholbrook force-pushed the nholbrook/probability-sampler branch from 0359e4c to 1097723 Compare June 30, 2020 16:17
@codecov
Copy link

codecov bot commented Jun 30, 2020

Codecov Report

Merging #136 into master will increase coverage by 0.40%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
+ Coverage   94.18%   94.59%   +0.40%     
==========================================
  Files          80       83       +3     
  Lines        1893     2034     +141     
==========================================
+ Hits         1783     1924     +141     
  Misses        110      110              
Impacted Files Coverage Δ
...ude/opentelemetry/sdk/trace/samplers/probability.h 100.00% <100.00%> (ø)
sdk/src/trace/samplers/probability.cc 100.00% <100.00%> (ø)
sdk/test/trace/probability_sampler_test.cc 100.00% <100.00%> (ø)

sdk/include/opentelemetry/sdk/trace/probability_sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/probability_sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/probability_sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
sdk/src/trace/probability_sampler.cc Outdated Show resolved Hide resolved
sdk/src/trace/probability_sampler.cc Outdated Show resolved Hide resolved
sdk/src/trace/probability_sampler.cc Outdated Show resolved Hide resolved
sdk/src/trace/probability_sampler.cc Outdated Show resolved Hide resolved
sdk/src/trace/probability_sampler.cc Outdated Show resolved Hide resolved
sdk/test/trace/probability_sampler_test.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

I think it would make sense to simplify this for the first go, and then discuss what of the SHOULD parts of the spec we want to add:

  • We can get rid of SamplingBehavior and just implement the default DETACHED_SPANS_ONLY logic.
  • We can get rid of defer_parent and just implement the default defer_parent = true.

If deemed necessary, we can add those parts in separate PRs (no of the other SDKs I looked at have this implemented now). This way we can reduce complexity.

You can refer to some other implementations:
Go
Java
Python

sdk/src/trace/samplers/probability.cc Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/samplers/probability.h Outdated Show resolved Hide resolved
sdk/src/trace/samplers/probability.cc Outdated Show resolved Hide resolved
sdk/src/trace/samplers/probability.cc Outdated Show resolved Hide resolved
sdk/src/trace/samplers/probability.cc Outdated Show resolved Hide resolved
sdk/test/trace/probability_sampler_test.cc Outdated Show resolved Hide resolved
sdk/src/trace/samplers/probability.cc Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/samplers/probability.h Outdated Show resolved Hide resolved
@nholbrook nholbrook force-pushed the nholbrook/probability-sampler branch from 84f5801 to a03c841 Compare July 6, 2020 13:55
@nholbrook nholbrook force-pushed the nholbrook/probability-sampler branch from a03c841 to b10f67d Compare July 8, 2020 13:51
@nholbrook nholbrook marked this pull request as ready for review July 8, 2020 16:30
@nholbrook nholbrook requested a review from a team July 8, 2020 16:30
@nholbrook
Copy link
Contributor Author

The current implementation of this Sampler handles invalid parameter values (i.e. any probability out of the bounds [0, 1]) silently. The intended functionality is to raise an exception to alert the user, however, this is currently not allowed by CI tests (see #156). This PR can be merged now and the error handing can be added in a later PR, or this will have to wait for #156 to be resolved.

sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
sdk/src/trace/samplers/probability.cc Outdated Show resolved Hide resolved
sdk/test/trace/probability_sampler_test.cc Outdated Show resolved Hide resolved
sdk/test/trace/probability_sampler_test.cc Outdated Show resolved Hide resolved
sdk/test/trace/probability_sampler_test.cc Outdated Show resolved Hide resolved
sdk/test/trace/probability_sampler_test.cc Outdated Show resolved Hide resolved
@nholbrook nholbrook force-pushed the nholbrook/probability-sampler branch from e6629b6 to d930347 Compare July 8, 2020 20:08
sdk/src/trace/samplers/probability.cc Show resolved Hide resolved
sdk/test/trace/probability_sampler_test.cc Outdated Show resolved Hide resolved
sdk/test/trace/probability_sampler_test.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

This looks good, I added some thoughts.

sdk/test/trace/probability_sampler_test.cc Outdated Show resolved Hide resolved
sdk/test/trace/sampler_benchmark.cc Outdated Show resolved Hide resolved
@nholbrook nholbrook force-pushed the nholbrook/probability-sampler branch from c4dd785 to a1470b0 Compare July 14, 2020 13:38
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@reyang
Copy link
Member

reyang commented Jul 14, 2020

@nholbrook would you please rebase? Thanks.

@nholbrook nholbrook force-pushed the nholbrook/probability-sampler branch from a1470b0 to a788576 Compare July 14, 2020 23:20
@nholbrook
Copy link
Contributor Author

@reyang Rebased

@reyang reyang merged commit ab2a5c1 into open-telemetry:master Jul 14, 2020
@reyang reyang mentioned this pull request Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants