[api] Fix Fields in CompositeTextMapPropagator

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

src/OpenTelemetry.Api/Context/Propagation/CompositeTextMapPropagator.cs

@@ -11,8 +11,8 @@ namespace OpenTelemetry.Context.Propagation;
 /// </summary>
 public class CompositeTextMapPropagator : TextMapPropagator
 {
-    private static readonly ISet<string> EmptyFields = new HashSet<string>();
-    private readonly List<TextMapPropagator> propagators;
+    private readonly IReadOnlyList<TextMapPropagator> propagators;
+    private readonly ISet<string> allFields;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="CompositeTextMapPropagator"/> class.
@@ -22,11 +22,48 @@ public CompositeTextMapPropagator(IEnumerable<TextMapPropagator> propagators)
     {
         Guard.ThrowIfNull(propagators);
 
-        this.propagators = new List<TextMapPropagator>(propagators);
+        var propagatorsList = new List<TextMapPropagator>();
+
+        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<string>();
+        }
+        else
+        {
+            ISet<string> fields = this.propagators[0].Fields;
+
+            var output = fields is not null
+                ? new HashSet<string>(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;
+        }
     }
 
     /// <inheritdoc/>
-    public override ISet<string> Fields => EmptyFields;
+    public override ISet<string> Fields => this.allFields;
 
     /// <inheritdoc/>
     public override PropagationContext Extract<T>(PropagationContext context, T carrier, Func<T, string, IEnumerable<string>> getter)

test/OpenTelemetry.Api.Tests/Trace/Propagation/CompositePropagatorTest.cs → 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<ArgumentNullException>(() => 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<TextMapPropagator>
-        {
-            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<string, string>();
         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<string, string>(), 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<TextMapPropagator>
-        {
-            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<string, string>();
 
         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<string, string> { ["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<string, string>();
         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<string, string>();

test/OpenTelemetry.Api.Tests/Trace/Propagation/TestPropagator.cs → test/OpenTelemetry.Api.Tests/Context/Propagation/TestPropagator.cs

@@ -11,23 +11,32 @@ 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;
         this.stateHeaderName = stateHeaderName;
         this.defaultContext = defaultContext;
     }
 
+    public int ExtractCount => this.extractCount;
+
+    public int InjectCount => this.injectCount;
+
     public override ISet<string> Fields => new HashSet<string>() { this.idHeaderName, this.stateHeaderName };
 
     public override PropagationContext Extract<T>(PropagationContext context, T carrier, Func<T, string, IEnumerable<string>> getter)
     {
+        Interlocked.Increment(ref this.extractCount);
+
         if (this.defaultContext)
         {
             return context;
         }
 
-        IEnumerable<string> id = getter(carrier, this.idHeaderName);
+        var id = getter(carrier, this.idHeaderName);
         if (!id.Any())
         {
             return context;
@@ -39,8 +48,8 @@ public override PropagationContext Extract<T>(PropagationContext context, T carr
             return context;
         }
 
-        string tracestate = string.Empty;
-        IEnumerable<string> 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<T>(PropagationContext context, T carr
 
     public override void Inject<T>(PropagationContext context, T carrier, Action<T, string, string> 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);

test/OpenTelemetry.Api.Tests/OpenTelemetry.Api.Tests.csproj

@@ -9,6 +9,7 @@
   </PropertyGroup>
 
   <ItemGroup>
+    <ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Api\OpenTelemetry.Api.csproj" />
     <ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Exporter.InMemory\OpenTelemetry.Exporter.InMemory.csproj" />
   </ItemGroup>