Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 9 additions & 2 deletions src/OpenTelemetry/Trace/Sampler/SamplingResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ namespace OpenTelemetry.Trace;
/// </summary>
public readonly struct SamplingResult : IEquatable<SamplingResult>
{
// Null when no attributes were supplied; avoids a GetEnumerator() call (and enumerator boxing)
// on the hot path inside TracerProviderSdk when the sampler returns no attributes.
private readonly IEnumerable<KeyValuePair<string, object>>? attributesField;

/// <summary>
/// Initializes a new instance of the <see cref="SamplingResult"/> struct.
/// </summary>
Expand Down Expand Up @@ -61,7 +65,7 @@ public SamplingResult(SamplingDecision decision, IEnumerable<KeyValuePair<string
// Note: Decision object takes ownership of the collection.
// Current implementation has no means to ensure the collection will not be modified by the caller.
// If this behavior will be abused we must switch to cloning of the collection.
this.Attributes = attributes ?? [];
this.attributesField = attributes;
Comment thread
martincostello marked this conversation as resolved.

this.TraceStateString = traceStateString;
}
Expand All @@ -74,13 +78,16 @@ public SamplingResult(SamplingDecision decision, IEnumerable<KeyValuePair<string
/// <summary>
/// Gets a map of attributes associated with the sampling decision.
/// </summary>
public IEnumerable<KeyValuePair<string, object>> Attributes { get; }
public IEnumerable<KeyValuePair<string, object>> Attributes => this.attributesField ?? [];

/// <summary>
/// Gets the tracestate.
/// </summary>
public string? TraceStateString { get; }

// Internal accessor used by TracerProviderSdk to skip iteration entirely when null.
internal IEnumerable<KeyValuePair<string, object>>? AttributesOrNull => this.attributesField;

/// <summary>
/// Compare two <see cref="SamplingResult"/> for equality.
/// </summary>
Expand Down
16 changes: 11 additions & 5 deletions src/OpenTelemetry/Trace/TracerProviderSdk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ internal TracerProviderSdk(

if (this.Sampler is AlwaysOnSampler)
{
activityListener.Sample = (ref options) =>
Comment thread
martincostello marked this conversation as resolved.
activityListener.Sample = static (ref _) =>
!Sdk.SuppressInstrumentation ? ActivitySamplingResult.AllDataAndRecorded : ActivitySamplingResult.None;
this.getRequestedDataAction = this.RunGetRequestedDataAlwaysOnSampler;
}
Expand Down Expand Up @@ -479,9 +479,12 @@ private static ActivitySamplingResult ComputeActivitySamplingResult(

if (activitySamplingResult > ActivitySamplingResult.PropagationData)
{
foreach (var att in samplingResult.Attributes)
if (samplingResult.AttributesOrNull is { } attributes)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❤️ nice elimination of the alloc here!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • would we get same benefit by comparing Attributes to empty array (Array.Empty<KeyValuePair<string, object>>()) )first before attempting to iterate? That'd be simpler from code complexity, even though it may not be future proof..

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure - but the compiler can use special private types for collection expressions so "not null" is probably best than assuming it's the exact object returned by Array.Empty<T>().

{
options.SamplingTags.Add(att.Key, att.Value);
foreach (var att in attributes)
{
options.SamplingTags.Add(att.Key, att.Value);
}
}
}

Expand Down Expand Up @@ -575,9 +578,12 @@ private void RunGetRequestedDataOtherSampler(Activity activity)

if (samplingResult.Decision != SamplingDecision.Drop)
{
foreach (var att in samplingResult.Attributes)
if (samplingResult.AttributesOrNull is { } attributes)
{
activity.SetTag(att.Key, att.Value);
foreach (var att in attributes)
{
activity.SetTag(att.Key, att.Value);
}
}
}

Expand Down
134 changes: 134 additions & 0 deletions test/Benchmarks/Trace/SamplingResultBenchmarks.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
// Copyright The OpenTelemetry Authors
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: could we put in the same SamplerBenchmarks?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry, I don't quite follow this comment.

Do you mean to just move the new benchmarks into SamplerBenchmarks?

// SPDX-License-Identifier: Apache-2.0

using System.Diagnostics;
using BenchmarkDotNet.Attributes;
using OpenTelemetry;
using OpenTelemetry.Trace;

namespace Benchmarks.Trace;

#pragma warning disable CA1001 // Types that own disposable fields should be disposable - handled by GlobalCleanup
[MemoryDiagnoser]
public class SamplingResultBenchmarks
#pragma warning restore CA1001 // Types that own disposable fields should be disposable - handled by GlobalCleanup
{
private static readonly KeyValuePair<string, object>[] SamplingAttributes =
[
new("sampling.priority", 1),
new("sampling.rule", "always"),
];

private ActivitySource? sourceNoAttributes;
private ActivitySource? sourceWithAttributeArray;
private ActivitySource? sourceWithAttributeList;
private ActivitySource? sourceDrop;
private ActivitySource? sourceParentBased;

private ActivityContext sampledRemoteParent;

private TracerProvider? providerNoAttributes;
private TracerProvider? providerWithAttributeArray;
private TracerProvider? providerWithAttributeList;
private TracerProvider? providerDrop;
private TracerProvider? providerParentBased;

[GlobalSetup]
public void Setup()
{
this.sourceNoAttributes = new ActivitySource("SamplingResult.NoAttributes");
this.sourceWithAttributeArray = new ActivitySource("SamplingResult.WithAttributeArray");
this.sourceWithAttributeList = new ActivitySource("SamplingResult.WithAttributeList");
this.sourceDrop = new ActivitySource("SamplingResult.Drop");
this.sourceParentBased = new ActivitySource("SamplingResult.ParentBased");

this.sampledRemoteParent = new ActivityContext(
ActivityTraceId.CreateRandom(),
ActivitySpanId.CreateRandom(),
ActivityTraceFlags.Recorded,
traceState: null,
isRemote: true);

// Sampler returns RecordAndSample with no attributes - the common case.
this.providerNoAttributes = Sdk.CreateTracerProviderBuilder()
.AddSource(this.sourceNoAttributes.Name)
.SetSampler(new DelegateSampler(_ => new SamplingResult(SamplingDecision.RecordAndSample)))
.Build();

// Sampler returns attributes as a T[] - exercises the array fast-path.
this.providerWithAttributeArray = Sdk.CreateTracerProviderBuilder()
.AddSource(this.sourceWithAttributeArray.Name)
.SetSampler(new DelegateSampler(_ => new SamplingResult(SamplingDecision.RecordAndSample, SamplingAttributes)))
.Build();

// Sampler returns attributes as a List<T> - exercises the IEnumerable fallback path.
this.providerWithAttributeList = Sdk.CreateTracerProviderBuilder()
.AddSource(this.sourceWithAttributeList.Name)
.SetSampler(new DelegateSampler(_ => new SamplingResult(SamplingDecision.RecordAndSample, [.. SamplingAttributes])))
.Build();

// Sampler drops the span - attribute loop is never entered.
this.providerDrop = Sdk.CreateTracerProviderBuilder()
.AddSource(this.sourceDrop.Name)
.SetSampler(new DelegateSampler(_ => new SamplingResult(SamplingDecision.Drop)))
.Build();

// ParentBasedSampler with AlwaysOnSampler root - realistic production default.
this.providerParentBased = Sdk.CreateTracerProviderBuilder()
.AddSource(this.sourceParentBased.Name)
.SetSampler(new ParentBasedSampler(new AlwaysOnSampler()))
.Build();
}

[GlobalCleanup]
public void Cleanup()
{
this.sourceNoAttributes?.Dispose();
this.sourceWithAttributeArray?.Dispose();
this.sourceWithAttributeList?.Dispose();
this.sourceDrop?.Dispose();
this.sourceParentBased?.Dispose();

this.providerNoAttributes?.Dispose();
this.providerWithAttributeArray?.Dispose();
this.providerWithAttributeList?.Dispose();
this.providerDrop?.Dispose();
this.providerParentBased?.Dispose();
}

[Benchmark(Baseline = true)]
public void NoAttributes()
{
using var activity = this.sourceNoAttributes!.StartActivity("Benchmark", ActivityKind.Server, this.sampledRemoteParent);
}

[Benchmark]
public void WithAttributeArray()
{
using var activity = this.sourceWithAttributeArray!.StartActivity("Benchmark", ActivityKind.Server, this.sampledRemoteParent);
}

[Benchmark]
public void WithAttributeList()
{
using var activity = this.sourceWithAttributeList!.StartActivity("Benchmark", ActivityKind.Server, this.sampledRemoteParent);
}

[Benchmark]
public void Drop()
{
using var activity = this.sourceDrop!.StartActivity("Benchmark", ActivityKind.Server, this.sampledRemoteParent);
}

[Benchmark]
public void ParentBasedSampled()
{
using var activity = this.sourceParentBased!.StartActivity("Benchmark", ActivityKind.Server, this.sampledRemoteParent);
}

private sealed class DelegateSampler(Func<SamplingParameters, SamplingResult> sample) : Sampler
{
public override SamplingResult ShouldSample(in SamplingParameters samplingParameters)
=> sample(samplingParameters);
}
}
Loading