RUM-6866 Allow typed Sampler#2385
Conversation
1e9cf52 to
4c2c9cf
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2385 +/- ##
===========================================
+ Coverage 70.28% 70.31% +0.02%
===========================================
Files 744 744
Lines 27733 27694 -39
Branches 4631 4621 -10
===========================================
- Hits 19492 19471 -21
+ Misses 6953 6947 -6
+ Partials 1288 1276 -12
|
|
|
||
| /** | ||
| * Creates a new instance of [RateBasedSampler] with the given sample rate. | ||
| * Creates a new instance lof [RateBasedSampler] with the given sample rate. |
| interface com.datadog.android.core.sampling.Sampler<T: Any> | ||
| fun sample(T): Boolean |
There was a problem hiding this comment.
this is potentially a binary-incompatible change, because public constructors of DatadogInterceptor and TracingInterceptor (and builder methods since recently) take Sampler instance which can be some customer-defined Sampler rather than default RateBasedSampler.
Is there a way to avoid breaking it?
There was a problem hiding this comment.
I hesitated to do it, as indeed it's technically a breaking change on a public interface. That being said, looking at our code base, the interface is used in:
- the
internal class TelemetryEventHandlerconstructor, but the sampler is built internally based on a sample rate provided in the configuration - the
internal class SessionReplayFeatureconstructor, but the sampler is built internally based on a sample rate provided in the configuration - the
internal class DatadogLogHandlerconstructor, but the sampler is built internally based on a sample rate provided in the configuration - an internal property of the
internal class WebViewLogEventConsumer, built internally based on a sample rate provided in the configuration - in the
DatadogInterceptorandTracingInterceptordeprecated constructors; they were replaced by a Builder. The builder only accepts a Sampler, which is the only place we could have ab issue for customer, which I guess is acceptable. I'll also add a builder method to set the sampling rate as I feel it's missing.
There was a problem hiding this comment.
🎯 suggestion: What about a keeping the builder method that accepts the Sampler and wrapping this old Sampler that the customer is passing to your own Sampler<Unit> ? In this case it will not be a breaking change anymore. And we could deprecate that method for later.
There was a problem hiding this comment.
will it make sense to make the API be like fun sample(T? = null): Boolean instead of fun sample(T): Boolean?
There was a problem hiding this comment.
I believe we probably don't want to allow nullable types in the interface definition. But probably even if we do so, it would be still binary incompatible change, because then customer implementing custom Sampler still need to update the method signature.
My main concern is indeed in the interceptors usage (other places are pretty much internal), but if we ultimately cannot avoid this and we need to progress on this topic, I'm fine with this minor breaking change.
4c2c9cf to
52ca276
Compare
| * By default it is a sampler accepting 20% of the traces. | ||
| */ | ||
| fun setTraceSampler(traceSampler: Sampler): R { | ||
| fun setTraceSampler(traceSampler: Sampler<Span>): R { |
There was a problem hiding this comment.
🎯 suggestion: Like I suggested in my previous comment maybe we can keep also the previous method and wrap the customer old Sampler implementation in our own Sampler<Span> which delegates to the customer. Not sure but I think is doable. We can also Deprecate the old method.
52ca276 to
808f3d7
Compare
| interface com.datadog.android.core.sampling.Sampler<T: Any> | ||
| fun sample(T): Boolean |
There was a problem hiding this comment.
I believe we probably don't want to allow nullable types in the interface definition. But probably even if we do so, it would be still binary incompatible change, because then customer implementing custom Sampler still need to update the method signature.
My main concern is indeed in the interceptors usage (other places are pretty much internal), but if we ultimately cannot avoid this and we need to progress on this topic, I'm fine with this minor breaking change.
5 participants
What does this PR do?
Preparing for a new implementation of a sampler for the TracingInterceptor, add a type to the sampler to allow adapting the sampling decision to the event being sampled.