From 90d9a91cd1ab8d7d3eed7d2beb1f19e5979d9a1e Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Fri, 5 Apr 2024 13:20:53 -0700 Subject: [PATCH 1/3] Minor refactoring for AggregatorStore and MetricPoint (#5510) --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 85 ++++++++++++-------- src/OpenTelemetry/Metrics/MetricPoint.cs | 5 +- 2 files changed, 55 insertions(+), 35 deletions(-) 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; From dbcc51190322c4343bb253278241255bad2f6401 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 5 Apr 2024 14:12:11 -0700 Subject: [PATCH 2/3] [opentracing-shim] Add missing XML comments (#5512) Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> --- .../OpenTelemetry.Shims.OpenTracing.csproj | 5 ----- src/OpenTelemetry.Shims.OpenTracing/TracerShim.cs | 13 +++++++++++++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/OpenTelemetry.Shims.OpenTracing/OpenTelemetry.Shims.OpenTracing.csproj b/src/OpenTelemetry.Shims.OpenTracing/OpenTelemetry.Shims.OpenTracing.csproj index 35295220a1f..5cb3b56a022 100644 --- a/src/OpenTelemetry.Shims.OpenTracing/OpenTelemetry.Shims.OpenTracing.csproj +++ b/src/OpenTelemetry.Shims.OpenTracing/OpenTelemetry.Shims.OpenTracing.csproj @@ -3,11 +3,6 @@ $(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); From e46a446c4a8a850b024645f697fbb82db602358a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20=C5=81ach?= Date: Sat, 6 Apr 2024 00:06:37 +0200 Subject: [PATCH 3/3] [api-baggage] fix encoding of space chars in baggage item value (#5303) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk Co-authored-by: Mikel Blanchard --- src/OpenTelemetry.Api/CHANGELOG.md | 5 +++++ .../Context/Propagation/BaggagePropagator.cs | 7 +++---- .../Trace/Propagation/BaggagePropagatorTest.cs | 17 +++++++++++++---- 3 files changed, 21 insertions(+), 8 deletions(-) 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> { 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]); } }