From b54cc80b67396c1e56f851db913b6c988e068b8a Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Mon, 29 Aug 2022 19:28:18 -0400 Subject: [PATCH] Add support for samplers to modify Tracestate (#3610) --- .vscode/settings.json | 2 + .../.publicApi/net462/PublicAPI.Unshipped.txt | 3 + .../.publicApi/net6.0/PublicAPI.Unshipped.txt | 3 + .../netstandard2.0/PublicAPI.Unshipped.txt | 3 + .../netstandard2.1/PublicAPI.Unshipped.txt | 3 + src/OpenTelemetry/CHANGELOG.md | 3 + src/OpenTelemetry/Trace/SamplingResult.cs | 46 ++++- src/OpenTelemetry/Trace/TracerProviderSdk.cs | 32 ++- .../OpenTelemetry.Tests/Trace/SamplersTest.cs | 182 ++++++++++++++++++ 9 files changed, 269 insertions(+), 8 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index d50e0ea856c..4868d951f64 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -8,6 +8,7 @@ "contoso", "cref", "grafana", + "inheritdoc", "janotti", "kanzhelev", "liudmila", @@ -36,6 +37,7 @@ "struct", "tbody", "thead", + "Tracestate", "tracestate", "triager", "umesan", diff --git a/src/OpenTelemetry/.publicApi/net462/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net462/PublicAPI.Unshipped.txt index 029e596e0cd..e1ad275ea8d 100644 --- a/src/OpenTelemetry/.publicApi/net462/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/net462/PublicAPI.Unshipped.txt @@ -19,6 +19,9 @@ OpenTelemetry.Logs.OpenTelemetryLoggerOptionsExtensions OpenTelemetry.Logs.OpenTelemetryLoggerProvider.AddProcessor(OpenTelemetry.BaseProcessor! processor) -> OpenTelemetry.Logs.OpenTelemetryLoggerProvider! OpenTelemetry.Logs.OpenTelemetryLoggerProvider.ForceFlush(int timeoutMilliseconds = -1) -> bool OpenTelemetry.Logs.OpenTelemetryLoggerProvider.OpenTelemetryLoggerProvider() -> void +~OpenTelemetry.Trace.SamplingResult.TraceStateString.get -> string +~OpenTelemetry.Trace.SamplingResult.SamplingResult(OpenTelemetry.Trace.SamplingDecision decision, string traceStateString) -> void +~OpenTelemetry.Trace.SamplingResult.SamplingResult(OpenTelemetry.Trace.SamplingDecision decision, System.Collections.Generic.IEnumerable> attributes, string traceStateString) -> void *REMOVED*static Microsoft.Extensions.Logging.OpenTelemetryLoggingExtensions.AddOpenTelemetry(this Microsoft.Extensions.Logging.ILoggingBuilder! builder, System.Action? configure = null) -> Microsoft.Extensions.Logging.ILoggingBuilder! OpenTelemetry.Trace.ExportActivityProcessorOptions OpenTelemetry.Trace.ExportActivityProcessorOptions.BatchExportProcessorOptions.get -> OpenTelemetry.Trace.BatchExportActivityProcessorOptions! diff --git a/src/OpenTelemetry/.publicApi/net6.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net6.0/PublicAPI.Unshipped.txt index 029e596e0cd..e1ad275ea8d 100644 --- a/src/OpenTelemetry/.publicApi/net6.0/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/net6.0/PublicAPI.Unshipped.txt @@ -19,6 +19,9 @@ OpenTelemetry.Logs.OpenTelemetryLoggerOptionsExtensions OpenTelemetry.Logs.OpenTelemetryLoggerProvider.AddProcessor(OpenTelemetry.BaseProcessor! processor) -> OpenTelemetry.Logs.OpenTelemetryLoggerProvider! OpenTelemetry.Logs.OpenTelemetryLoggerProvider.ForceFlush(int timeoutMilliseconds = -1) -> bool OpenTelemetry.Logs.OpenTelemetryLoggerProvider.OpenTelemetryLoggerProvider() -> void +~OpenTelemetry.Trace.SamplingResult.TraceStateString.get -> string +~OpenTelemetry.Trace.SamplingResult.SamplingResult(OpenTelemetry.Trace.SamplingDecision decision, string traceStateString) -> void +~OpenTelemetry.Trace.SamplingResult.SamplingResult(OpenTelemetry.Trace.SamplingDecision decision, System.Collections.Generic.IEnumerable> attributes, string traceStateString) -> void *REMOVED*static Microsoft.Extensions.Logging.OpenTelemetryLoggingExtensions.AddOpenTelemetry(this Microsoft.Extensions.Logging.ILoggingBuilder! builder, System.Action? configure = null) -> Microsoft.Extensions.Logging.ILoggingBuilder! OpenTelemetry.Trace.ExportActivityProcessorOptions OpenTelemetry.Trace.ExportActivityProcessorOptions.BatchExportProcessorOptions.get -> OpenTelemetry.Trace.BatchExportActivityProcessorOptions! diff --git a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt index 440dabec6ea..b96dd6dd3ee 100644 --- a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt @@ -19,6 +19,9 @@ OpenTelemetry.Logs.OpenTelemetryLoggerProvider.AddProcessor(OpenTelemetry.BasePr OpenTelemetry.Logs.OpenTelemetryLoggerProvider.ForceFlush(int timeoutMilliseconds = -1) -> bool OpenTelemetry.Logs.OpenTelemetryLoggerProvider.OpenTelemetryLoggerProvider() -> void OpenTelemetry.Logs.OpenTelemetryLoggerOptions.ConfigureResource(System.Action! configure) -> OpenTelemetry.Logs.OpenTelemetryLoggerOptions! +~OpenTelemetry.Trace.SamplingResult.TraceStateString.get -> string +~OpenTelemetry.Trace.SamplingResult.SamplingResult(OpenTelemetry.Trace.SamplingDecision decision, string traceStateString) -> void +~OpenTelemetry.Trace.SamplingResult.SamplingResult(OpenTelemetry.Trace.SamplingDecision decision, System.Collections.Generic.IEnumerable> attributes, string traceStateString) -> void *REMOVED*static Microsoft.Extensions.Logging.OpenTelemetryLoggingExtensions.AddOpenTelemetry(this Microsoft.Extensions.Logging.ILoggingBuilder! builder, System.Action? configure = null) -> Microsoft.Extensions.Logging.ILoggingBuilder! OpenTelemetry.Trace.ExportActivityProcessorOptions OpenTelemetry.Trace.ExportActivityProcessorOptions.BatchExportProcessorOptions.get -> OpenTelemetry.Trace.BatchExportActivityProcessorOptions! diff --git a/src/OpenTelemetry/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt index 029e596e0cd..e1ad275ea8d 100644 --- a/src/OpenTelemetry/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt @@ -19,6 +19,9 @@ OpenTelemetry.Logs.OpenTelemetryLoggerOptionsExtensions OpenTelemetry.Logs.OpenTelemetryLoggerProvider.AddProcessor(OpenTelemetry.BaseProcessor! processor) -> OpenTelemetry.Logs.OpenTelemetryLoggerProvider! OpenTelemetry.Logs.OpenTelemetryLoggerProvider.ForceFlush(int timeoutMilliseconds = -1) -> bool OpenTelemetry.Logs.OpenTelemetryLoggerProvider.OpenTelemetryLoggerProvider() -> void +~OpenTelemetry.Trace.SamplingResult.TraceStateString.get -> string +~OpenTelemetry.Trace.SamplingResult.SamplingResult(OpenTelemetry.Trace.SamplingDecision decision, string traceStateString) -> void +~OpenTelemetry.Trace.SamplingResult.SamplingResult(OpenTelemetry.Trace.SamplingDecision decision, System.Collections.Generic.IEnumerable> attributes, string traceStateString) -> void *REMOVED*static Microsoft.Extensions.Logging.OpenTelemetryLoggingExtensions.AddOpenTelemetry(this Microsoft.Extensions.Logging.ILoggingBuilder! builder, System.Action? configure = null) -> Microsoft.Extensions.Logging.ILoggingBuilder! OpenTelemetry.Trace.ExportActivityProcessorOptions OpenTelemetry.Trace.ExportActivityProcessorOptions.BatchExportProcessorOptions.get -> OpenTelemetry.Trace.BatchExportActivityProcessorOptions! diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index b0657593758..bdce8710d3f 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +* Allows samplers the ability to modify tracestate if desired. + ([#3610](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3610)) + ## 1.4.0-alpha.2 Released 2022-Aug-18 diff --git a/src/OpenTelemetry/Trace/SamplingResult.cs b/src/OpenTelemetry/Trace/SamplingResult.cs index 7646ac731b0..ccdaa5c9d94 100644 --- a/src/OpenTelemetry/Trace/SamplingResult.cs +++ b/src/OpenTelemetry/Trace/SamplingResult.cs @@ -20,7 +20,7 @@ namespace OpenTelemetry.Trace { /// - /// Sampling decision. + /// Sampling result. /// public readonly struct SamplingResult : System.IEquatable { @@ -32,6 +32,7 @@ public SamplingResult(SamplingDecision decision) { this.Decision = decision; this.Attributes = Enumerable.Empty>(); + this.TraceStateString = null; } /// @@ -42,6 +43,7 @@ public SamplingResult(bool isSampled) { this.Decision = isSampled ? SamplingDecision.RecordAndSample : SamplingDecision.Drop; this.Attributes = Enumerable.Empty>(); + this.TraceStateString = null; } /// @@ -58,6 +60,38 @@ public SamplingResult(SamplingDecision decision, IEnumerable + /// Initializes a new instance of the struct. + /// + /// indicates whether an activity object is recorded and sampled. + /// traceStateString associated with the created Activity. + public SamplingResult(SamplingDecision decision, string traceStateString) + { + this.Decision = decision; + this.Attributes = Enumerable.Empty>(); + this.TraceStateString = traceStateString; + } + + /// + /// Initializes a new instance of the struct. + /// + /// indicates whether an activity object is recorded and sampled. + /// attributes associated with the sampling decision. Attributes list passed to + /// this method must be immutable. Mutations of the collection and/or attribute values may lead to unexpected behavior. + /// traceStateString associated with the created Activity. + public SamplingResult(SamplingDecision decision, IEnumerable> attributes, string traceStateString) + { + this.Decision = decision; + + // 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.TraceStateString = traceStateString; } /// @@ -70,6 +104,11 @@ public SamplingResult(SamplingDecision decision, IEnumerable public IEnumerable> Attributes { get; } + /// + /// Gets the tracestate. + /// + public string TraceStateString { get; } + /// /// Compare two for equality. /// @@ -93,7 +132,7 @@ public override bool Equals(object obj) } var that = (SamplingResult)obj; - return this.Decision == that.Decision && this.Attributes == that.Attributes; + return this.Decision == that.Decision && this.Attributes == that.Attributes && this.TraceStateString == that.TraceStateString; } /// @@ -102,13 +141,14 @@ public override int GetHashCode() var result = 1; result = (31 * result) + this.Decision.GetHashCode(); result = (31 * result) + this.Attributes.GetHashCode(); + result = this.TraceStateString == null ? result : (result * 31) + this.TraceStateString.GetHashCode(); return result; } /// public bool Equals(SamplingResult other) { - return this.Decision == other.Decision && this.Attributes.SequenceEqual(other.Attributes); + return this.Decision == other.Decision && this.Attributes.SequenceEqual(other.Attributes) && this.TraceStateString == other.TraceStateString; } } } diff --git a/src/OpenTelemetry/Trace/TracerProviderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderSdk.cs index adf1738b174..13d3d969cb0 100644 --- a/src/OpenTelemetry/Trace/TracerProviderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderSdk.cs @@ -219,7 +219,7 @@ internal TracerProviderSdk( { // This delegate informs ActivitySource about sampling decision when the parent context is an ActivityContext. listener.Sample = (ref ActivityCreationOptions options) => - !Sdk.SuppressInstrumentation ? ComputeActivitySamplingResult(options, this.sampler) : ActivitySamplingResult.None; + !Sdk.SuppressInstrumentation ? ComputeActivitySamplingResult(ref options, this.sampler) : ActivitySamplingResult.None; this.getRequestedDataAction = this.RunGetRequestedDataOtherSampler; } @@ -380,7 +380,7 @@ protected override void Dispose(bool disposing) } private static ActivitySamplingResult ComputeActivitySamplingResult( - in ActivityCreationOptions options, + ref ActivityCreationOptions options, Sampler sampler) { var samplingParameters = new SamplingParameters( @@ -391,9 +391,9 @@ private static ActivitySamplingResult ComputeActivitySamplingResult( options.Tags, options.Links); - var shouldSample = sampler.ShouldSample(samplingParameters); + var samplingResult = sampler.ShouldSample(samplingParameters); - var activitySamplingResult = shouldSample.Decision switch + var activitySamplingResult = samplingResult.Decision switch { SamplingDecision.RecordAndSample => ActivitySamplingResult.AllDataAndRecorded, SamplingDecision.RecordOnly => ActivitySamplingResult.AllData, @@ -402,11 +402,28 @@ private static ActivitySamplingResult ComputeActivitySamplingResult( if (activitySamplingResult != ActivitySamplingResult.PropagationData) { - foreach (var att in shouldSample.Attributes) + foreach (var att in samplingResult.Attributes) { options.SamplingTags.Add(att.Key, att.Value); } + // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#sampler + // Spec requires clearing Tracestate if empty Tracestate is returned. + // Since .NET did not have this capability, it'll break + // existing samplers if we did that. So the following is + // adopted to remain spec-compliant and backward compat. + // The behavior is: + // if sampler returns null, its treated as if it has no intend + // to change Tracestate. Existing SamplingResult ctors will put null as default TraceStateString, + // so all existing samplers will get this behavior. + // if sampler returns non-null, then it'll be used as the + // new value for Tracestate + // A sampler can return string.Empty if it intends to clear the state. + if (samplingResult.TraceStateString != null) + { + options = options with { TraceState = samplingResult.TraceStateString }; + } + return activitySamplingResult; } @@ -493,6 +510,11 @@ private void RunGetRequestedDataOtherSampler(Activity activity) { activity.SetTag(att.Key, att.Value); } + + if (samplingResult.TraceStateString != null) + { + activity.TraceStateString = samplingResult.TraceStateString; + } } } } diff --git a/test/OpenTelemetry.Tests/Trace/SamplersTest.cs b/test/OpenTelemetry.Tests/Trace/SamplersTest.cs index 57946ac82ea..979270ca6fd 100644 --- a/test/OpenTelemetry.Tests/Trace/SamplersTest.cs +++ b/test/OpenTelemetry.Tests/Trace/SamplersTest.cs @@ -15,12 +15,14 @@ // using System.Collections.Generic; using System.Diagnostics; +using OpenTelemetry.Tests; using Xunit; namespace OpenTelemetry.Trace.Tests { public class SamplersTest { + private const string ActivitySourceName = "SamplerTest"; private static readonly ActivityKind ActivityKindServer = ActivityKind.Server; private readonly ActivityTraceId traceId; private readonly ActivitySpanId spanId; @@ -70,5 +72,185 @@ public void AlwaysOffSampler_GetDescription() { Assert.Equal("AlwaysOffSampler", new AlwaysOffSampler().Description); } + + [Theory] + [InlineData(SamplingDecision.Drop)] + [InlineData(SamplingDecision.RecordOnly)] + [InlineData(SamplingDecision.RecordAndSample)] + public void TracerProviderSdkSamplerAttributesAreAppliedToLegacyActivity(SamplingDecision samplingDecision) + { + var testSampler = new TestSampler + { + SamplingAction = (samplingParams) => + { + var attributes = new Dictionary + { + { "tagkeybysampler", "tagvalueaddedbysampler" }, + }; + return new SamplingResult(samplingDecision, attributes); + }, + }; + + var operationNameForLegacyActivity = "TestOperationName"; + using var tracerProvider = Sdk.CreateTracerProviderBuilder() + .SetSampler(testSampler) + .AddLegacySource(operationNameForLegacyActivity) + .Build(); + + Activity activity = new Activity(operationNameForLegacyActivity); + activity.Start(); + Assert.NotNull(activity); + if (samplingDecision != SamplingDecision.Drop) + { + Assert.Contains(new KeyValuePair("tagkeybysampler", "tagvalueaddedbysampler"), activity.TagObjects); + } + + activity.Stop(); + } + + [Theory] + [InlineData(SamplingDecision.Drop)] + [InlineData(SamplingDecision.RecordOnly)] + [InlineData(SamplingDecision.RecordAndSample)] + public void SamplersCanModifyTraceStateOnLegacyActivity(SamplingDecision samplingDecision) + { + var existingTraceState = "a=1,b=2"; + var newTraceState = "a=1,b=2,c=3,d=4"; + var testSampler = new TestSampler + { + SamplingAction = (samplingParams) => + { + Assert.Equal(existingTraceState, samplingParams.ParentContext.TraceState); + return new SamplingResult(samplingDecision, newTraceState); + }, + }; + + var operationNameForLegacyActivity = "TestOperationName"; + using var tracerProvider = Sdk.CreateTracerProviderBuilder() + .SetSampler(testSampler) + .AddLegacySource(operationNameForLegacyActivity) + .Build(); + + Activity parentActivity = new Activity("Foo"); + parentActivity.TraceStateString = existingTraceState; + parentActivity.Start(); + + Activity activity = new Activity(operationNameForLegacyActivity); + activity.Start(); + Assert.NotNull(activity); + if (samplingDecision != SamplingDecision.Drop) + { + Assert.Equal(newTraceState, activity.TraceStateString); + } + + activity.Stop(); + parentActivity.Stop(); + } + + [Theory] + [InlineData(SamplingDecision.Drop)] + [InlineData(SamplingDecision.RecordOnly)] + [InlineData(SamplingDecision.RecordAndSample)] + public void SamplersDoesNotImpactTraceStateWhenUsingNullLegacyActivity(SamplingDecision samplingDecision) + { + var existingTraceState = "a=1,b=2"; + var testSampler = new TestSampler + { + SamplingAction = (samplingParams) => + { + Assert.Equal(existingTraceState, samplingParams.ParentContext.TraceState); + return new SamplingResult(samplingDecision); + }, + }; + + var operationNameForLegacyActivity = "TestOperationName"; + using var tracerProvider = Sdk.CreateTracerProviderBuilder() + .SetSampler(testSampler) + .AddLegacySource(operationNameForLegacyActivity) + .Build(); + + Activity parentActivity = new Activity("Foo"); + parentActivity.TraceStateString = existingTraceState; + parentActivity.Start(); + + Activity activity = new Activity(operationNameForLegacyActivity); + activity.Start(); + Assert.NotNull(activity); + if (samplingDecision != SamplingDecision.Drop) + { + Assert.Equal(existingTraceState, activity.TraceStateString); + } + + activity.Stop(); + parentActivity.Stop(); + } + + [Theory] + [InlineData(SamplingDecision.Drop)] + [InlineData(SamplingDecision.RecordOnly)] + [InlineData(SamplingDecision.RecordAndSample)] + public void SamplersCanModifyTraceState(SamplingDecision sampling) + { + var parentTraceState = "a=1,b=2"; + var newTraceState = "a=1,b=2,c=3,d=4"; + var testSampler = new TestSampler + { + SamplingAction = (samplingParams) => + { + Assert.Equal(parentTraceState, samplingParams.ParentContext.TraceState); + return new SamplingResult(sampling, newTraceState); + }, + }; + + using var activitySource = new ActivitySource(ActivitySourceName); + using var tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddSource(ActivitySourceName) + .SetSampler(testSampler) + .Build(); + + var parentContext = new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.Recorded, parentTraceState, true); + + using var activity = activitySource.StartActivity("root", ActivityKind.Server, parentContext); + if (sampling != SamplingDecision.Drop) + { + Assert.Equal(newTraceState, activity.TraceStateString); + } + } + + [Theory] + [InlineData(SamplingDecision.Drop)] + [InlineData(SamplingDecision.RecordOnly)] + [InlineData(SamplingDecision.RecordAndSample)] + public void SamplersDoesNotImpactTraceStateWhenUsingNull(SamplingDecision sampling) + { + var parentTraceState = "a=1,b=2"; + var testSampler = new TestSampler + { + SamplingAction = (samplingParams) => + { + Assert.Equal(parentTraceState, samplingParams.ParentContext.TraceState); + + // Not explicitly setting tracestate, leaving it null. + // backward compat test that existing + // samplers will not inadvertently + // reset Tracestate + return new SamplingResult(sampling); + }, + }; + + using var activitySource = new ActivitySource(ActivitySourceName); + using var tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddSource(ActivitySourceName) + .SetSampler(testSampler) + .Build(); + + var parentContext = new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.Recorded, parentTraceState, true); + + using var activity = activitySource.StartActivity("root", ActivityKind.Server, parentContext); + if (sampling != SamplingDecision.Drop) + { + Assert.Equal(parentTraceState, activity.TraceStateString); + } + } } }