From cb9b07fb49d15ded314495ab205fee82485636c4 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 31 Aug 2021 16:40:02 -0700 Subject: [PATCH 1/8] Implement thread-safety in Baggage API. --- src/OpenTelemetry.Api/Baggage.cs | 101 ++++++++++++++++++----- test/OpenTelemetry.Tests/BaggageTests.cs | 17 +++- 2 files changed, 97 insertions(+), 21 deletions(-) diff --git a/src/OpenTelemetry.Api/Baggage.cs b/src/OpenTelemetry.Api/Baggage.cs index 8d55adc68dd..87bcc0bf861 100644 --- a/src/OpenTelemetry.Api/Baggage.cs +++ b/src/OpenTelemetry.Api/Baggage.cs @@ -46,21 +46,34 @@ internal Baggage(Dictionary baggage) /// /// Gets or sets the current . /// + /// + /// Note: returns a copy of the current Baggage. + /// Changes to the returned instance will not automatically be reflected + /// back on the version. To update the version either set to a new + /// instance or use one of the static methods that target as the default source. Examples: + /// + /// Baggage.SetBaggage("newKey1", "newValue1"); // Updates Baggage.Current with 'newkey1' + /// Baggage.SetBaggage("newKey2", "newValue2"); // Updates Baggage.Current with 'newkey2' + /// + /// Or: + /// + /// 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; + /// + /// 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; } /// @@ -143,9 +156,18 @@ public static string GetBaggage(string name, Baggage baggage = default) /// Baggage item value. /// Optional . is used if not specified. /// New containing the key/value pair. + /// Note: The returned will be set as the new instance. [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); + } + } /// /// Returns a new which contains the new key/value pair. @@ -153,9 +175,18 @@ public static Baggage SetBaggage(string name, string value, Baggage baggage = de /// Baggage key/value pairs. /// Optional . is used if not specified. /// New containing the key/value pair. + /// Note: The returned will be set as the new instance. [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> 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); + } + } /// /// Returns a new with the key/value pair removed. @@ -163,16 +194,34 @@ public static Baggage SetBaggage(IEnumerable> bagga /// Baggage item name. /// Optional . is used if not specified. /// New containing the key/value pair. + /// Note: The returned will be set as the new instance. 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); + } + } /// /// Returns a new with all the key/value pairs removed. /// /// Optional . is used if not specified. /// New containing the key/value pair. + /// Note: The returned will be set as the new instance. 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(); + } + } /// /// Returns the name/value pairs in the . @@ -211,7 +260,7 @@ public Baggage SetBaggage(string name, string value) return this.RemoveBaggage(name); } - return Current = new Baggage( + return new Baggage( new Dictionary(this.baggage ?? EmptyBaggage, StringComparer.OrdinalIgnoreCase) { [name] = value, @@ -252,7 +301,7 @@ public Baggage SetBaggage(IEnumerable> baggageItems } } - return Current = new Baggage(newBaggage); + return new Baggage(newBaggage); } /// @@ -265,7 +314,7 @@ public Baggage RemoveBaggage(string name) var baggage = new Dictionary(this.baggage ?? EmptyBaggage, StringComparer.OrdinalIgnoreCase); baggage.Remove(name); - return Current = new Baggage(baggage); + return new Baggage(baggage); } /// @@ -273,7 +322,7 @@ public Baggage RemoveBaggage(string name) /// /// New containing the key/value pair. public Baggage ClearBaggage() - => Current = default; + => default; /// /// Returns an enumerator that iterates through the . @@ -317,6 +366,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; diff --git a/test/OpenTelemetry.Tests/BaggageTests.cs b/test/OpenTelemetry.Tests/BaggageTests.cs index 6904210735c..9ed80f01f4a 100644 --- a/test/OpenTelemetry.Tests/BaggageTests.cs +++ b/test/OpenTelemetry.Tests/BaggageTests.cs @@ -49,7 +49,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)); @@ -147,6 +148,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); @@ -298,5 +300,18 @@ static async Task InnerTask() // key2 & key3 changes don't flow backward automatically } } + + [Fact] + public void ThreadSafetyTest() + { + Baggage.SetBaggage("root", "root"); + + Parallel.For(0, 100, (i) => + { + Baggage.SetBaggage($"key{i}", $"value{i}"); + }); + + Assert.Equal(101, Baggage.Current.Count); + } } } From 960d567672939c04b8e155a80a9865bf0d190a52 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 1 Sep 2021 10:00:24 -0700 Subject: [PATCH 2/8] Code review. --- src/OpenTelemetry.Api/Baggage.cs | 12 ++++++------ test/OpenTelemetry.Tests/BaggageTests.cs | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/OpenTelemetry.Api/Baggage.cs b/src/OpenTelemetry.Api/Baggage.cs index 87bcc0bf861..139e0afb271 100644 --- a/src/OpenTelemetry.Api/Baggage.cs +++ b/src/OpenTelemetry.Api/Baggage.cs @@ -54,19 +54,19 @@ internal Baggage(Dictionary baggage) /// instance or use one of the static methods that target as the default source. Examples: /// - /// Baggage.SetBaggage("newKey1", "newValue1"); // Updates Baggage.Current with 'newkey1' - /// Baggage.SetBaggage("newKey2", "newValue2"); // Updates Baggage.Current with 'newkey2' + /// Baggage.SetBaggage("newKey1", "newValue1"); // Updates Baggage.Current with 'newKey1' + /// Baggage.SetBaggage("newKey2", "newValue2"); // Updates Baggage.Current with 'newKey2' /// /// Or: /// /// var baggageCopy = Baggage.Current; - /// Baggage.SetBaggage("newKey1", "newValue1"); // Updates Baggage.Current with 'newkey1' + /// 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. */ + /// // 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; /// /// diff --git a/test/OpenTelemetry.Tests/BaggageTests.cs b/test/OpenTelemetry.Tests/BaggageTests.cs index cf7c2ca41b3..8c43e4dbe1d 100644 --- a/test/OpenTelemetry.Tests/BaggageTests.cs +++ b/test/OpenTelemetry.Tests/BaggageTests.cs @@ -303,7 +303,7 @@ static async Task InnerTask() [Fact] public void ThreadSafetyTest() { - Baggage.SetBaggage("root", "root"); + Baggage.SetBaggage("rootKey", "rootValue"); // Note: Required to establish a root ExecutionContext containg the BaggageHolder we use as a lock Parallel.For(0, 100, (i) => { From 8eb00e6fca4feb958175cd0abf15a86d0d8dd1f6 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 1 Sep 2021 10:43:42 -0700 Subject: [PATCH 3/8] Fixed broken test. --- .../HttpClientTests.Basic.netcore31.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs index 9083d5145d2..33c85c9277a 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs +++ b/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); From 3ce8e372008b4af4337e8f63c951cd237d2b5430 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 1 Sep 2021 10:53:49 -0700 Subject: [PATCH 4/8] Code review. --- src/OpenTelemetry.Api/Baggage.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/OpenTelemetry.Api/Baggage.cs b/src/OpenTelemetry.Api/Baggage.cs index 139e0afb271..f612886f479 100644 --- a/src/OpenTelemetry.Api/Baggage.cs +++ b/src/OpenTelemetry.Api/Baggage.cs @@ -47,12 +47,12 @@ internal Baggage(Dictionary baggage) /// Gets or sets the current . /// /// - /// Note: returns a copy of the current Baggage. - /// Changes to the returned instance will not automatically be reflected - /// back on the version. To update the version either set to a new - /// instance or use one of the static methods that target as the default source. Examples: + /// Note: returns a forked version of the current + /// Baggage. Changes to the forked version will not automatically be + /// reflected back on . To update either set to a new instance + /// or use one of the static methods that target + /// as the default source. Examples: /// /// Baggage.SetBaggage("newKey1", "newValue1"); // Updates Baggage.Current with 'newKey1' /// Baggage.SetBaggage("newKey2", "newValue2"); // Updates Baggage.Current with 'newKey2' From e3c7788255f294b8309dbb9daada12348c2f8f5d Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 1 Sep 2021 10:56:02 -0700 Subject: [PATCH 5/8] Fix misspell. --- test/OpenTelemetry.Tests/BaggageTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/OpenTelemetry.Tests/BaggageTests.cs b/test/OpenTelemetry.Tests/BaggageTests.cs index 8c43e4dbe1d..db995df2066 100644 --- a/test/OpenTelemetry.Tests/BaggageTests.cs +++ b/test/OpenTelemetry.Tests/BaggageTests.cs @@ -303,7 +303,7 @@ static async Task InnerTask() [Fact] public void ThreadSafetyTest() { - Baggage.SetBaggage("rootKey", "rootValue"); // Note: Required to establish a root ExecutionContext containg the BaggageHolder we use as a lock + Baggage.SetBaggage("rootKey", "rootValue"); // Note: Required to establish a root ExecutionContext containing the BaggageHolder we use as a lock Parallel.For(0, 100, (i) => { From 232af674c67e429639d14c45e18f7d47fd479cc0 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 3 Sep 2021 14:34:47 -0700 Subject: [PATCH 6/8] Code review. --- src/OpenTelemetry.Api/Baggage.cs | 7 ++++--- src/OpenTelemetry.Api/README.md | 15 +++++++-------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/OpenTelemetry.Api/Baggage.cs b/src/OpenTelemetry.Api/Baggage.cs index f612886f479..ac80a5250b5 100644 --- a/src/OpenTelemetry.Api/Baggage.cs +++ b/src/OpenTelemetry.Api/Baggage.cs @@ -50,9 +50,10 @@ internal Baggage(Dictionary baggage) /// Note: returns a forked version of the current /// Baggage. Changes to the forked version will not automatically be /// reflected back on . To update either set to a new instance - /// or use one of the static methods that target - /// as the default source. Examples: + /// cref="Current"/> either use one of the static methods that target + /// as the default source or set to a new instance of . + /// Examples: /// /// Baggage.SetBaggage("newKey1", "newValue1"); // Updates Baggage.Current with 'newKey1' /// Baggage.SetBaggage("newKey2", "newValue2"); // Updates Baggage.Current with 'newKey2' diff --git a/src/OpenTelemetry.Api/README.md b/src/OpenTelemetry.Api/README.md index 5d53ae4cb46..070ecf5412b 100644 --- a/src/OpenTelemetry.Api/README.md +++ b/src/OpenTelemetry.Api/README.md @@ -85,26 +85,25 @@ 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) +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 From 81059872e9981f6dda35708cb26b21517dbe0c54 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 3 Sep 2021 14:39:09 -0700 Subject: [PATCH 7/8] CHANGELOG update. --- src/OpenTelemetry.Api/CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md index 6dae313a6e8..9308df2245a 100644 --- a/src/OpenTelemetry.Api/CHANGELOG.md +++ b/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 From 9799293415279e3e78014cd4308d851a8bd8420f Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 3 Sep 2021 16:38:44 -0700 Subject: [PATCH 8/8] Code review. --- src/OpenTelemetry.Api/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Api/README.md b/src/OpenTelemetry.Api/README.md index 070ecf5412b..9ab0409c151 100644 --- a/src/OpenTelemetry.Api/README.md +++ b/src/OpenTelemetry.Api/README.md @@ -84,7 +84,7 @@ 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 +// Use GetBaggage to get all the key/value pairs present in Baggage foreach (var item in Baggage.GetBaggage()) { Console.WriteLine(item.Key);