-
Notifications
You must be signed in to change notification settings - Fork 778
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 Probability Sampler for Activity #702
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
// <copyright file="ActivitySamplingParameters.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> | ||
/// Sampling parameters passed to an <see cref="ActivitySampler"/> for it to make a sampling decision. | ||
/// </summary> | ||
public readonly struct ActivitySamplingParameters | ||
{ | ||
/// <summary> | ||
/// Initializes a new instance of the <see cref="ActivitySamplingParameters"/> struct. | ||
/// </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="name">The 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="kind">The kind of the Activity to be created.</param> | ||
/// <param name="tags">Initial set of Tags for the Activity being constructed.</param> | ||
/// <param name="links">Links associated with the activity.</param> | ||
public ActivitySamplingParameters( | ||
ActivityContext parentContext, | ||
ActivityTraceId traceId, | ||
string name, | ||
ActivityKind kind, | ||
IEnumerable<KeyValuePair<string, string>> tags = null, // TODO: Empty | ||
IEnumerable<ActivityLink> links = null) | ||
{ | ||
this.ParentContext = parentContext; | ||
this.TraceId = traceId; | ||
this.Name = name; | ||
this.Kind = kind; | ||
this.Tags = tags; | ||
this.Links = links; | ||
} | ||
|
||
/// <summary> | ||
/// Gets the parent activity context. | ||
/// </summary> | ||
public ActivityContext ParentContext { get; } | ||
|
||
/// <summary> | ||
/// Gets the trace ID of parent activity or a new generated one for root span/activity. | ||
/// </summary> | ||
public ActivityTraceId TraceId { get; } | ||
|
||
/// <summary> | ||
/// Gets the name to be given to the span/activity. | ||
/// </summary> | ||
public string Name { get; } | ||
|
||
/// <summary> | ||
/// Gets the kind of span/activity to be created. | ||
/// </summary> | ||
public ActivityKind Kind { get; } | ||
|
||
/// <summary> | ||
/// Gets the tags to be associated to the span/activity to be created. | ||
/// </summary> | ||
public IEnumerable<KeyValuePair<string, string>> Tags { get; } | ||
|
||
/// <summary> | ||
/// Gets the links to be added to the activity to be created. | ||
/// </summary> | ||
public IEnumerable<ActivityLink> Links { get; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,14 +84,8 @@ public static IDisposable EnableOpenTelemetry(Action<OpenTelemetryBuilder> confi | |
// This prevents Activity from being created at all. | ||
GetRequestedDataUsingContext = (ref ActivityCreationOptions<ActivityContext> options) => | ||
{ | ||
var shouldSample = sampler.ShouldSample( | ||
options.Parent, | ||
options.Parent.TraceId, | ||
spanId: default, // Passing default SpanId here. The actual SpanId is not known before actual Activity creation | ||
options.Name, | ||
options.Kind, | ||
options.Tags, | ||
options.Links); | ||
BuildSamplingParameters(options, out var samplingParameters); | ||
var shouldSample = sampler.ShouldSample(samplingParameters); | ||
if (shouldSample.IsSampled) | ||
{ | ||
return ActivityDataRequest.AllDataAndRecorded; | ||
|
@@ -109,5 +103,35 @@ public static IDisposable EnableOpenTelemetry(Action<OpenTelemetryBuilder> confi | |
|
||
return listener; | ||
} | ||
|
||
internal static void BuildSamplingParameters( | ||
in ActivityCreationOptions<ActivityContext> options, out ActivitySamplingParameters samplingParameters) | ||
{ | ||
ActivityContext parentContext = options.Parent; | ||
if (parentContext == default) | ||
{ | ||
// Check if there is already a parent for the current activity. | ||
var parentActivity = Activity.Current; | ||
if (parentActivity != null) | ||
{ | ||
parentContext = parentActivity.Context; | ||
} | ||
} | ||
Comment on lines
+111
to
+119
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PR dotnet/runtime#37185 removes the need for this part of the code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if you'll merge this OT PR as is till getting the .NET change. but if you do, please be aware that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the heads up @tarekgh - I think that for now it is better to go as it is so we can keep experimenting with the |
||
|
||
// This is not going to be the final traceId of the Activity (if one is created), however, it is | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if this is correct (except when starting a brand new activity without parent) If there was no parent, then the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The unit test you have validates this. Can we state that TraceId is different only for the Root one scenario, and for everything else, the traceid is the correct one of the activity to be created. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed: the behavior here is a bit tricky. For the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, we should be able to pass the already generated @tarekgh I remember that you had already stated this issue in other comment threads. Seeing it concretely here makes me wonder if we could force the Activity to be created to use the same one passed to the sampler. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I am not clear about the ask here. In the current APIs, you can create the parent context and pass it to ActivitySource.StartActivity, and we'll honor this context and the new Activity will be created with the trace Id passed in this parent context. I am seeing you have full control over how you want the new Activity will be created with which trace id. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cijothomas @tarekgh Putting this together with https://github.com/open-telemetry/opentelemetry-dotnet/pull/702/files#r431881590 I think that we should consider if all work that is done here ( The main downside that I see is that since the current There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually there is a bug with the code as it is: if the real "logical root" is not sampled it is possible that one of its "logical children" becomes sampled and it will show up as an incorrect "root". The proposal above should fix that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/dotnet/runtime/pull/37185/files#diff-ccf594406736f7234e01bead593d4c8aR140 - @pjanotti Please check this PR. It should address our concern? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Partially: it solves the handling of the parent in the case that the root span is sampled (see https://github.com/open-telemetry/opentelemetry-dotnet/pull/702/files#r432778065). However, it doesn't solve the case of not sampling the root/parent since In order to make each step clear and easy to review I suggest the PR on dotnet/runtime (dotnet/runtime#37185) to be merged as it is since it fixes a specific issue on its own and we address the other part separately. /cc @cijothomas @tarekgh |
||
// needed in order for the sampling to work. This differs from other OTel SDKs in which it is | ||
// the Sampler always receives the actual traceId of a root span/activity. | ||
ActivityTraceId traceId = parentContext.TraceId != default | ||
? parentContext.TraceId | ||
: ActivityTraceId.CreateRandom(); | ||
pjanotti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
samplingParameters = new ActivitySamplingParameters( | ||
parentContext, | ||
traceId, | ||
options.Name, | ||
options.Kind, | ||
options.Tags, | ||
options.Links); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
// <copyright file="ProbabilityActivitySampler.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; | ||
using System.Diagnostics; | ||
using System.Globalization; | ||
|
||
namespace OpenTelemetry.Trace.Samplers | ||
{ | ||
/// <summary> | ||
/// Sampler implementation which will take a sample if parent Activity or any linked Activity is sampled. | ||
/// Otherwise, samples traces according to the specified probability. | ||
/// </summary> | ||
public sealed class ProbabilityActivitySampler : ActivitySampler | ||
{ | ||
private readonly long idUpperBound; | ||
private readonly double probability; | ||
|
||
/// <summary> | ||
/// Initializes a new instance of the <see cref="ProbabilityActivitySampler"/> class. | ||
/// </summary> | ||
/// <param name="probability">The desired probability of sampling. This must be between 0.0 and 1.0. | ||
/// Higher the value, higher is the probability of a given Activity to be sampled in. | ||
/// </param> | ||
public ProbabilityActivitySampler(double probability) | ||
{ | ||
if (probability < 0.0 || probability > 1.0) | ||
{ | ||
throw new ArgumentOutOfRangeException(nameof(probability), "Probability must be in range [0.0, 1.0]"); | ||
} | ||
|
||
this.probability = probability; | ||
|
||
// The expected description is like ProbabilityActivitySampler{0.000100} | ||
this.Description = "ProbabilityActivitySampler{" + this.probability.ToString("F6", CultureInfo.InvariantCulture) + "}"; | ||
|
||
// Special case the limits, to avoid any possible issues with lack of precision across | ||
// double/long boundaries. For probability == 0.0, we use Long.MIN_VALUE as this guarantees | ||
// that we will never sample a trace, even in the case where the id == Long.MIN_VALUE, since | ||
// Math.Abs(Long.MIN_VALUE) == Long.MIN_VALUE. | ||
if (this.probability == 0.0) | ||
{ | ||
this.idUpperBound = long.MinValue; | ||
} | ||
else if (this.probability == 1.0) | ||
{ | ||
this.idUpperBound = long.MaxValue; | ||
} | ||
else | ||
{ | ||
this.idUpperBound = (long)(probability * long.MaxValue); | ||
} | ||
} | ||
|
||
/// <inheritdoc /> | ||
public override string Description { get; } | ||
|
||
/// <inheritdoc /> | ||
public override SamplingResult ShouldSample(in ActivitySamplingParameters samplingParameters) | ||
{ | ||
// If the parent is sampled keep the sampling decision. | ||
var parentContext = samplingParameters.ParentContext; | ||
if ((parentContext.TraceFlags & ActivityTraceFlags.Recorded) != 0) | ||
{ | ||
return new SamplingResult(true); | ||
} | ||
|
||
if (samplingParameters.Links != null) | ||
{ | ||
// If any parent link is sampled keep the sampling decision. | ||
foreach (var parentLink in samplingParameters.Links) | ||
{ | ||
if ((parentLink.Context.TraceFlags & ActivityTraceFlags.Recorded) != 0) | ||
{ | ||
return new SamplingResult(true); | ||
} | ||
} | ||
} | ||
|
||
// Always sample if we are within probability range. This is true even for child activities (that | ||
// may have had a different sampling decision made) to allow for different sampling policies, | ||
// and dynamic increases to sampling probabilities for debugging purposes. | ||
// Note use of '<' for comparison. This ensures that we never sample for probability == 0.0, | ||
// while allowing for a (very) small chance of *not* sampling if the id == Long.MAX_VALUE. | ||
// This is considered a reasonable trade-off for the simplicity/performance requirements (this | ||
// code is executed in-line for every Activity creation). | ||
Span<byte> traceIdBytes = stackalloc byte[16]; | ||
samplingParameters.TraceId.CopyTo(traceIdBytes); | ||
return Math.Abs(this.GetLowerLong(traceIdBytes)) < this.idUpperBound ? new SamplingResult(true) : new SamplingResult(false); | ||
} | ||
|
||
private long GetLowerLong(ReadOnlySpan<byte> bytes) | ||
{ | ||
long result = 0; | ||
for (var i = 0; i < 8; i++) | ||
{ | ||
result <<= 8; | ||
#pragma warning disable CS0675 // Bitwise-or operator used on a sign-extended operand | ||
result |= bytes[i] & 0xff; | ||
#pragma warning restore CS0675 // Bitwise-or operator used on a sign-extended operand | ||
} | ||
|
||
return result; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we fit sampling into
GetRequestedDataUsingParentId
(we current do onlyGetRequestedDataUsingContext
), the sampling parameter will not have ActivityContext, but only the parentid.(just posting a note for later revisit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except by the
TraceId
this becomesActivityCreationOptions<T>
depending on how the other issues are handled we may be able to use it directly.