Merge branch 'main' into instrumentation-scope-version

src/OpenTelemetry.Api/CHANGELOG.md

@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+* **Breaking change:** Fix space character encoding from `+` to `%20`
+  for baggage item values when propagating baggage as defined in
+  [W3C Baggage propagation format specification](https://www.w3.org/TR/baggage/).
+  ([#5303](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5303))
+
 ## 1.8.0
 
 Released 2024-Apr-02

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

@@ -1,7 +1,6 @@
 // Copyright The OpenTelemetry Authors
 // SPDX-License-Identifier: Apache-2.0
 
-using System.Net;
 using System.Text;
 using OpenTelemetry.Internal;
 
@@ -94,7 +93,7 @@ public override void Inject<T>(PropagationContext context, T carrier, Action<T,
                     continue;
                 }
 
-                baggage.Append(WebUtility.UrlEncode(item.Key)).Append('=').Append(WebUtility.UrlEncode(item.Value)).Append(',');
+                baggage.Append(Uri.EscapeDataString(item.Key)).Append('=').Append(Uri.EscapeDataString(item.Value)).Append(',');
             }
             while (e.MoveNext() && ++itemCount < MaxBaggageItems && baggage.Length < MaxBaggageLength);
             baggage.Remove(baggage.Length - 1, 1);
@@ -141,8 +140,8 @@ internal static bool TryExtractBaggage(string[] baggageCollection, out Dictionar
                     continue;
                 }
 
-                var key = WebUtility.UrlDecode(parts[0]);
-                var value = WebUtility.UrlDecode(parts[1]);
+                var key = Uri.UnescapeDataString(parts[0]);
+                var value = Uri.UnescapeDataString(parts[1]);
 
                 if (string.IsNullOrEmpty(key) || string.IsNullOrEmpty(value))
                 {

src/OpenTelemetry.Shims.OpenTracing/OpenTelemetry.Shims.OpenTracing.csproj

@@ -3,11 +3,6 @@
     <TargetFrameworks>$(TargetFrameworksForLibraries)</TargetFrameworks>
     <Description>OpenTracing shim for OpenTelemetry .NET</Description>
     <PackageTags>$(PackageTags);distributed-tracing;OpenTracing</PackageTags>
-    <IsPackable>true</IsPackable>
-    <!--
-    TODO: Disable this exception, and actually do document all public API.
-    -->
-    <NoWarn>$(NoWarn),1591</NoWarn>
 
     <!-- this is temporary. will remove in future PR. -->
     <Nullable>disable</Nullable>

src/OpenTelemetry.Shims.OpenTracing/TracerShim.cs

@@ -7,16 +7,29 @@
 
 namespace OpenTelemetry.Shims.OpenTracing;
 
+/// <summary>
+/// Implements OpenTracing <see cref="global::OpenTracing.ITracer"/> interface
+/// using OpenTelemetry <see cref="Trace.Tracer"/> implementation.
+/// </summary>
 public class TracerShim : global::OpenTracing.ITracer
 {
     private readonly Trace.Tracer tracer;
     private readonly TextMapPropagator definedPropagator;
 
+    /// <summary>
+    /// Initializes a new instance of the <see cref="TracerShim"/> class.
+    /// </summary>
+    /// <param name="tracerProvider"><see cref="Trace.TracerProvider"/>.</param>
     public TracerShim(Trace.TracerProvider tracerProvider)
         : this(tracerProvider, null)
     {
     }
 
+    /// <summary>
+    /// Initializes a new instance of the <see cref="TracerShim"/> class.
+    /// </summary>
+    /// <param name="tracerProvider"><see cref="Trace.TracerProvider"/>.</param>
+    /// <param name="textFormat"><see cref="TextMapPropagator"/>.</param>
     public TracerShim(Trace.TracerProvider tracerProvider, TextMapPropagator textFormat)
     {
         Guard.ThrowIfNull(tracerProvider);

src/OpenTelemetry/Metrics/AggregatorStore.cs

@@ -270,39 +270,7 @@ internal void SnapshotDeltaWithMetricPointReclaim()
                 // the MetricPoint can be reused for other tags.
                 if (metricPoint.LookupData != null && Interlocked.CompareExchange(ref metricPoint.ReferenceCount, int.MinValue, 0) == 0)
                 {
-                    var lookupData = metricPoint.LookupData;
-
-                    metricPoint.Reclaim();
-
-                    Debug.Assert(this.TagsToMetricPointIndexDictionaryDelta != null, "this.tagsToMetricPointIndexDictionaryDelta was null");
-
-                    lock (this.TagsToMetricPointIndexDictionaryDelta!)
-                    {
-                        LookupData? dictionaryValue;
-                        if (lookupData.SortedTags != Tags.EmptyTags)
-                        {
-                            // Check if no other thread added a new entry for the same Tags.
-                            // If no, then remove the existing entries.
-                            if (this.TagsToMetricPointIndexDictionaryDelta.TryGetValue(lookupData.SortedTags, out dictionaryValue) &&
-                                dictionaryValue == lookupData)
-                            {
-                                this.TagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.SortedTags, out var _);
-                                this.TagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.GivenTags, out var _);
-                            }
-                        }
-                        else
-                        {
-                            if (this.TagsToMetricPointIndexDictionaryDelta.TryGetValue(lookupData.GivenTags, out dictionaryValue) &&
-                                dictionaryValue == lookupData)
-                            {
-                                this.TagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.GivenTags, out var _);
-                            }
-                        }
-
-                        Debug.Assert(this.availableMetricPoints != null, "this.availableMetricPoints was null");
-
-                        this.availableMetricPoints!.Enqueue(i);
-                    }
+                    this.ReclaimMetricPoint(ref metricPoint, i);
                 }
 
                 continue;
@@ -372,6 +340,57 @@ private void TakeMetricPointSnapshot(ref MetricPoint metricPoint, bool outputDel
         }
     }
 
+    private void ReclaimMetricPoint(ref MetricPoint metricPoint, int metricPointIndex)
+    {
+        /*
+         This method does three things:
+          1. Set `metricPoint.LookupData` and `metricPoint.mpComponents` to `null` to have them collected faster by GC.
+          2. Tries to remove the entry for this MetricPoint from the lookup dictionary. An update thread which retrieves this
+             MetricPoint would realize that the MetricPoint is not valid for use since its reference count would have been set to a negative number.
+             When that happens, the update thread would also try to remove the entry for this MetricPoint from the lookup dictionary.
+             We only care about the entry getting removed from the lookup dictionary and not about which thread removes it.
+          3. Put the array index of this MetricPoint to the queue of available metric points. This makes it available for update threads
+             to use this MetricPoint to track newer dimension combinations.
+        */
+
+        var lookupData = metricPoint.LookupData;
+
+        // This method is only called after checking that `metricPoint.LookupData` is not `null`.
+        Debug.Assert(lookupData != null, "LookupData for the provided MetricPoint was null");
+
+        metricPoint.NullifyMetricPointState();
+
+        Debug.Assert(this.TagsToMetricPointIndexDictionaryDelta != null, "this.tagsToMetricPointIndexDictionaryDelta was null");
+
+        lock (this.TagsToMetricPointIndexDictionaryDelta!)
+        {
+            LookupData? dictionaryValue;
+            if (lookupData!.SortedTags != Tags.EmptyTags)
+            {
+                // Check if no other thread added a new entry for the same Tags.
+                // If no, then remove the existing entries.
+                if (this.TagsToMetricPointIndexDictionaryDelta.TryGetValue(lookupData.SortedTags, out dictionaryValue) &&
+                    dictionaryValue == lookupData)
+                {
+                    this.TagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.SortedTags, out var _);
+                    this.TagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.GivenTags, out var _);
+                }
+            }
+            else
+            {
+                if (this.TagsToMetricPointIndexDictionaryDelta.TryGetValue(lookupData.GivenTags, out dictionaryValue) &&
+                    dictionaryValue == lookupData)
+                {
+                    this.TagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.GivenTags, out var _);
+                }
+            }
+
+            Debug.Assert(this.availableMetricPoints != null, "this.availableMetricPoints was null");
+
+            this.availableMetricPoints!.Enqueue(metricPointIndex);
+        }
+    }
+
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     private void InitializeZeroTagPointIfNotInitialized()
     {

src/OpenTelemetry/Metrics/MetricPoint.cs

@@ -1197,9 +1197,10 @@ internal void TakeSnapshotWithExemplar(bool outputDelta)
     }
 
     /// <summary>
