From f39501aa77f72576d9efe7220ab37e5780dc86fd Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 10 Jan 2025 11:42:15 -0800 Subject: [PATCH 1/3] Fixed a bug causing TraceState set from sampler being dropped for propagation-only spans. --- src/OpenTelemetry/Trace/TracerProviderSdk.cs | 35 ++-- .../OpenTelemetry.Tests/Trace/SamplersTest.cs | 172 +++++++++++------- 2 files changed, 123 insertions(+), 84 deletions(-) diff --git a/src/OpenTelemetry/Trace/TracerProviderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderSdk.cs index 8fd333b1958..fb5b24bfe7c 100644 --- a/src/OpenTelemetry/Trace/TracerProviderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderSdk.cs @@ -237,7 +237,7 @@ internal TracerProviderSdk( else if (this.sampler is AlwaysOffSampler) { activityListener.Sample = (ref ActivityCreationOptions options) => - !Sdk.SuppressInstrumentation ? PropagateOrIgnoreData(options.Parent) : ActivitySamplingResult.None; + !Sdk.SuppressInstrumentation ? PropagateOrIgnoreData(ref options) : ActivitySamplingResult.None; this.getRequestedDataAction = this.RunGetRequestedDataAlwaysOffSampler; } else @@ -493,47 +493,46 @@ private static ActivitySamplingResult ComputeActivitySamplingResult( { SamplingDecision.RecordAndSample => ActivitySamplingResult.AllDataAndRecorded, SamplingDecision.RecordOnly => ActivitySamplingResult.AllData, - _ => ActivitySamplingResult.PropagationData, + _ => PropagateOrIgnoreData(ref options), }; - if (activitySamplingResult != ActivitySamplingResult.PropagationData) + if (activitySamplingResult > ActivitySamplingResult.PropagationData) { foreach (var att in samplingResult.Attributes) { options.SamplingTags.Add(att.Key, att.Value); } + } + if (activitySamplingResult != ActivitySamplingResult.None + && samplingResult.TraceStateString != null) + { // 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 + // if sampler returns null, its treated as if it has not intended // 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; + options = options with { TraceState = samplingResult.TraceStateString }; } - return PropagateOrIgnoreData(options.Parent); + return activitySamplingResult; } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static ActivitySamplingResult PropagateOrIgnoreData(in ActivityContext parentContext) + private static ActivitySamplingResult PropagateOrIgnoreData(ref ActivityCreationOptions options) { - var isRootSpan = parentContext.TraceId == default; + var isRootSpan = options.Parent.TraceId == default; // If it is the root span or the parent is remote select PropagationData so the trace ID is preserved // even if no activity of the trace is recorded (sampled per OpenTelemetry parlance). - return (isRootSpan || parentContext.IsRemote) + return (isRootSpan || options.Parent.IsRemote) ? ActivitySamplingResult.PropagationData : ActivitySamplingResult.None; } @@ -606,11 +605,11 @@ private void RunGetRequestedDataOtherSampler(Activity activity) { activity.SetTag(att.Key, att.Value); } + } - if (samplingResult.TraceStateString != null) - { - activity.TraceStateString = samplingResult.TraceStateString; - } + 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 2c4b85fe523..af1a175a4f2 100644 --- a/test/OpenTelemetry.Tests/Trace/SamplersTest.cs +++ b/test/OpenTelemetry.Tests/Trace/SamplersTest.cs @@ -100,37 +100,7 @@ public void TracerProviderSdkSamplerAttributesAreAppliedToLegacyActivity(Samplin [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 = Utils.GetCurrentMethodName(); - using var tracerProvider = Sdk.CreateTracerProviderBuilder() - .SetSampler(testSampler) - .AddLegacySource(operationNameForLegacyActivity) - .Build(); - - using var parentActivity = new Activity("Foo"); - parentActivity.TraceStateString = existingTraceState; - parentActivity.Start(); - - using var activity = new Activity(operationNameForLegacyActivity); - activity.Start(); - Assert.NotNull(activity); - if (samplingDecision != SamplingDecision.Drop) - { - Assert.Equal(newTraceState, activity.TraceStateString); - } - - activity.Stop(); - parentActivity.Stop(); + RunLegacyActivitySamplerTest(samplingDecision, samplerTraceState: "a=1,b=2,c=3,d=4"); } [Theory] @@ -139,36 +109,7 @@ public void SamplersCanModifyTraceStateOnLegacyActivity(SamplingDecision samplin [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 = Utils.GetCurrentMethodName(); - using var tracerProvider = Sdk.CreateTracerProviderBuilder() - .SetSampler(testSampler) - .AddLegacySource(operationNameForLegacyActivity) - .Build(); - - using var parentActivity = new Activity("Foo"); - parentActivity.TraceStateString = existingTraceState; - parentActivity.Start(); - - using var activity = new Activity(operationNameForLegacyActivity); - activity.Start(); - Assert.NotNull(activity); - if (samplingDecision != SamplingDecision.Drop) - { - Assert.Equal(existingTraceState, activity.TraceStateString); - } - - activity.Stop(); - parentActivity.Stop(); + RunLegacyActivitySamplerTest(samplingDecision, samplerTraceState: null); } [Theory] @@ -183,7 +124,15 @@ public void SamplersCanModifyTraceState(SamplingDecision sampling) { SamplingAction = (samplingParams) => { - Assert.Equal(parentTraceState, samplingParams.ParentContext.TraceState); + if (samplingParams.Name == "root") + { + Assert.Equal(parentTraceState, samplingParams.ParentContext.TraceState); + } + else + { + Assert.Equal(newTraceState, samplingParams.ParentContext.TraceState); + } + return new SamplingResult(sampling, newTraceState); }, }; @@ -195,13 +144,50 @@ public void SamplersCanModifyTraceState(SamplingDecision sampling) .SetSampler(testSampler) .Build(); - var parentContext = new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.Recorded, parentTraceState, true); + // Note: Remote parent is set as recorded + var parentContext = new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.Recorded, parentTraceState, isRemote: true); + + using var root = activitySource.StartActivity("root", ActivityKind.Server, parentContext); + + // Note: We always create a root even for Drop. When dropping the + // created root is for propagation only + Assert.NotNull(root); + Assert.Equal(newTraceState, root.TraceStateString); + + if (sampling == SamplingDecision.RecordAndSample) + { + Assert.True(root.Recorded); + Assert.True(root.IsAllDataRequested); + } + else if (sampling == SamplingDecision.RecordOnly) + { + // Note: Seems to be a bug in DiagnosticSource. Root in this case + // inherits context from the remote parent and Recorded doesn't get + // cleared + // Assert.False(root.Recorded); + + Assert.True(root.IsAllDataRequested); + } + else + { + // Note: Seems to be a bug in DiagnosticSource. Root in this case + // inherits context from the remote parent and Recorded doesn't get + // cleared + // Assert.False(root.Recorded); + + Assert.False(root.IsAllDataRequested); + } + + using var child = activitySource.StartActivity("child", ActivityKind.Server); - using var activity = activitySource.StartActivity("root", ActivityKind.Server, parentContext); if (sampling != SamplingDecision.Drop) { - Assert.NotNull(activity); - Assert.Equal(newTraceState, activity.TraceStateString); + Assert.NotNull(child); + Assert.Equal(newTraceState, child.TraceStateString); + } + else + { + Assert.Null(child); } } @@ -260,6 +246,60 @@ public void SamplerExceptionBubblesUpTest() Assert.Throws(() => activitySource.StartActivity("ThrowingSampler")); } + private static void RunLegacyActivitySamplerTest(SamplingDecision samplingDecision, string? samplerTraceState) + { + var existingTraceState = "a=1,b=2"; + + var operationNameForLegacyActivity = Utils.GetCurrentMethodName(); + + var testSampler = new TestSampler + { + SamplingAction = (samplingParams) => + { + Assert.Equal(samplingParams.Name, operationNameForLegacyActivity); + Assert.Equal(existingTraceState, samplingParams.ParentContext.TraceState); + return new SamplingResult(samplingDecision, samplerTraceState); + }, + }; + + using var tracerProvider = Sdk.CreateTracerProviderBuilder() + .SetSampler(testSampler) + .AddLegacySource(operationNameForLegacyActivity) + .Build(); + + using var parentActivity = new Activity("Foo"); + parentActivity.TraceStateString = existingTraceState; + parentActivity.Start(); + + using var childActivity = new Activity(operationNameForLegacyActivity); + childActivity.Start(); + + if (samplerTraceState != null) + { + Assert.Equal(samplerTraceState, childActivity.TraceStateString); + } + else + { + Assert.Equal(existingTraceState, childActivity.TraceStateString); + } + + if (samplingDecision == SamplingDecision.RecordAndSample) + { + Assert.True(childActivity.Recorded); + Assert.True(childActivity.IsAllDataRequested); + } + else if (samplingDecision == SamplingDecision.RecordOnly) + { + Assert.False(childActivity.Recorded); + Assert.True(childActivity.IsAllDataRequested); + } + else + { + Assert.False(childActivity.Recorded); + Assert.False(childActivity.IsAllDataRequested); + } + } + private sealed class ThrowingSampler : Sampler { public override SamplingResult ShouldSample(in SamplingParameters samplingParameters) From 610fc90531c33e4497c884eb968a840f223e2833 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 10 Jan 2025 12:04:20 -0800 Subject: [PATCH 2/3] CHANGELOG patch. --- src/OpenTelemetry/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 0c6cfef5527..6f2dae58c9e 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -12,6 +12,10 @@ Notes](../../RELEASENOTES.md). now lead to unique metrics. ([#5982](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5982)) +* Fixed a bug in tracing where `TraceState` set by a custom `Sampler` is not + applied when creating propagation-only spans. + ([#6058](https://github.com/open-telemetry/opentelemetry-dotnet/pull/6058)) + ## 1.11.0-rc.1 Released 2024-Dec-11 From 804e3c2025cbc7de8a5deceb4ab24b9fcdb094ac Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 14 Jan 2025 14:05:19 -0800 Subject: [PATCH 3/3] Comment updates. --- test/OpenTelemetry.Tests/Trace/SamplersTest.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/OpenTelemetry.Tests/Trace/SamplersTest.cs b/test/OpenTelemetry.Tests/Trace/SamplersTest.cs index af1a175a4f2..3851eaa31a8 100644 --- a/test/OpenTelemetry.Tests/Trace/SamplersTest.cs +++ b/test/OpenTelemetry.Tests/Trace/SamplersTest.cs @@ -161,18 +161,22 @@ public void SamplersCanModifyTraceState(SamplingDecision sampling) } else if (sampling == SamplingDecision.RecordOnly) { + // TODO: Update this when repo consumes DS v10. // Note: Seems to be a bug in DiagnosticSource. Root in this case // inherits context from the remote parent and Recorded doesn't get - // cleared + // cleared. This should be fixed in .NET 10: + // https://github.com/dotnet/runtime/pull/111289 // Assert.False(root.Recorded); Assert.True(root.IsAllDataRequested); } else { + // TODO: Update this when repo consumes DS v10. // Note: Seems to be a bug in DiagnosticSource. Root in this case // inherits context from the remote parent and Recorded doesn't get - // cleared + // cleared. This should be fixed in .NET 10: + // https://github.com/dotnet/runtime/pull/111289 // Assert.False(root.Recorded); Assert.False(root.IsAllDataRequested);