add a provider for consistent parent based probability sampler #1005
Conversation
Not sure why, but I am seeing this, am I pointing to a wrong branch?
|
|
|
||
| @Override | ||
| public String getName() { | ||
| return "consistent_parent_based_probability"; |
There was a problem hiding this comment.
samplers in java sdk use a different naming scheme https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/README.md#sampler
There was a problem hiding this comment.
Would consistent_parentbased_probability work?
There was a problem hiding this comment.
how about
consistent_always_onconsistent_always_offconsistent_probabilityconsistent_rate_limitparentbased_consistent_always_onparentbased_consistent_always_offparentbased_consistent_probabilityparentbased_consistent_rate_limit
(don't need to add them all in this PR, but trying to understand the naming pattern)
There was a problem hiding this comment.
Sounds good, updated to parentbased_consistent_probability
There was a problem hiding this comment.
could you also rename the sampler provider class to match the name
That is fine, only code owners can merge. |
|
|
||
| @Override | ||
| public Sampler createSampler(ConfigProperties config) { | ||
| double samplingProbability = config.getDouble("otel.traces.sampler.arg", 1.0d); |
There was a problem hiding this comment.
Would you want to force the samplingProbability into the interval [0.0, 1.0]? If out of the range, an exception will be thrown by the statement below.
There was a problem hiding this comment.
ConsistentProbabilityBasedSampler is already throwing an IllegalArgumentException if it's not within the range.
if (samplingProbability < 0.0 || samplingProbability > 1.0) {
throw new IllegalArgumentException("Sampling probability must be in range [0.0, 1.0]!");
}
There was a problem hiding this comment.
Exactly my point. I just wondered if this is the behavior you want.
There was a problem hiding this comment.
Sorry, I misread your comment.
Yes and I believe that was @oertl's intention as well.
TraceIdRatioBasedSampler in java sdk also has the same behaviour.
We could default to 1.0, but I think failing here would probably be more explicit.
Description:
Adding a ConfigurableSamplerProvider for parent based probability sampler to autoconfigure to use in auto instrumentation.
Existing Issue(s):
Discussed in here
#999
Testing:
Tested it by using it in a java application with auto instrumentation on.
Documentation:
Outstanding items:
It's only adding a provider for one sampler that I need, but I am happy to discuss if we should add them all.