diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md index f39d81b108e..54710b1022e 100644 --- a/src/OpenTelemetry.Api/CHANGELOG.md +++ b/src/OpenTelemetry.Api/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +* **Breaking change:** CompositeTextMapPropagator.Fields now returns a + unioned set of fields from all combined propagators. Previously this always + returned an empty set. + ([#5745](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5745)) + ## 1.9.0 Released 2024-Jun-14 diff --git a/src/OpenTelemetry.Api/Context/Propagation/CompositeTextMapPropagator.cs b/src/OpenTelemetry.Api/Context/Propagation/CompositeTextMapPropagator.cs index a6b12c3cd1b..016ef0fda4d 100644 --- a/src/OpenTelemetry.Api/Context/Propagation/CompositeTextMapPropagator.cs +++ b/src/OpenTelemetry.Api/Context/Propagation/CompositeTextMapPropagator.cs @@ -11,8 +11,8 @@ namespace OpenTelemetry.Context.Propagation; /// public class CompositeTextMapPropagator : TextMapPropagator { - private static readonly ISet EmptyFields = new HashSet(); - private readonly List propagators; + private readonly IReadOnlyList propagators; + private readonly ISet allFields; /// /// Initializes a new instance of the class. @@ -22,18 +22,55 @@ public CompositeTextMapPropagator(IEnumerable propagators) { Guard.ThrowIfNull(propagators); - this.propagators = new List(propagators); + var propagatorsList = new List(); + + foreach (var propagator in propagators) + { + if (propagator is not null) + { + propagatorsList.Add(propagator); + } + } + + this.propagators = propagatorsList; + + // For efficiency, we resolve the fields from all propagators only once, as they are + // not expected to change (although the implementation doesn't strictly prevent that). + if (this.propagators.Count == 0) + { + // Use a new empty HashSet for each instance to avoid any potential mutation issues. + this.allFields = new HashSet(); + } + else + { + ISet fields = this.propagators[0].Fields; + + var output = fields is not null + ? new HashSet(fields) + : []; + + for (int i = 1; i < this.propagators.Count; i++) + { + fields = this.propagators[i].Fields; + if (fields is not null) + { + output.UnionWith(fields); + } + } + + this.allFields = output; + } } /// - public override ISet Fields => EmptyFields; + public override ISet Fields => this.allFields; /// public override PropagationContext Extract(PropagationContext context, T carrier, Func> getter) { - foreach (var propagator in this.propagators) + for (int i = 0; i < this.propagators.Count; i++) { - context = propagator.Extract(context, carrier, getter); + context = this.propagators[i].Extract(context, carrier, getter); } return context; @@ -42,9 +79,9 @@ public override PropagationContext Extract(PropagationContext context, T carr /// public override void Inject(PropagationContext context, T carrier, Action setter) { - foreach (var propagator in this.propagators) + for (int i = 0; i < this.propagators.Count; i++) { - propagator.Inject(context, carrier, setter); + this.propagators[i].Inject(context, carrier, setter); } } } diff --git a/test/OpenTelemetry.Api.Tests/Trace/Propagation/B3PropagatorTest.cs b/test/OpenTelemetry.Api.Tests/Context/Propagation/B3PropagatorTest.cs similarity index 100% rename from test/OpenTelemetry.Api.Tests/Trace/Propagation/B3PropagatorTest.cs rename to test/OpenTelemetry.Api.Tests/Context/Propagation/B3PropagatorTest.cs diff --git a/test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs b/test/OpenTelemetry.Api.Tests/Context/Propagation/BaggagePropagatorTest.cs similarity index 100% rename from test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs rename to test/OpenTelemetry.Api.Tests/Context/Propagation/BaggagePropagatorTest.cs diff --git a/test/OpenTelemetry.Api.Tests/Trace/Propagation/CompositePropagatorTest.cs b/test/OpenTelemetry.Api.Tests/Context/Propagation/CompositePropagatorTest.cs similarity index 56% rename from test/OpenTelemetry.Api.Tests/Trace/Propagation/CompositePropagatorTest.cs rename to test/OpenTelemetry.Api.Tests/Context/Propagation/CompositePropagatorTest.cs index f4066edcc1e..0fcf7a1952f 100644 --- a/test/OpenTelemetry.Api.Tests/Trace/Propagation/CompositePropagatorTest.cs +++ b/test/OpenTelemetry.Api.Tests/Context/Propagation/CompositePropagatorTest.cs @@ -31,28 +31,72 @@ public class CompositePropagatorTest private readonly ActivitySpanId spanId = ActivitySpanId.CreateRandom(); [Fact] - public void CompositePropagator_NullTextFormatList() + public void CompositePropagator_NullTextMapPropagators() { Assert.Throws(() => new CompositeTextMapPropagator(null)); } + [Fact] + public void CompositePropagator_EmptyTextMapPropagators() + { + var compositePropagator = new CompositeTextMapPropagator([]); + Assert.Empty(compositePropagator.Fields); + } + + [Fact] + public void CompositePropagator_NullTextMapPropagator() + { + var compositePropagator = new CompositeTextMapPropagator([null]); + Assert.Empty(compositePropagator.Fields); + } + + [Fact] + public void CompositePropagator_NoOpTextMapPropagators() + { + var compositePropagator = new CompositeTextMapPropagator([new NoopTextMapPropagator()]); + Assert.Empty(compositePropagator.Fields); + } + + [Fact] + public void CompositePropagator_SingleTextMapPropagator() + { + var testPropagator = new TestPropagator("custom-traceparent-1", "custom-tracestate-1"); + + var compositePropagator = new CompositeTextMapPropagator([testPropagator]); + + // We expect a new HashSet, with a copy of the values from the propagator. + Assert.Equal(testPropagator.Fields, compositePropagator.Fields); + Assert.NotSame(testPropagator.Fields, compositePropagator.Fields); + } + [Fact] public void CompositePropagator_TestPropagator() { - var compositePropagator = new CompositeTextMapPropagator(new List - { - new TestPropagator("custom-traceparent-1", "custom-tracestate-1"), - new TestPropagator("custom-traceparent-2", "custom-tracestate-2"), - }); + var testPropagatorA = new TestPropagator("custom-traceparent-1", "custom-tracestate-1"); + var testPropagatorB = new TestPropagator("custom-traceparent-2", "custom-tracestate-2"); + + var compositePropagator = new CompositeTextMapPropagator([testPropagatorA, testPropagatorB,]); var activityContext = new ActivityContext(this.traceId, this.spanId, ActivityTraceFlags.Recorded, traceState: null); - PropagationContext propagationContext = new PropagationContext(activityContext, default); + var propagationContext = new PropagationContext(activityContext, default); var carrier = new Dictionary(); using var activity = new Activity("test"); compositePropagator.Inject(propagationContext, carrier, Setter); Assert.Contains(carrier, kv => kv.Key == "custom-traceparent-1"); Assert.Contains(carrier, kv => kv.Key == "custom-traceparent-2"); + + Assert.Equal(testPropagatorA.Fields.Count + testPropagatorB.Fields.Count, compositePropagator.Fields.Count); + Assert.Subset(compositePropagator.Fields, testPropagatorA.Fields); + Assert.Subset(compositePropagator.Fields, testPropagatorB.Fields); + + Assert.Equal(1, testPropagatorA.InjectCount); + Assert.Equal(1, testPropagatorB.InjectCount); + + compositePropagator.Extract(default, new Dictionary(), Getter); + + Assert.Equal(1, testPropagatorA.ExtractCount); + Assert.Equal(1, testPropagatorB.ExtractCount); } [Fact] @@ -61,20 +105,24 @@ public void CompositePropagator_UsingSameTag() const string header01 = "custom-tracestate-01"; const string header02 = "custom-tracestate-02"; - var compositePropagator = new CompositeTextMapPropagator(new List - { - new TestPropagator("custom-traceparent", header01, true), - new TestPropagator("custom-traceparent", header02), - }); + var testPropagatorA = new TestPropagator("custom-traceparent", header01, true); + var testPropagatorB = new TestPropagator("custom-traceparent", header02); + + var compositePropagator = new CompositeTextMapPropagator([testPropagatorA, testPropagatorB,]); var activityContext = new ActivityContext(this.traceId, this.spanId, ActivityTraceFlags.Recorded, traceState: null); - PropagationContext propagationContext = new PropagationContext(activityContext, default); + var propagationContext = new PropagationContext(activityContext, default); var carrier = new Dictionary(); compositePropagator.Inject(propagationContext, carrier, Setter); Assert.Contains(carrier, kv => kv.Key == "custom-traceparent"); + Assert.Equal(3, compositePropagator.Fields.Count); + + Assert.Equal(1, testPropagatorA.InjectCount); + Assert.Equal(1, testPropagatorB.InjectCount); + // checking if the latest propagator is the one with the data. So, it will replace the previous one. Assert.Equal($"00-{this.traceId}-{this.spanId}-{header02.Split('-').Last()}", carrier["custom-traceparent"]); @@ -85,6 +133,9 @@ public void CompositePropagator_UsingSameTag() // checking if we accessed only two times: header/headerstate options // if that's true, we skipped the first one since we have a logic to for the default result Assert.Equal(2, count); + + Assert.Equal(1, testPropagatorA.ExtractCount); + Assert.Equal(1, testPropagatorB.ExtractCount); } [Fact] @@ -99,13 +150,13 @@ public void CompositePropagator_ActivityContext_Baggage() var activityContext = new ActivityContext(this.traceId, this.spanId, ActivityTraceFlags.Recorded, traceState: null, isRemote: true); var baggage = new Dictionary { ["key1"] = "value1" }; - PropagationContext propagationContextActivityOnly = new PropagationContext(activityContext, default); - PropagationContext propagationContextBaggageOnly = new PropagationContext(default, new Baggage(baggage)); - PropagationContext propagationContextBoth = new PropagationContext(activityContext, new Baggage(baggage)); + var propagationContextActivityOnly = new PropagationContext(activityContext, default); + var propagationContextBaggageOnly = new PropagationContext(default, new Baggage(baggage)); + var propagationContextBoth = new PropagationContext(activityContext, new Baggage(baggage)); var carrier = new Dictionary(); compositePropagator.Inject(propagationContextActivityOnly, carrier, Setter); - PropagationContext extractedContext = compositePropagator.Extract(default, carrier, Getter); + var extractedContext = compositePropagator.Extract(default, carrier, Getter); Assert.Equal(propagationContextActivityOnly, extractedContext); carrier = new Dictionary(); diff --git a/test/OpenTelemetry.Api.Tests/Trace/Propagation/TestPropagator.cs b/test/OpenTelemetry.Api.Tests/Context/Propagation/TestPropagator.cs similarity index 78% rename from test/OpenTelemetry.Api.Tests/Trace/Propagation/TestPropagator.cs rename to test/OpenTelemetry.Api.Tests/Context/Propagation/TestPropagator.cs index abd4a17b1e9..f07875c5d28 100644 --- a/test/OpenTelemetry.Api.Tests/Trace/Propagation/TestPropagator.cs +++ b/test/OpenTelemetry.Api.Tests/Context/Propagation/TestPropagator.cs @@ -11,6 +11,9 @@ public class TestPropagator : TextMapPropagator private readonly string stateHeaderName; private readonly bool defaultContext; + private int extractCount = 0; + private int injectCount = 0; + public TestPropagator(string idHeaderName, string stateHeaderName, bool defaultContext = false) { this.idHeaderName = idHeaderName; @@ -18,16 +21,22 @@ public TestPropagator(string idHeaderName, string stateHeaderName, bool defaultC this.defaultContext = defaultContext; } + public int ExtractCount => this.extractCount; + + public int InjectCount => this.injectCount; + public override ISet Fields => new HashSet() { this.idHeaderName, this.stateHeaderName }; public override PropagationContext Extract(PropagationContext context, T carrier, Func> getter) { + Interlocked.Increment(ref this.extractCount); + if (this.defaultContext) { return context; } - IEnumerable id = getter(carrier, this.idHeaderName); + var id = getter(carrier, this.idHeaderName); if (!id.Any()) { return context; @@ -39,8 +48,8 @@ public override PropagationContext Extract(PropagationContext context, T carr return context; } - string tracestate = string.Empty; - IEnumerable tracestateCollection = getter(carrier, this.stateHeaderName); + var tracestate = string.Empty; + var tracestateCollection = getter(carrier, this.stateHeaderName); if (tracestateCollection?.Any() ?? false) { TraceContextPropagator.TryExtractTracestate(tracestateCollection.ToArray(), out tracestate); @@ -53,14 +62,16 @@ public override PropagationContext Extract(PropagationContext context, T carr public override void Inject(PropagationContext context, T carrier, Action setter) { - string headerNumber = this.stateHeaderName.Split('-').Last(); + Interlocked.Increment(ref this.injectCount); + + var headerNumber = this.stateHeaderName.Split('-').Last(); var traceparent = string.Concat("00-", context.ActivityContext.TraceId.ToHexString(), "-", context.ActivityContext.SpanId.ToHexString()); traceparent = string.Concat(traceparent, "-", headerNumber); setter(carrier, this.idHeaderName, traceparent); - string tracestateStr = context.ActivityContext.TraceState; + var tracestateStr = context.ActivityContext.TraceState; if (tracestateStr?.Length > 0) { setter(carrier, this.stateHeaderName, tracestateStr); diff --git a/test/OpenTelemetry.Api.Tests/Trace/Propagation/TracestateUtilsTests.cs b/test/OpenTelemetry.Api.Tests/Context/Propagation/TracestateUtilsTests.cs similarity index 100% rename from test/OpenTelemetry.Api.Tests/Trace/Propagation/TracestateUtilsTests.cs rename to test/OpenTelemetry.Api.Tests/Context/Propagation/TracestateUtilsTests.cs diff --git a/test/OpenTelemetry.Api.Tests/OpenTelemetry.Api.Tests.csproj b/test/OpenTelemetry.Api.Tests/OpenTelemetry.Api.Tests.csproj index b2e0fa8b6db..08073d127f7 100644 --- a/test/OpenTelemetry.Api.Tests/OpenTelemetry.Api.Tests.csproj +++ b/test/OpenTelemetry.Api.Tests/OpenTelemetry.Api.Tests.csproj @@ -9,6 +9,7 @@ +