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

Add optional inner samplers to make ParentBasedSampler more customizable #1727

Merged
merged 21 commits into from
Feb 27, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
d7b8101
WIP for ParentBasedSampler.
mbakalov Jan 26, 2021
0348bc4
Merge remote-tracking branch 'upstream/master' into parentbasedsampler
mbakalov Jan 27, 2021
c0e1f81
Better comments, tests for null arguments.
mbakalov Jan 27, 2021
8c7421d
Merge remote-tracking branch 'upstream/master' into parentbasedsampler
mbakalov Jan 27, 2021
92d174b
Remove unnecessary remark, left it there by mistake.
mbakalov Jan 27, 2021
684bc48
Merge branch 'master' into parentbasedsampler
cijothomas Jan 27, 2021
aee90b1
Merge branch 'main' into parentbasedsampler
cijothomas Jan 29, 2021
33046b8
Merge remote-tracking branch 'upstream/main' into parentbasedsampler
mbakalov Feb 5, 2021
509c26b
Merge remote-tracking branch 'upstream/main' into parentbasedsampler
mbakalov Feb 5, 2021
f42ccd7
Merge branch 'parentbasedsampler' of https://github.com/mbakalov/open…
mbakalov Feb 5, 2021
4198efb
Merge remote-tracking branch 'upstream/main' into parentbasedsampler
mbakalov Feb 12, 2021
d398331
Make other samplers optional
mbakalov Feb 14, 2021
cbc8595
Change to new delegating behavior.
mbakalov Feb 16, 2021
d0c9301
Merge remote-tracking branch 'upstream/main' into parentbasedsampler
mbakalov Feb 25, 2021
a206fcb
Update CHANGELOG
mbakalov Feb 25, 2021
495b288
Merge branch 'main' into parentbasedsampler
cijothomas Feb 25, 2021
d52be32
Merge branch 'main' into parentbasedsampler
cijothomas Feb 25, 2021
833b384
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
mbakalov Feb 25, 2021
973a9c4
Comment to note default values for inner samplers.
mbakalov Feb 25, 2021
cedeb86
Merge branch 'parentbasedsampler' of https://github.com/mbakalov/open…
mbakalov Feb 25, 2021
14155b4
Merge branch 'main' into parentbasedsampler
cijothomas Feb 27, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
static OpenTelemetry.Trace.TracerProviderExtensions.ForceFlush(this OpenTelemetry.Trace.TracerProvider provider, int timeoutMilliseconds = -1) -> bool
OpenTelemetry.Trace.ParentBasedSampler.ParentBasedSampler(OpenTelemetry.Trace.Sampler rootSampler, OpenTelemetry.Trace.Sampler remoteParentSampled = null, OpenTelemetry.Trace.Sampler remoteParentNotSampled = null, OpenTelemetry.Trace.Sampler localParentSampled = null, OpenTelemetry.Trace.Sampler localParentNotSampled = null) -> void
static OpenTelemetry.Trace.TracerProviderExtensions.ForceFlush(this OpenTelemetry.Trace.TracerProvider provider, int timeoutMilliseconds = -1) -> bool
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
OpenTelemetry.Trace.ParentBasedSampler.ParentBasedSampler(OpenTelemetry.Trace.Sampler rootSampler, OpenTelemetry.Trace.Sampler remoteParentSampled = null, OpenTelemetry.Trace.Sampler remoteParentNotSampled = null, OpenTelemetry.Trace.Sampler localParentSampled = null, OpenTelemetry.Trace.Sampler localParentNotSampled = null) -> void
static OpenTelemetry.Trace.TracerProviderExtensions.ForceFlush(this OpenTelemetry.Trace.TracerProvider provider, int timeoutMilliseconds = -1) -> bool
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
OpenTelemetry.Trace.ParentBasedSampler.ParentBasedSampler(OpenTelemetry.Trace.Sampler rootSampler, OpenTelemetry.Trace.Sampler remoteParentSampled = null, OpenTelemetry.Trace.Sampler remoteParentNotSampled = null, OpenTelemetry.Trace.Sampler localParentSampled = null, OpenTelemetry.Trace.Sampler localParentNotSampled = null) -> void
static OpenTelemetry.Trace.TracerProviderExtensions.ForceFlush(this OpenTelemetry.Trace.TracerProvider provider, int timeoutMilliseconds = -1) -> bool
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
OpenTelemetry.Trace.ParentBasedSampler.ParentBasedSampler(OpenTelemetry.Trace.Sampler rootSampler, OpenTelemetry.Trace.Sampler remoteParentSampled = null, OpenTelemetry.Trace.Sampler remoteParentNotSampled = null, OpenTelemetry.Trace.Sampler localParentSampled = null, OpenTelemetry.Trace.Sampler localParentNotSampled = null) -> void
static OpenTelemetry.Trace.TracerProviderExtensions.ForceFlush(this OpenTelemetry.Trace.TracerProvider provider, int timeoutMilliseconds = -1) -> bool
75 changes: 70 additions & 5 deletions src/OpenTelemetry/Trace/ParentBasedSampler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,23 @@
namespace OpenTelemetry.Trace
{
/// <summary>
/// Sampler implementation which will take a sample if parent Activity or any linked Activity is sampled.
/// Sampler implementation which by default will take a sample if parent Activity or any linked Activity is sampled.
/// Otherwise, samples root traces according to the specified root sampler.
/// </summary>
/// <remarks>
/// The default behavior can be customized by providing additional samplers to be invoked for different
/// combinations of local/remote parent and its sampling decision.
/// See <see cref="ParentBasedSampler(Sampler, Sampler, Sampler, Sampler, Sampler)"/>.
/// </remarks>
public sealed class ParentBasedSampler : Sampler
{
private readonly Sampler rootSampler;

private readonly Sampler remoteParentSampled;
private readonly Sampler remoteParentNotSampled;
private readonly Sampler localParentSampled;
private readonly Sampler localParentNotSampled;

/// <summary>
/// Initializes a new instance of the <see cref="ParentBasedSampler"/> class.
/// </summary>
Expand All @@ -35,6 +45,46 @@ public ParentBasedSampler(Sampler rootSampler)
this.rootSampler = rootSampler ?? throw new ArgumentNullException(nameof(rootSampler));

this.Description = $"ParentBased{{{rootSampler.Description}}}";

this.remoteParentSampled = new AlwaysOnSampler();
this.remoteParentNotSampled = new AlwaysOffSampler();
this.localParentSampled = new AlwaysOnSampler();
this.localParentNotSampled = new AlwaysOffSampler();
}

/// <summary>
/// Initializes a new instance of the <see cref="ParentBasedSampler"/> class with ability to delegate
/// sampling decision to one of the inner samplers provided.
/// </summary>
/// <param name="rootSampler">The <see cref="Sampler"/> to be called for root span/activity.</param>
/// <param name="remoteParentSampled">
/// A <see cref="Sampler"/> to delegate sampling decision to in case of
/// remote parent (<see cref="ActivityContext.IsRemote"/> == true) with <see cref="ActivityTraceFlags.Recorded"/> flag == true.
/// </param>
/// <param name="remoteParentNotSampled">
/// A <see cref="Sampler"/> to delegate sampling decision to in case of
/// remote parent (<see cref="ActivityContext.IsRemote"/> == true) with <see cref="ActivityTraceFlags.Recorded"/> flag == false.
/// </param>
/// <param name="localParentSampled">
/// A <see cref="Sampler"/> to delegate sampling decision to in case of
/// local parent (<see cref="ActivityContext.IsRemote"/> == false) with <see cref="ActivityTraceFlags.Recorded"/> flag == true.
/// </param>
/// <param name="localParentNotSampled">
/// A <see cref="Sampler"/> to delegate sampling decision to in case of
/// local parent (<see cref="ActivityContext.IsRemote"/> == false) with <see cref="ActivityTraceFlags.Recorded"/> flag == false.
/// </param>
public ParentBasedSampler(
Sampler rootSampler,
Sampler remoteParentSampled = null,
Sampler remoteParentNotSampled = null,
Sampler localParentSampled = null,
Sampler localParentNotSampled = null)
: this(rootSampler)
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
{
this.remoteParentSampled = remoteParentSampled ?? new AlwaysOnSampler();
this.remoteParentNotSampled = remoteParentNotSampled ?? new AlwaysOffSampler();
this.localParentSampled = localParentSampled ?? new AlwaysOnSampler();
this.localParentNotSampled = localParentNotSampled ?? new AlwaysOffSampler();
}

/// <inheritdoc />
Expand All @@ -47,10 +97,17 @@ public override SamplingResult ShouldSample(in SamplingParameters samplingParame
return this.rootSampler.ShouldSample(samplingParameters);
}

// If the parent is sampled keep the sampling decision.
// Is parent sampled?
if ((parentContext.TraceFlags & ActivityTraceFlags.Recorded) != 0)
{
return new SamplingResult(SamplingDecision.RecordAndSample);
if (parentContext.IsRemote)
{
return this.remoteParentSampled.ShouldSample(samplingParameters);
}
else
{
return this.localParentSampled.ShouldSample(samplingParameters);
}
}

if (samplingParameters.Links != null)
Expand All @@ -68,8 +125,16 @@ public override SamplingResult ShouldSample(in SamplingParameters samplingParame
}
}

