diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md index 63046fa589d..5dfd8dc48e2 100644 --- a/src/OpenTelemetry.Api/CHANGELOG.md +++ b/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 diff --git a/src/OpenTelemetry.Api/Context/Propagation/BaggagePropagator.cs b/src/OpenTelemetry.Api/Context/Propagation/BaggagePropagator.cs index 57bdb453b8f..e9dc29d950d 100644 --- a/src/OpenTelemetry.Api/Context/Propagation/BaggagePropagator.cs +++ b/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(PropagationContext context, T carrier, Action$(TargetFrameworksForLibraries) OpenTracing shim for OpenTelemetry .NET $(PackageTags);distributed-tracing;OpenTracing - true - - $(NoWarn),1591 disable diff --git a/src/OpenTelemetry.Shims.OpenTracing/TracerShim.cs b/src/OpenTelemetry.Shims.OpenTracing/TracerShim.cs index 1c16ac476fd..3ed448da945 100644 --- a/src/OpenTelemetry.Shims.OpenTracing/TracerShim.cs +++ b/src/OpenTelemetry.Shims.OpenTracing/TracerShim.cs @@ -7,16 +7,29 @@ namespace OpenTelemetry.Shims.OpenTracing; +/// +/// Implements OpenTracing interface +/// using OpenTelemetry implementation. +/// public class TracerShim : global::OpenTracing.ITracer { private readonly Trace.Tracer tracer; private readonly TextMapPropagator definedPropagator; + /// + /// Initializes a new instance of the class. + /// + /// . public TracerShim(Trace.TracerProvider tracerProvider) : this(tracerProvider, null) { } + /// + /// Initializes a new instance of the class. + /// + /// . + /// . public TracerShim(Trace.TracerProvider tracerProvider, TextMapPropagator textFormat) { Guard.ThrowIfNull(tracerProvider); diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index 5271a329e40..5022e48dee1 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/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() { diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index e1fe8e1695c..aa3974e2537 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -1197,9 +1197,10 @@ internal void TakeSnapshotWithExemplar(bool outputDelta) } /// - /// 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. /// - internal void Reclaim() + internal void NullifyMetricPointState() { this.LookupData = null; this.mpComponents = null; diff --git a/test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs b/test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs index 097f63e039d..9d8a8972586 100644 --- a/test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs +++ b/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> { new KeyValuePair(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]); } }