Add ProbabilitySampler in SDK. #337
Conversation
songy23
requested review from
bogdandrutu,
carlosalberto,
pavolloffay,
SergeyKanzhelev,
tylerbenson and
yurishkuro
as code owners
Codecov Report
@@ Coverage Diff @@
## master #337 +/- ##
============================================
- Coverage 62.54% 62.48% -0.06%
- Complexity 306 316 +10
============================================
Files 52 53 +1
Lines 1196 1213 +17
Branches 104 110 +6
============================================
+ Hits 748 758 +10
- Misses 411 414 +3
- Partials 37 41 +4
Continue to review full report at Codecov.
|
sdk/src/test/java/io/opentelemetry/sdk/trace/samplers/ProbabilitySamplerTest.java
Show resolved
Hide resolved
| * range. We convert an incoming probability into an upper bound on that value, such that we can | ||
| * just compare the absolute value of the id and the bound to see if we are within the desired | ||
| * probability range. Using the low bits of the traceId also ensures that systems that only use 64 | ||
| * bit ID's will also work with this sampler. |
There was a problem hiding this comment.
Nice comment. Is there any chance the assumption about the random distribution might fail?
There was a problem hiding this comment.
I cannot think of any - @bogdandrutu may have a better example here.
There was a problem hiding this comment.
This should be also documented into the specification + @c24t, because we need all the implementations to be consistent.
There was a problem hiding this comment.
Spec issue: open-telemetry/opentelemetry-specification#31.
|
Apologies I forgot to mention these two files are a copy-and-paste from https://github.com/census-instrumentation/opencensus-java/blob/master/api/src/main/java/io/opencensus/trace/samplers/ProbabilitySampler.java and https://github.com/census-instrumentation/opencensus-java/blob/master/api/src/test/java/io/opencensus/trace/samplers/SamplersTest.java. Previously we removed |
We had similar discussions about Jaeger defaults, and the feeling was that 100% would be a better default as new users often get confused why they are not getting traces out. |
|
To be precise, in Jaeger the default is "remotely controlled sampler" where the sampling strategy is loaded from Jaeger backend, but if the backend is not available then the default probability is still very low. |
| import javax.annotation.Nullable; | ||
| import javax.annotation.concurrent.Immutable; | ||
|
|
||
| /** |
There was a problem hiding this comment.
@since 0.1.0 is missing here.
There was a problem hiding this comment.
Seems none of the SDK classes have the @since tag. I'll wait to see our decision on #344.
Maybe the SDK could have modes, production and test. |
Agreed.
If we were to do this, we should also make sure |
|
We can discuss on which sampler to use as the default, but IMO we need to provide the probability sampler option even if it's not made default. WDYT? |
songy23
force-pushed
the
probability-sampler
branch
from
2e3eba2 to
d3d027e
Compare
|
I'm not authorized to merge this pull request - please help me merge it if it looks good :) |
|
@songy23 did you follow up with an issue on the specification about the probability algorithm to make sure we have this consistent? |
songy23
force-pushed
the
probability-sampler
branch
from
d3d027e to
4691abb
Compare
songy23
force-pushed
the
probability-sampler
branch
from
4691abb to
61e59d7
Compare
OpenCensus used to use probability sampler of 1/10000 as default in trace configs. Not sure if we want to keep it that way in the new project.