// If parent was not sampled, do not sample.
return new SamplingResult(SamplingDecision.Drop);
// If parent was not sampled (and no linked context exists) => delegate to the "not sampled"
// inner samplers.
if (parentContext.IsRemote)
{
return this.remoteParentNotSampled.ShouldSample(samplingParameters);
}
else
{
return this.localParentNotSampled.ShouldSample(samplingParameters);
}
}
}
}
73 changes: 73 additions & 0 deletions test/OpenTelemetry.Tests/Trace/ParentBasedSamplerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Diagnostics;
using Moq;
using Xunit;

namespace OpenTelemetry.Trace.Tests
Expand Down Expand Up @@ -111,5 +114,75 @@ public void SampledParentLink()
kind: ActivityKind.Client,
links: sampledLink)));
}

[Theory]
[InlineData(true, true)]
[InlineData(true, false)]
[InlineData(false, true)]
[InlineData(false, false)]
public void CustomSamplers(bool parentIsRemote, bool parentIsSampled)
{
var mockRepository = new MockRepository(MockBehavior.Strict);
var remoteParentSampled = mockRepository.Create<Sampler>();
var remoteParentNotSampled = mockRepository.Create<Sampler>();
var localParentSampled = mockRepository.Create<Sampler>();
var localParentNotSampled = mockRepository.Create<Sampler>();

var samplerUnderTest = new ParentBasedSampler(
new AlwaysOnSampler(), // root
remoteParentSampled.Object,
remoteParentNotSampled.Object,
localParentSampled.Object,
localParentNotSampled.Object);

var samplingParams = this.MakeTestParameters(parentIsRemote, parentIsSampled);

Mock<Sampler> invokedSampler;
if (parentIsRemote && parentIsSampled)
{
invokedSampler = remoteParentSampled;
}
else if (parentIsRemote && !parentIsSampled)
{
invokedSampler = remoteParentNotSampled;
}
else if (!parentIsRemote && parentIsSampled)
{
invokedSampler = localParentSampled;
}
else
{
invokedSampler = localParentNotSampled;
}

var expectedResult = new SamplingResult(SamplingDecision.RecordAndSample);
invokedSampler.Setup(sampler => sampler.ShouldSample(samplingParams)).Returns(expectedResult);

var actualResult = samplerUnderTest.ShouldSample(samplingParams);

mockRepository.VerifyAll();
Assert.Equal(expectedResult, actualResult);
mockRepository.VerifyNoOtherCalls();
}

[Fact]
public void DisallowNullRootSampler()
{
Assert.Throws<ArgumentNullException>(() => new ParentBasedSampler(null));
}

private SamplingParameters MakeTestParameters(bool parentIsRemote, bool parentIsSampled)
{
return new SamplingParameters(
parentContext: new ActivityContext(
ActivityTraceId.CreateRandom(),
ActivitySpanId.CreateRandom(),
parentIsSampled ? ActivityTraceFlags.Recorded : ActivityTraceFlags.None,
null,
parentIsRemote),
traceId: default,
name: "Span",
kind: ActivityKind.Client);
}
}
}