Baggage API: Thread-safety

src/OpenTelemetry.Api/Baggage.cs

@@ -46,21 +46,35 @@ internal Baggage(Dictionary<string, string> baggage)
         /// <summary>
         /// Gets or sets the current <see cref="Baggage"/>.
         /// </summary>
+        /// <remarks>
+        /// Note: <see cref="Current"/> returns a forked version of the current
+        /// Baggage. Changes to the forked version will not automatically be
+        /// reflected back on <see cref="Current"/>. To update <see
+        /// cref="Current"/> either use one of the static methods that target
+        /// <see cref="Current"/> as the default source or set <see
+        /// cref="Current"/> to a new instance of <see cref="Baggage"/>.
+        /// Examples:
+        /// <code>
+        /// Baggage.SetBaggage("newKey1", "newValue1"); // Updates Baggage.Current with 'newKey1'
+        /// Baggage.SetBaggage("newKey2", "newValue2"); // Updates Baggage.Current with 'newKey2'
+        /// </code>
+        /// Or:
+        /// <code>
+        /// var baggageCopy = Baggage.Current;
+        /// Baggage.SetBaggage("newKey1", "newValue1"); // Updates Baggage.Current with 'newKey1'
+        /// var newBaggage = baggageCopy
+        ///     .SetBaggage("newKey2", "newValue2");
+        ///     .SetBaggage("newKey3", "newValue3");
+        /// // Sets Baggage.Current to 'newBaggage' which will override any
+        /// // changes made to Baggage.Current after the copy was made. For example
+        /// // the 'newKey1' change is lost.
+        /// Baggage.Current = newBaggage;
+        /// </code>
+        /// </remarks>
         public static Baggage Current
         {
             get => RuntimeContextSlot.Get()?.Baggage ?? default;
-            set
-            {
-                var baggageHolder = RuntimeContextSlot.Get();
-                if (baggageHolder == null)
-                {
-                    RuntimeContextSlot.Set(new BaggageHolder { Baggage = value });
-                }
-                else
-                {
-                    baggageHolder.Baggage = value;
-                }
-            }
+            set => EnsureBaggageHolder().Baggage = value;
         }
 
         /// <summary>
@@ -143,36 +157,72 @@ public static string GetBaggage(string name, Baggage baggage = default)
         /// <param name="value">Baggage item value.</param>
         /// <param name="baggage">Optional <see cref="Baggage"/>. <see cref="Current"/> is used if not specified.</param>
         /// <returns>New <see cref="Baggage"/> containing the key/value pair.</returns>
+        /// <remarks>Note: The <see cref="Baggage"/> returned will be set as the new <see cref="Current"/> instance.</remarks>
         [System.Diagnostics.CodeAnalysis.SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "This was agreed on to be the friendliest API surface")]
         public static Baggage SetBaggage(string name, string value, Baggage baggage = default)
-            => baggage == default ? Current.SetBaggage(name, value) : baggage.SetBaggage(name, value);
+        {
+            var baggageHolder = EnsureBaggageHolder();
+            lock (baggageHolder)
+            {
+                return baggageHolder.Baggage = baggage == default
+                    ? baggageHolder.Baggage.SetBaggage(name, value)
+                    : baggage.SetBaggage(name, value);
+            }
+        }
 
         /// <summary>
         /// Returns a new <see cref="Baggage"/> which contains the new key/value pair.
         /// </summary>
         /// <param name="baggageItems">Baggage key/value pairs.</param>
         /// <param name="baggage">Optional <see cref="Baggage"/>. <see cref="Current"/> is used if not specified.</param>
         /// <returns>New <see cref="Baggage"/> containing the key/value pair.</returns>
+        /// <remarks>Note: The <see cref="Baggage"/> returned will be set as the new <see cref="Current"/> instance.</remarks>
         [System.Diagnostics.CodeAnalysis.SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "This was agreed on to be the friendliest API surface")]
         public static Baggage SetBaggage(IEnumerable<KeyValuePair<string, string>> baggageItems, Baggage baggage = default)
