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

Added sampler to Activity - v1 #683

Merged
merged 10 commits into from
May 21, 2020
6 changes: 6 additions & 0 deletions samples/Exporters/Console/TestConsoleActivity.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ internal static object Run(ConsoleActivityOptions options)
if (parent != null)
{
parent.DisplayName = "HttpIn DisplayName";

// IsAllDataRequested is equivalent of Span.IsRecording
if (parent.IsAllDataRequested)
{
parent.AddTag("expensive data", "This data is expensive to obtain. Avoid it if activity is not being recorded");
}
}

try
Expand Down
48 changes: 48 additions & 0 deletions src/OpenTelemetry/Trace/ActivitySampler.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// <copyright file="ActivitySampler.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>
using System.Collections.Generic;
using System.Diagnostics;

namespace OpenTelemetry.Trace
{
/// <summary>
/// Sampler to select data to be exported. This sampler executes before Activity object is created.
/// </summary>
public abstract class ActivitySampler
{
/// <summary>
/// Gets the sampler description.
/// </summary>
public abstract string Description { get; }

/// <summary>
/// Checks whether activity needs to be created and tracked.
/// </summary>
/// <param name="parentContext">Parent activity context. Typically taken from the wire.</param>
/// <param name="traceId">Trace ID of a activity to be created.</param>
/// <param name="spanId">Span ID of a activity to be created.</param>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whats the value of spanid here - the span/activity is not yet created, and will be done based on the outcome of this method.
Can't think of the use of spanid in a typical sampling algorithm, as new spanid is just random.
(With activity there is no option to know the spanid ahead of Activity creation, and no option to change spanid after creation).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC an OTel SDK should be able to generate the IDs before calling the sampler, and historically some distributed tracing systems actually used the span ID instead of the trace ID to make their decision.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a requirement, we need to address this,
Adding it here for tracking: #675 (comment)

cc: @tarekgh

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC an OTel SDK should be able to generate the IDs before calling the sampler, and historically some distributed tracing systems actually used the span ID instead of the trace ID to make their decision.

I am wondering how a SpanId (for a span which not created yet) will be used in the sampling decision? this looks weird to me as this Id never been used till the sampling time. do we expect the samplers recognize specific patterns of Span Ids to make the decision accordingly? I can understand it could make sense to use the Span Id for sampling if the span is already created but not if the span really not created at all.

@cijothomas maybe the trace id is interesting too here.

@noahfalk

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since traceid for parent, and the child-to-be-created are going to be the same, I dont see how traceid is an issue?
Unless parent is null?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reyang here and there I tried to dig something about this and I didn't see any PR/discussion mentioning the SpanID perhaps it was an oversight? Unless @SergeyKanzhelev has something to add we should propose a simple change of removing it to see if anyone complains :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far my archeology work traced back to OpenCensus 😆

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah 🤠 (they didn't have an Indiana Jones emoji but the cowboy has a hat 🤷‍♂️), it seems very much like OpenCensus - that said everything that I see is using traceId to make their decisions, now will be the moment to remove it before some "creative person" starts to depend on it :)

Copy link
Member

@reyang reyang May 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this topic to Specification SIG meeting 05/26/2020 agenda.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed during the SIG meeting and agreed to remove it from the spec.

open-telemetry/opentelemetry-specification#621

/// <param name="name"> Name (DisplayName) of the activity to be created. Note, that the name of the activity is settable.
/// So this name can be changed later and Sampler implementation should assume that.
/// Typical example of a name change is when <see cref="Activity"/> representing incoming http request
/// has a name of url path and then being updated with route name when routing complete.
/// </param>
/// <param name="activityKind">The kind of the Activity.</param>
/// <param name="tags">Initial set of Tags for the Activity being constructed.</param>
/// <param name="links">Links associated with the activity.</param>
/// <returns>Sampling decision on whether activity needs to be sampled or not.</returns>
public abstract SamplingResult ShouldSample(in ActivityContext parentContext, in ActivityTraceId traceId, in ActivitySpanId spanId, string name, ActivityKind activityKind, IEnumerable<KeyValuePair<string, string>> tags, IEnumerable<ActivityLink> links);
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
}
}
13 changes: 13 additions & 0 deletions src/OpenTelemetry/Trace/Configuration/OpenTelemetryBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ internal OpenTelemetryBuilder()

internal ActivityProcessorPipelineBuilder ProcessingPipeline { get; private set; }

internal ActivitySampler Sampler { get; private set; }

internal HashSet<string> ActivitySourceNames { get; private set; }

/// <summary>
Expand All @@ -52,6 +54,17 @@ public OpenTelemetryBuilder SetProcessorPipeline(Action<ActivityProcessorPipelin
return this;
}

/// <summary>
/// Configures sampler.
/// </summary>
/// <param name="sampler">Sampler instance.</param>
/// <returns>Returns <see cref="OpenTelemetryBuilder"/> for chaining.</returns>
public OpenTelemetryBuilder SetSampler(ActivitySampler sampler)
{
this.Sampler = sampler ?? throw new ArgumentNullException(nameof(sampler));
return this;
}

/// <summary>
/// Adds given activitysource name to the list of subscribed sources.
/// </summary>
Expand Down
38 changes: 36 additions & 2 deletions src/OpenTelemetry/Trace/Configuration/OpenTelemetrySdk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using System;
using System.Diagnostics;
using OpenTelemetry.Trace.Export;
using OpenTelemetry.Trace.Samplers;

namespace OpenTelemetry.Trace.Configuration
{
Expand All @@ -40,6 +41,8 @@ public static void EnableOpenTelemetry(Action<OpenTelemetryBuilder> configureOpe
var openTelemetryBuilder = new OpenTelemetryBuilder();
configureOpenTelemetryBuilder(openTelemetryBuilder);

ActivitySampler sampler = openTelemetryBuilder.Sampler ?? new AlwaysOnActivitySampler();

ActivityProcessor activityProcessor;
if (openTelemetryBuilder.ProcessingPipeline == null)
{
Expand All @@ -65,9 +68,40 @@ public static void EnableOpenTelemetry(Action<OpenTelemetryBuilder> configureOpe
// or not
ShouldListenTo = (activitySource) => openTelemetryBuilder.ActivitySourceNames.Contains(activitySource.Name.ToUpperInvariant()),

// The following parameters are not used now.
// The following parameter is not used now.
GetRequestedDataUsingParentId = (ref ActivityCreationOptions<string> options) => ActivityDataRequest.AllData,
GetRequestedDataUsingContext = (ref ActivityCreationOptions<ActivityContext> options) => ActivityDataRequest.AllData,

// This delegate informs ActivitySource about sampling decision.
// Following simple behavior is enabled now:
// If Sampler returns IsSampled as true, returns ActivityDataRequest.AllDataAndRecorded
// This creates Activity and sets its IsAllDataRequested to true.
// Library authors can check activity.IsAllDataRequested and avoid
// doing any additional telemetry population.
// Activity.IsAllDataRequested is the equivalent of Span.IsRecording
//
// If Sampler returns IsSampled as false, returns ActivityDataRequest.None
// This prevents Activity from being created at all.
GetRequestedDataUsingContext = (ref ActivityCreationOptions<ActivityContext> options) =>
{
var shouldSample = sampler.ShouldSample(
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
options.Parent,
options.Parent.TraceId,
default(ActivitySpanId), // Passing default SpanId here. The actual SpanId is not known before actual Activity creation
options.Name,
options.Kind,
options.Tags,
options.Links);
if (shouldSample.IsSampled)
{
return ActivityDataRequest.AllDataAndRecorded;
}
else
{
return ActivityDataRequest.None;
}

// TODO: Improve this to properly use ActivityDataRequest.AllData, PropagationData as well.
},
};

ActivitySource.AddActivityListener(listener);
Expand Down
35 changes: 35 additions & 0 deletions src/OpenTelemetry/Trace/Samplers/AlwaysOffActivitySampler.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// <copyright file="AlwaysOffActivitySampler.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>
using System.Collections.Generic;
using System.Diagnostics;

namespace OpenTelemetry.Trace.Samplers
{
/// <summary>
/// Sampler implementation which never samples any activity.
/// </summary>
public sealed class AlwaysOffActivitySampler : ActivitySampler
{
/// <inheritdoc />
public override string Description { get; } = nameof(AlwaysOffActivitySampler);

/// <inheritdoc />
public override SamplingResult ShouldSample(in ActivityContext parentContext, in ActivityTraceId traceId, in ActivitySpanId spanId, string name, ActivityKind activityKind, IEnumerable<KeyValuePair<string, string>> tags, IEnumerable<ActivityLink> links)
{
return new SamplingResult(false);
}
}
}
36 changes: 36 additions & 0 deletions src/OpenTelemetry/Trace/Samplers/AlwaysOnActivitySampler.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// <copyright file="AlwaysOnActivitySampler.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>
using System.Collections.Generic;
using System.Diagnostics;

namespace OpenTelemetry.Trace.Samplers
{
/// <summary>
/// Sampler implementation which samples every activity.
/// This sampler will be used as the default Sampler, if no other Sampler is configured.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this comment should be on the builder to increase visibility.

/// </summary>
public sealed class AlwaysOnActivitySampler : ActivitySampler
{
/// <inheritdoc />
public override string Description { get; } = nameof(AlwaysOnActivitySampler);

/// <inheritdoc />
public override SamplingResult ShouldSample(in ActivityContext parentContext, in ActivityTraceId traceId, in ActivitySpanId spanId, string name, ActivityKind activityKind, IEnumerable<KeyValuePair<string, string>> tags, IEnumerable<ActivityLink> links)
{
return new SamplingResult(true);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// <copyright file="ActivitySamplersTest.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>
using System.Collections.Generic;
using System.Diagnostics;
using Xunit;

namespace OpenTelemetry.Trace.Samplers.Test
{
public class ActivitySamplersTest
{
private static readonly ActivityKind ActivityKindServer = ActivityKind.Server;
private readonly ActivityTraceId traceId;
private readonly ActivitySpanId spanId;
private readonly ActivitySpanId parentSpanId;

public ActivitySamplersTest()
{
traceId = ActivityTraceId.CreateRandom();
spanId = ActivitySpanId.CreateRandom();
parentSpanId = ActivitySpanId.CreateRandom();
}

[Theory]
[InlineData(ActivityTraceFlags.Recorded)]
[InlineData(ActivityTraceFlags.None)]
public void AlwaysOnSampler_AlwaysReturnTrue(ActivityTraceFlags flags)
{
var parentContext = new ActivityContext(traceId, parentSpanId, flags);
var link = new ActivityLink(parentContext);

Assert.True(
new AlwaysOnActivitySampler()
.ShouldSample(
parentContext,
traceId,
spanId,
"Another name",
ActivityKindServer,
null,
new List<ActivityLink>() { link }).IsSampled);
}

[Fact]
public void AlwaysOnSampler_GetDescription()
{
// TODO: The name must be AlwaysOnSampler as per spec.
// We should correct it when we replace span sampler with this.
Assert.Equal("AlwaysOnActivitySampler", new AlwaysOnActivitySampler().Description);
}

[Theory]
[InlineData(ActivityTraceFlags.Recorded)]
[InlineData(ActivityTraceFlags.None)]
public void AlwaysOffSampler_AlwaysReturnFalse(ActivityTraceFlags flags)
{
var parentContext = new ActivityContext(traceId, parentSpanId, flags);
var link = new ActivityLink(parentContext);

Assert.False(
new AlwaysOffActivitySampler()
.ShouldSample(
parentContext,
traceId,
spanId,
"Another name",
ActivityKindServer,
null,
new List<ActivityLink>() { link }).IsSampled);
}

[Fact]
public void AlwaysOffSampler_GetDescription()
{
// TODO: The name must be AlwaysOffSampler as per spec.
// We should correct it when we replace span sampler with this.
Assert.Equal("AlwaysOffActivitySampler", new AlwaysOffActivitySampler().Description);
}
}
}