Add optional inner samplers to make ParentBasedSampler more customizable #1727
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1727 +/- ##
==========================================
+ Coverage 83.77% 83.99% +0.21%
==========================================
Files 187 187
Lines 5967 5984 +17
==========================================
+ Hits 4999 5026 +27
+ Misses 968 958 -10
|
|
@mbakalov Given we are very close to 1.0, I'll hold off from this PR for 1.0, so that we can take more time and review. |
Will come back to this this week. |
|
Changed the API a bit to make additional samplers optional parameters in the new constructor. The compiler's overload resolution prefers the first constructor when no other parameters are provided, so the existing code will still compile and work as expected. public ParentBasedSampler(Sampler rootSampler)
{
}
public ParentBasedSampler(
Sampler rootSampler,
Sampler remoteParentSampled = null,
Sampler remoteParentNotSampled = null,
Sampler localParentSampled = null,
Sampler localParentNotSampled = null)
: this(rootSampler)
{
}We can do things like I'd also done some benchmarking and surprisingly the version that always delegates to inner samplers didn't turn out to be slower at all! Looks like all the inner sampler calls are getting inlined anyway. I've no idea why the delegating version is even a tiny bit faster with a slightly smaller code size too - perhaps I am doing something wrong? Finally, I've kept the current behavior of "always sample if a linked activity was sampled". To retain current semantics, it comes before the "parent not sampled" delegation, which is probably unexpected for the user. Should we provide a Switching this back to "ready for review" for more feedback 😄 |
|
Added and cleaned up the benchmarking code I used to here: [Benchmark(Baseline = true)]
public void ShouldSample_Original()
{
this.sampler.ShouldSample_Original(this.remoteParentSampledParams);
// this.sampler.ShouldSample_Original(this.localParentSampledParams);
// this.sampler.ShouldSample_Original(this.remoteParentNotSampledParams);
// this.sampler.ShouldSample_Original(this.localParentNotSampledParams);
}
[Benchmark]
public void ShouldSample_Delegating()
{
this.sampler.ShouldSample(this.remoteParentSampledParams);
// this.delegatingSampler.ShouldSample_Delegating(this.localParentSampledParams);
// this.delegatingSampler.ShouldSample_Delegating(this.remoteParentNotSampledParams);
// this.delegatingSampler.ShouldSample_Delegating(this.localParentNotSampledParams);
}BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18363.1379 (1909/November2018Update/19H2)
Intel Core i7-8650U CPU 1.90GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.103
[Host] : .NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X64 RyuJIT
DefaultJob : .NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X64 RyuJITThe public SamplingResult ShouldSample_Original(in SamplingParameters samplingParameters)
{
var parentContext = samplingParameters.ParentContext;
if (parentContext.TraceId == default)
{
// If no parent, use the rootSampler to determine sampling.
return this.rootSampler.ShouldSample(samplingParameters);
}
// If the parent is sampled keep the sampling decision.
if ((parentContext.TraceFlags & ActivityTraceFlags.Recorded) != 0)
{
return new SamplingResult(SamplingDecision.RecordAndSample);
}
if (samplingParameters.Links != null)
{
// If any linked context is sampled keep the sampling decision.
// TODO: This is not mentioned in the spec.
// Follow up with spec to see if context from Links
// must be used in ParentBasedSampler.
foreach (var parentLink in samplingParameters.Links)
{
if ((parentLink.Context.TraceFlags & ActivityTraceFlags.Recorded) != 0)
{
return new SamplingResult(SamplingDecision.RecordAndSample);
}
}
}
// If parent was not sampled, do not sample.
return new SamplingResult(SamplingDecision.Drop);
} |
|
@cijothomas - just noticed a comment about links. I can remove as part of this PR. Will make an update later today! |
Could you do a standalone PR for that alone to make it easy for tracking purposes :) |
|
Leaving unmerged for 1 more day to give other reviewers a chance to raise concerns. |
…y-dotnet into parentbasedsampler
…telemetry-dotnet into parentbasedsampler
Fixes #1710.
Changes
According to the spec the
ParentBasedSamplershould support optional parameters to allow delegating sampling decision for cases when remote/local parent was/wasn't sampled.Added a new constructor to
ParentBasedSamplerthat takes 4 moreSamplersas arguments.The spec still doesn't say anything about linked spans, so I left that part in.
CHANGELOG.mdupdated for non-trivial changes