-           => baggage == default ? Current.SetBaggage(baggageItems) : baggage.SetBaggage(baggageItems);
+        {
+            var baggageHolder = EnsureBaggageHolder();
+            lock (baggageHolder)
+            {
+                return baggageHolder.Baggage = baggage == default
+                    ? baggageHolder.Baggage.SetBaggage(baggageItems)
+                    : baggage.SetBaggage(baggageItems);
+            }
+        }
 
         /// <summary>
         /// Returns a new <see cref="Baggage"/> with the key/value pair removed.
         /// </summary>
         /// <param name="name">Baggage item name.</param>
         /// <param name="baggage">Optional <see cref="Baggage"/>. <see cref="Current"/> is used if not specified.</param>
         /// <returns>New <see cref="Baggage"/> containing the key/value pair.</returns>
+        /// <remarks>Note: The <see cref="Baggage"/> returned will be set as the new <see cref="Current"/> instance.</remarks>
         public static Baggage RemoveBaggage(string name, Baggage baggage = default)
-            => baggage == default ? Current.RemoveBaggage(name) : baggage.RemoveBaggage(name);
+        {
+            var baggageHolder = EnsureBaggageHolder();
+            lock (baggageHolder)
+            {
+                return baggageHolder.Baggage = baggage == default
+                    ? baggageHolder.Baggage.RemoveBaggage(name)
+                    : baggage.RemoveBaggage(name);
+            }
+        }
 
         /// <summary>
         /// Returns a new <see cref="Baggage"/> with all the key/value pairs removed.
         /// </summary>
         /// <param name="baggage">Optional <see cref="Baggage"/>. <see cref="Current"/> is used if not specified.</param>
         /// <returns>New <see cref="Baggage"/> containing the key/value pair.</returns>
+        /// <remarks>Note: The <see cref="Baggage"/> returned will be set as the new <see cref="Current"/> instance.</remarks>
         public static Baggage ClearBaggage(Baggage baggage = default)
-            => baggage == default ? Current.ClearBaggage() : baggage.ClearBaggage();
+        {
+            var baggageHolder = EnsureBaggageHolder();
+            lock (baggageHolder)
+            {
+                return baggageHolder.Baggage = baggage == default
+                    ? baggageHolder.Baggage.ClearBaggage()
+                    : baggage.ClearBaggage();
+            }
+        }
 
         /// <summary>
         /// Returns the name/value pairs in the <see cref="Baggage"/>.
@@ -211,7 +261,7 @@ public Baggage SetBaggage(string name, string value)
                 return this.RemoveBaggage(name);
             }
 
-            return Current = new Baggage(
+            return new Baggage(
                 new Dictionary<string, string>(this.baggage ?? EmptyBaggage, StringComparer.OrdinalIgnoreCase)
                 {
                     [name] = value,
@@ -252,7 +302,7 @@ public Baggage SetBaggage(IEnumerable<KeyValuePair<string, string>> baggageItems
                 }
             }
 
-            return Current = new Baggage(newBaggage);
+            return new Baggage(newBaggage);
         }
 
         /// <summary>
@@ -265,15 +315,15 @@ public Baggage RemoveBaggage(string name)
             var baggage = new Dictionary<string, string>(this.baggage ?? EmptyBaggage, StringComparer.OrdinalIgnoreCase);
             baggage.Remove(name);
 
-            return Current = new Baggage(baggage);
+            return new Baggage(baggage);
         }
 
         /// <summary>
         /// Returns a new <see cref="Baggage"/> with all the key/value pairs removed.
         /// </summary>
         /// <returns>New <see cref="Baggage"/> containing the key/value pair.</returns>
         public Baggage ClearBaggage()
-            => Current = default;
+            => default;
 
         /// <summary>
         /// Returns an enumerator that iterates through the <see cref="Baggage"/>.
@@ -317,6 +367,18 @@ public override int GetHashCode()
             }
         }
 