-    /// Denote that this MetricPoint is reclaimed.
+    /// This method sets the member object references of MetricPoint to `null`.
+    /// This is done to have them collected faster by GC.
     /// </summary>
-    internal void Reclaim()
+    internal void NullifyMetricPointState()
     {
         this.LookupData = null;
         this.mpComponents = null;

test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs

@@ -131,7 +131,9 @@ public void ValidateSpecialCharsBaggageExtraction()
         Assert.Equal("key%28%293", escapedKey);
         Assert.Equal("value%28%29%21%26%3B%3A", escapedValue);
 
-        var initialBaggage = $"key+1=value+1,{encodedKey}={encodedValue},{escapedKey}={escapedValue}";
+        var initialBaggage =
+            $"key%201=value%201,{encodedKey}={encodedValue},{escapedKey}={escapedValue},key4=%20%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F0123456789%3A%3B%3C%3D%3E%3F%40ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5C%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B%7C%7D~";
+
         var carrier = new List<KeyValuePair<string, string>>
         {
             new KeyValuePair<string, string>(BaggagePropagator.BaggageHeaderName, initialBaggage),
@@ -142,11 +144,11 @@ public void ValidateSpecialCharsBaggageExtraction()
         Assert.False(propagationContext == default);
         Assert.True(propagationContext.ActivityContext == default);
 
-        Assert.Equal(3, propagationContext.Baggage.Count);
+        Assert.Equal(4, propagationContext.Baggage.Count);
 
         var actualBaggage = propagationContext.Baggage.GetBaggage();
 
-        Assert.Equal(3, actualBaggage.Count);
+        Assert.Equal(4, actualBaggage.Count);
 
         Assert.True(actualBaggage.ContainsKey("key 1"));
         Assert.Equal("value 1", actualBaggage["key 1"]);
@@ -156,6 +158,10 @@ public void ValidateSpecialCharsBaggageExtraction()
 
         Assert.True(actualBaggage.ContainsKey("key()3"));
         Assert.Equal("value()!&;:", actualBaggage["key()3"]);
+
+        // x20-x7E range
+        Assert.True(actualBaggage.ContainsKey("key4"));
+        Assert.Equal(" !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", actualBaggage["key4"]);
     }
 
     [Fact]
@@ -195,11 +201,14 @@ public void ValidateSpecialCharsBaggageInjection()
             {
                 { "key 1", "value 1" },
                 { "key2", "!x_x,x-x&x(x\");:" },
+
+                // x20-x7E range
+                { "key3", " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~" },
             }));
 
         this.baggage.Inject(propagationContext, carrier, Setter);
 
         Assert.Single(carrier);
-        Assert.Equal("key+1=value+1,key2=!x_x%2Cx-x%26x(x%22)%3B%3A", carrier[BaggagePropagator.BaggageHeaderName]);
+        Assert.Equal("key%201=value%201,key2=%21x_x%2Cx-x%26x%28x%22%29%3B%3A,key3=%20%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F0123456789%3A%3B%3C%3D%3E%3F%40ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5C%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B%7C%7D~", carrier[BaggagePropagator.BaggageHeaderName]);
     }
 }