Optimize unnecessarily allocation when initializing MetricProvider (#1685)

* Optimize unnecessary allocation when using GetOrAdd() on Collection objects

* Refactor SortAndDedup method for LabelSets

* Add Tests for Double versions of Counter and Measures

* Refactor to use TryGetValue/TryGetOrAdd instead of Func<> due to concerns over performance.

* Fix CR/LF to LF

* Refactor to avoid allocation of Func<>

* Fix styling issue

* kick build to start

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

src/OpenTelemetry/Metrics/CounterMetricSdkBase.cs

@@ -14,6 +14,7 @@
 // limitations under the License.
 // </copyright>
 
+using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 
@@ -28,11 +29,16 @@ namespace OpenTelemetry.Metrics
         private readonly ConcurrentDictionary<LabelSet, BoundCounterMetricSdkBase<T>> counterBoundInstruments =
             new ConcurrentDictionary<LabelSet, BoundCounterMetricSdkBase<T>>();
 
+        private readonly Func<LabelSet, BoundCounterMetricSdkBase<T>> createBoundMetricFunc;
+        private readonly Func<LabelSet, BoundCounterMetricSdkBase<T>> createShortLivedMetricFunc;
+
         private string metricName;
 
         protected CounterMetricSdkBase(string name)
         {
             this.metricName = name;
+            this.createBoundMetricFunc = (_) => this.CreateMetric(RecordStatus.Bound);
+            this.createShortLivedMetricFunc = (_) => this.CreateMetric(RecordStatus.UpdatePending);
         }
 
         public ConcurrentDictionary<LabelSet, BoundCounterMetricSdkBase<T>> GetAllBoundInstruments()
@@ -58,8 +64,9 @@ namespace OpenTelemetry.Metrics
 
             lock (this.bindUnbindLock)
             {
-                var recStatus = isShortLived ? RecordStatus.UpdatePending : RecordStatus.Bound;
-                boundInstrument = this.counterBoundInstruments.GetOrAdd(labelset, this.CreateMetric(recStatus));
+                boundInstrument = this.counterBoundInstruments.GetOrAdd(
+                    labelset,
+                    isShortLived ? this.createShortLivedMetricFunc : this.createBoundMetricFunc);
             }
 
             switch (boundInstrument.Status)
@@ -97,7 +104,7 @@ namespace OpenTelemetry.Metrics
                         {
                             boundInstrument.Status = RecordStatus.UpdatePending;
 
-                            this.counterBoundInstruments.GetOrAdd(labelset, boundInstrument);
+                            boundInstrument = this.counterBoundInstruments.GetOrAdd(labelset, boundInstrument);
                         }
 
                         break;

src/OpenTelemetry/Metrics/DoubleObserverMetricSdk.cs

@@ -22,6 +22,8 @@ namespace OpenTelemetry.Metrics
 {
     internal class DoubleObserverMetricSdk : DoubleObserverMetric
     {
+        private static readonly Func<LabelSet, DoubleObserverMetricHandleSdk> NewDoubleObserverMetricHandleSdkFunc = (_) => new DoubleObserverMetricHandleSdk();
+
         private readonly ConcurrentDictionary<LabelSet, DoubleObserverMetricHandleSdk> observerHandles = new ConcurrentDictionary<LabelSet, DoubleObserverMetricHandleSdk>();
         private readonly string metricName;
         private readonly Action<DoubleObserverMetric> callback;
@@ -35,9 +37,7 @@ namespace OpenTelemetry.Metrics
         public override void Observe(double value, LabelSet labelset)
         {
             // TODO cleanup of handle/aggregator.   Issue #530
-            var boundInstrument =
-                this.observerHandles.GetOrAdd(labelset, new DoubleObserverMetricHandleSdk());
-
+            var boundInstrument = this.observerHandles.GetOrAdd(labelset, NewDoubleObserverMetricHandleSdkFunc);
             boundInstrument.Observe(value);
         }
 

src/OpenTelemetry/Metrics/Int64ObserverMetricSdk.cs

@@ -22,6 +22,8 @@ namespace OpenTelemetry.Metrics
 {
     internal class Int64ObserverMetricSdk : Int64ObserverMetric
     {
+        private static readonly Func<LabelSet, Int64ObserverMetricHandleSdk> NewInt64ObserverMetricHandleSdkFunc = (_) => new Int64ObserverMetricHandleSdk();
+
         private readonly ConcurrentDictionary<LabelSet, Int64ObserverMetricHandleSdk> observerHandles = new ConcurrentDictionary<LabelSet, Int64ObserverMetricHandleSdk>();
         private readonly string metricName;
         private readonly Action<Int64ObserverMetric> callback;
@@ -35,9 +37,7 @@ namespace OpenTelemetry.Metrics
         public override void Observe(long value, LabelSet labelset)
         {
             // TODO cleanup of handle/aggregator.   Issue #530
-            var boundInstrument =
-                this.observerHandles.GetOrAdd(labelset, new Int64ObserverMetricHandleSdk());
-
+            var boundInstrument = this.observerHandles.GetOrAdd(labelset, NewInt64ObserverMetricHandleSdkFunc);
             boundInstrument.Observe(value);
         }
 

src/OpenTelemetry/Metrics/LabelSetSdk.cs

@@ -62,31 +62,13 @@ namespace OpenTelemetry.Metrics
 
         private static IEnumerable<KeyValuePair<string, string>> SortAndDedup(IEnumerable<KeyValuePair<string, string>> labels)
         {
-            // TODO - could be optimized to avoid creating List twice.
-            var orderedList = labels.OrderBy(x => x.Key).ToList();
-            if (orderedList.Count == 1)
+            var dedupedList = new SortedDictionary<string, KeyValuePair<string, string>>(StringComparer.Ordinal);
+            foreach (var label in labels)
             {
-                return orderedList;
+                dedupedList[label.Key] = label;
             }
 
-            var dedupedList = new List<KeyValuePair<string, string>>();
-
-            int dedupedListIndex = 0;
-            dedupedList.Add(orderedList[dedupedListIndex]);
-            for (int i = 1; i < orderedList.Count; i++)
-            {
-                if (orderedList[i].Key.Equals(orderedList[i - 1].Key, StringComparison.Ordinal))
-                {
-                    dedupedList[dedupedListIndex] = orderedList[i];
-                }
-                else
-                {
-                    dedupedList.Add(orderedList[i]);
-                    dedupedListIndex++;
-                }
-            }
-
-            return dedupedList;
+            return dedupedList.Values;
         }
 
         private static string GetLabelSetEncoded(IEnumerable<KeyValuePair<string, string>> labels)

src/OpenTelemetry/Metrics/MeasureMetricSdk.cs

@@ -14,6 +14,7 @@
 // limitations under the License.
 // </copyright>
 
+using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 
@@ -23,16 +24,18 @@ namespace OpenTelemetry.Metrics
         where T : struct
     {
         private readonly ConcurrentDictionary<LabelSet, BoundMeasureMetricSdkBase<T>> measureBoundInstruments = new ConcurrentDictionary<LabelSet, BoundMeasureMetricSdkBase<T>>();
+        private readonly Func<LabelSet, BoundMeasureMetricSdkBase<T>> createMetricFunc;
         private string metricName;
 
         public MeasureMetricSdk(string name)
         {
             this.metricName = name;
+            this.createMetricFunc = (_) => this.CreateMetric();
         }
 
         public override BoundMeasureMetric<T> Bind(LabelSet labelset)
         {
-            return this.measureBoundInstruments.GetOrAdd(labelset, this.CreateMetric());
+            return this.measureBoundInstruments.GetOrAdd(labelset, this.createMetricFunc);
         }
 
         public override BoundMeasureMetric<T> Bind(IEnumerable<KeyValuePair<string, string>> labels)

src/OpenTelemetry/Metrics/MeterSdk.cs

@@ -24,6 +24,11 @@ namespace OpenTelemetry.Metrics
 {
     internal class MeterSdk : Meter
     {
+        private static readonly Func<string, Int64CounterMetricSdk> NewInt64CounterMetricSdkFunc = (name) => new Int64CounterMetricSdk(name);
+        private static readonly Func<string, DoubleCounterMetricSdk> NewDoubleCounterMetricSdkFunc = (name) => new DoubleCounterMetricSdk(name);
+        private static readonly Func<string, Int64MeasureMetricSdk> NewInt64MeasureMetricSdkFunc = (name) => new Int64MeasureMetricSdk(name);
+        private static readonly Func<string, DoubleMeasureMetricSdk> NewDoubleMeasureMetricSdkFunc = (name) => new DoubleMeasureMetricSdk(name);
+
         private readonly string meterName;
         private readonly MetricProcessor metricProcessor;
         private readonly ConcurrentDictionary<string, Int64CounterMetricSdk> longCounters = new ConcurrentDictionary<string, Int64CounterMetricSdk>();
@@ -262,34 +267,46 @@ namespace OpenTelemetry.Metrics
 
         public override CounterMetric<long> CreateInt64Counter(string name, bool monotonic = true)
         {
-            return this.longCounters.GetOrAdd(name, new Int64CounterMetricSdk(name));
+            return this.longCounters.GetOrAdd(name, NewInt64CounterMetricSdkFunc);
         }
 
         public override CounterMetric<double> CreateDoubleCounter(string name, bool monotonic = true)
         {
-            return this.doubleCounters.GetOrAdd(name, new DoubleCounterMetricSdk(name));
+            return this.doubleCounters.GetOrAdd(name, NewDoubleCounterMetricSdkFunc);
         }
 
         public override MeasureMetric<double> CreateDoubleMeasure(string name, bool absolute = true)
         {
-            return this.doubleMeasures.GetOrAdd(name, new DoubleMeasureMetricSdk(name));
+            return this.doubleMeasures.GetOrAdd(name, NewDoubleMeasureMetricSdkFunc);
         }
 
         public override MeasureMetric<long> CreateInt64Measure(string name, bool absolute = true)
         {
-            return this.longMeasures.GetOrAdd(name, new Int64MeasureMetricSdk(name));
+            return this.longMeasures.GetOrAdd(name, NewInt64MeasureMetricSdkFunc);
         }
 
         /// <inheritdoc/>
         public override Int64ObserverMetric CreateInt64Observer(string name, Action<Int64ObserverMetric> callback, bool absolute = true)
         {
-            return this.longObservers.GetOrAdd(name, new Int64ObserverMetricSdk(name, callback));
+            Int64ObserverMetricSdk metric;
+            if (!this.longObservers.TryGetValue(name, out metric))
+            {
+                metric = this.longObservers.GetOrAdd(name, new Int64ObserverMetricSdk(name, callback));
+            }
+
+            return metric;
         }
 
         /// <inheritdoc/>
         public override DoubleObserverMetric CreateDoubleObserver(string name, Action<DoubleObserverMetric> callback, bool absolute = true)
         {
-            return this.doubleObservers.GetOrAdd(name, new DoubleObserverMetricSdk(name, callback));
+            DoubleObserverMetricSdk metric;
+            if (!this.doubleObservers.TryGetValue(name, out metric))
+            {
+                metric = this.doubleObservers.GetOrAdd(name, new DoubleObserverMetricSdk(name, callback));
+            }
+
+            return metric;
         }
     }
 }

test/OpenTelemetry.Tests/Metrics/MetricsTest.cs

@@ -26,7 +26,7 @@ namespace OpenTelemetry.Metrics.Tests
     public class MetricsTest
     {
         [Fact]
-        public void CounterSendsAggregateToRegisteredProcessor()
+        public void LongCounterSendsAggregateToRegisteredProcessor()
         {
             var testProcessor = new TestMetricProcessor();
             var meter = Sdk.CreateMeterProviderBuilder()
@@ -79,7 +79,63 @@ namespace OpenTelemetry.Metrics.Tests
         }
 
         [Fact]
-        public void MeasureSendsAggregateToRegisteredProcessor()
+        public void DoubleCounterSendsAggregateToRegisteredProcessor()
+        {
+            var testProcessor = new TestMetricProcessor();
+            var meter = Sdk.CreateMeterProviderBuilder()
+                .SetProcessor(testProcessor)
+                .Build()
+                .GetMeter("library1") as MeterSdk;
+
+            var testCounter = meter.CreateDoubleCounter("testCounter");
+
+            var labels1 = new List<KeyValuePair<string, string>>();
+            labels1.Add(new KeyValuePair<string, string>("dim1", "value1"));
+
+            var labels2 = new List<KeyValuePair<string, string>>();
+            labels2.Add(new KeyValuePair<string, string>("dim1", "value2"));
+
+            var labels3 = new List<KeyValuePair<string, string>>();
+            labels3.Add(new KeyValuePair<string, string>("dim1", "value3"));
+
+            var context = default(SpanContext);
+            testCounter.Add(context, 100.2, meter.GetLabelSet(labels1));
+            testCounter.Add(context, 10.2, meter.GetLabelSet(labels1));
+
+            var boundCounterLabel2 = testCounter.Bind(labels2);
+            boundCounterLabel2.Add(context, 200.2);
+
+            testCounter.Add(context, 200.2, meter.GetLabelSet(labels3));
+            testCounter.Add(context, 10.2, meter.GetLabelSet(labels3));
+
+            meter.Collect();
+
+            Assert.Single(testProcessor.Metrics);
+            var metric = testProcessor.Metrics[0];
+
+            Assert.Equal("testCounter", metric.MetricName);
+            Assert.Equal("library1", metric.MetricNamespace);
+
+            // 3 time series, as 3 unique label sets.
+            Assert.Equal(3, metric.Data.Count);
+            var expectedSum = 100.2 + 10.2;
+            var metricSeries = metric.Data.Single(data => data.Labels.Any(l => l.Key == "dim1" && l.Value == "value1"));
+            var metricDouble = metricSeries as DoubleSumData;
+            Assert.Equal(expectedSum, metricDouble.Sum);
+
+            expectedSum = 200.2;
+            metricSeries = metric.Data.Single(data => data.Labels.Any(l => l.Key == "dim1" && l.Value == "value2"));
+            metricDouble = metricSeries as DoubleSumData;
+            Assert.Equal(expectedSum, metricDouble.Sum);
+
+            expectedSum = 200.2 + 10.2;
+            metricSeries = metric.Data.Single(data => data.Labels.Any(l => l.Key == "dim1" && l.Value == "value3"));
+            metricDouble = metricSeries as DoubleSumData;
+            Assert.Equal(expectedSum, metricDouble.Sum);
+        }
+
+        [Fact]
+        public void LongMeasureSendsAggregateToRegisteredProcessor()
         {
             var testProcessor = new TestMetricProcessor();
             var meter = Sdk.CreateMeterProviderBuilder()
@@ -126,6 +182,56 @@ namespace OpenTelemetry.Metrics.Tests
             Assert.Equal(200, metricSummary.Max);
         }
 
+        [Fact]
+        public void DoubleMeasureSendsAggregateToRegisteredProcessor()
+        {
+            var testProcessor = new TestMetricProcessor();
+            var meter = Sdk.CreateMeterProviderBuilder()
+                .SetProcessor(testProcessor)
+                .Build()
+                .GetMeter("library1") as MeterSdk;
+            var testMeasure = meter.CreateDoubleMeasure("testMeasure");
+
+            var labels1 = new List<KeyValuePair<string, string>>();
+            labels1.Add(new KeyValuePair<string, string>("dim1", "value1"));
+
+            var labels2 = new List<KeyValuePair<string, string>>();
+            labels2.Add(new KeyValuePair<string, string>("dim1", "value2"));
+
+            var context = default(SpanContext);
+            testMeasure.Record(context, 100.2, meter.GetLabelSet(labels1));
+            testMeasure.Record(context, 10.2, meter.GetLabelSet(labels1));
+            testMeasure.Record(context, 1.2, meter.GetLabelSet(labels1));
+            testMeasure.Record(context, 200.2, meter.GetLabelSet(labels2));
+            testMeasure.Record(context, 20.2, meter.GetLabelSet(labels2));
+
+            meter.Collect();
+
+            Assert.Single(testProcessor.Metrics);
+            var metric = testProcessor.Metrics[0];
+            Assert.Equal("testMeasure", metric.MetricName);
+            Assert.Equal("library1", metric.MetricNamespace);
+
+            // 2 time series, as 2 unique label sets.
+            Assert.Equal(2, metric.Data.Count);
+
+            var expectedSum = 100.2 + 10.2 + 1.2;
+            var metricSeries = metric.Data.Single(data => data.Labels.Any(l => l.Key == "dim1" && l.Value == "value1"));
+            var metricSummary = metricSeries as DoubleSummaryData;
+            Assert.Equal(expectedSum, metricSummary.Sum);
+            Assert.Equal(3, metricSummary.Count);
+            Assert.Equal(1.2, metricSummary.Min);
+            Assert.Equal(100.2, metricSummary.Max);
+
+            expectedSum = 200.2 + 20.2;
+            metricSeries = metric.Data.Single(data => data.Labels.Any(l => l.Key == "dim1" && l.Value == "value2"));
+            metricSummary = metricSeries as DoubleSummaryData;
+            Assert.Equal(expectedSum, metricSummary.Sum);
+            Assert.Equal(2, metricSummary.Count);
+            Assert.Equal(20.2, metricSummary.Min);
+            Assert.Equal(200.2, metricSummary.Max);
+        }
+
         [Fact]
         public void LongObserverSendsAggregateToRegisteredProcessor()
         {