+        private static BaggageHolder EnsureBaggageHolder()
+        {
+            var baggageHolder = RuntimeContextSlot.Get();
+            if (baggageHolder == null)
+            {
+                baggageHolder = new BaggageHolder();
+                RuntimeContextSlot.Set(baggageHolder);
+            }
+
+            return baggageHolder;
+        }
+
         private class BaggageHolder
         {
             public Baggage Baggage;

src/OpenTelemetry.Api/CHANGELOG.md

@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+* Static Baggage operations (`SetBaggage`, `RemoveBaggage`, & `ClearBaggage`)
+  are now thread-safe. Instance-based Baggage operations no longer mutate
+  `Baggage.Current` (breaking behavior change). For details see:
+  ([#2298](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2298))
+
 ## 1.2.0-alpha2
 
 Released 2021-Aug-24

src/OpenTelemetry.Api/README.md

@@ -84,27 +84,26 @@ propagated out of proc using
 OpenTelemetry SDK ships a BaggagePropagator and enables it by default.
 
 ```csharp
-// Use Baggage.Current to get all the key/value pairs present in Baggage
-foreach (var item in Baggage.Current)
+// Use GetBaggage to get all the key/value pairs present in Baggage
+foreach (var item in Baggage.GetBaggage())
 {
     Console.WriteLine(item.Key);
     Console.WriteLine(item.Value);
 }
 
 // Use SetBaggage method to add a key/value pair in Baggage
-Baggage.Current.SetBaggage("AppName", "MyApp");
-Baggage.Current.SetBaggage("Region", "West US");
+Baggage.SetBaggage("AppName", "MyApp");
+Baggage.SetBaggage("Region", "West US");
 
 // Use RemoveBaggage method to remove a key/value pair in Baggage
-Baggage.Current.RemoveBaggage("AppName");
+Baggage.RemoveBaggage("AppName");
 
 // Use ClearBaggage method to remove all the key/value pairs in Baggage
-Baggage.Current.ClearBaggage();
+Baggage.ClearBaggage();
 ```
 
-The recommended way to add Baggage is to use `SetBaggage()` on
-`Baggage.Current`. OpenTelemetry users should not use the method `AddBaggage` on
-`Activity`.
+The recommended way to add Baggage is to use the `Baggage.SetBaggage()` API.
+OpenTelemetry users should not use the `Activity.AddBaggage` method.
 
 ## Introduction to OpenTelemetry .NET Tracing API
 

test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs

@@ -375,11 +375,11 @@ public async Task HttpClientInstrumentationContextPropagation()
                 .Start();
             parent.TraceStateString = "k1=v1,k2=v2";
             parent.ActivityTraceFlags = ActivityTraceFlags.Recorded;
-            Baggage.Current.SetBaggage("b1", "v1");
+            Baggage.SetBaggage("b1", "v1");
             using (Sdk.CreateTracerProviderBuilder()
-                        .AddHttpClientInstrumentation()
-                        .AddProcessor(processor.Object)
-                        .Build())
+                .AddHttpClientInstrumentation()
+                .AddProcessor(processor.Object)
+                .Build())
             {
                 using var c = new HttpClient();
                 await c.SendAsync(request);

test/OpenTelemetry.Tests/BaggageTests.cs

@@ -48,7 +48,8 @@ public void SetAndGetTest()
             };
 
             Baggage.SetBaggage(K1, V1);
-            Baggage.Current.SetBaggage(K2, V2);
+            var baggage = Baggage.Current.SetBaggage(K2, V2);
+            Baggage.Current = baggage;
 
             Assert.NotEmpty(Baggage.GetBaggage());
             Assert.Equal(list, Baggage.GetBaggage(Baggage.Current));
@@ -146,6 +147,7 @@ public void ContextFlowTest()
         {
             var baggage = Baggage.SetBaggage(K1, V1);
             var baggage2 = Baggage.Current.SetBaggage(K2, V2);
+            Baggage.Current = baggage2;
             var baggage3 = Baggage.SetBaggage(K3, V3);
 
             Assert.Equal(1, baggage.Count);
@@ -297,5 +299,18 @@ static async Task InnerTask()
                 // key2 & key3 changes don't flow backward automatically
             }
         }
+
+        [Fact]
+        public void ThreadSafetyTest()
+        {
+            Baggage.SetBaggage("rootKey", "rootValue"); // Note: Required to establish a root ExecutionContext containing the BaggageHolder we use as a lock
+
+            Parallel.For(0, 100, (i) =>
+            {
+                Baggage.SetBaggage($"key{i}", $"value{i}");
+            });
+
+            Assert.Equal(101, Baggage.Current.Count);
+        }
     }
 }