[OpenTelemetry] Fix metric storage leak (#7466)

Author: martincostello

src/OpenTelemetry/CHANGELOG.md

@@ -27,6 +27,10 @@ Notes](../../RELEASENOTES.md).
 * Added support for a Schema URL on `Resource` instances.
   ([#7472](https://github.com/open-telemetry/opentelemetry-dotnet/pull/7472))
 
+* Fixed a metric storage leak that occurred when meters and instruments were
+  repeatedly created and disposed.
+  ([#7466](https://github.com/open-telemetry/opentelemetry-dotnet/pull/7466))
+
 ## 1.16.0
 
 Released 2026-Jun-10

src/OpenTelemetry/Metrics/Reader/MetricReaderExt.cs

@@ -13,9 +13,11 @@ namespace OpenTelemetry.Metrics;
 /// </summary>
 public abstract partial class MetricReader
 {
-    private readonly HashSet<string> metricStreamNames = new(StringComparer.OrdinalIgnoreCase);
+    private readonly Dictionary<string, int> metricStreamNames = new(StringComparer.OrdinalIgnoreCase);
     private readonly ConcurrentDictionary<MetricStreamIdentity, Metric?> instrumentIdentityToMetric = new();
     private readonly Lock instrumentCreationLock = new();
+    private readonly ConcurrentQueue<int> availableMetricIndices = new();
+
     private int metricLimit;
     private int cardinalityLimit;
     private Metric?[] metrics = [];
@@ -28,11 +30,11 @@ internal static void DeactivateMetric(Metric metric)
     {
         if (metric.Active)
         {
-            // TODO: This will cause the metric to be removed from the storage
-            // array during the next collect/export. If this happens often we
-            // will run out of storage. Would it be better instead to set the
-            // end time on the metric and keep it around so it can be
-            // reactivated?
+            // Note: This causes the metric to be removed from the storage array
+            // during the next collect/export. The freed storage slot is then
+            // reused for a subsequently published instrument (see
+            // TryReserveMetricIndex), so repeatedly creating and disposing
+            // meters/instruments does not exhaust the metric storage.
             metric.Active = false;
 
             OpenTelemetrySdkEventSource.Log.MetricInstrumentDeactivated(
@@ -67,8 +69,7 @@ internal virtual List<Metric> AddMetricWithNoViews(Instrument instrument)
                 return [existingMetric];
             }
 
-            var index = ++this.metricIndex;
-            if (index >= this.metricLimit)
+            if (!this.TryReserveMetricIndex(out var index))
             {
                 OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricStreamIdentity.InstrumentName, metricStreamIdentity.MeterName, "Maximum allowed Metric streams for the provider exceeded.", "Use MeterProviderBuilder.AddView to drop unused instruments. Or use MeterProviderBuilder.SetMaxMetricStreams to configure MeterProvider to allow higher limit.");
                 return [];
@@ -86,6 +87,9 @@ internal virtual List<Metric> AddMetricWithNoViews(Instrument instrument)
                 }
                 catch (NotSupportedException nse)
                 {
+                    // The reserved slot was never populated, so return it for reuse.
+                    this.availableMetricIndices.Enqueue(index);
+
                     // TODO: This allocates string even if none listening.
                     // Could be improved with separate Event.
                     // Also the message could call out what Instruments
@@ -158,8 +162,7 @@ internal virtual List<Metric> AddMetricWithViews(Instrument instrument, List<Met
                     continue;
                 }
 
-                var index = ++this.metricIndex;
-                if (index >= this.metricLimit)
+                if (!this.TryReserveMetricIndex(out var index))
                 {
                     OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricStreamIdentity.InstrumentName, metricStreamIdentity.MeterName, "Maximum allowed Metric streams for the provider exceeded.", "Use MeterProviderBuilder.AddView to drop unused instruments. Or use MeterProviderBuilder.SetMaxMetricStreams to configure MeterProvider to allow higher limit.");
                 }
@@ -221,15 +224,25 @@ private bool TryGetExistingMetric(in MetricStreamIdentity metricStreamIdentity,
 
     private void CreateOrUpdateMetricStreamRegistration(in MetricStreamIdentity metricStreamIdentity)
     {
-        if (!this.metricStreamNames.Add(metricStreamIdentity.MetricStreamName))
+        if (this.metricStreamNames.TryGetValue(metricStreamIdentity.MetricStreamName, out var count))
         {
-            // TODO: If a metric is deactivated and then reactivated we log the
-            // same warning as if it was a duplicate.
+            // TODO: This can be a false positive. The registration is only
+            // released when the metric is removed during a collection cycle.
+            // If an instrument is deactivated and an instrument with the same
+            // stream name is re-created before the next collect, the stale
+            // registration is still present and this logs a duplicate/conflict
+            // warning even though there is no actual conflict.
             OpenTelemetrySdkEventSource.Log.DuplicateMetricInstrument(
                 metricStreamIdentity.InstrumentName,
                 metricStreamIdentity.MeterName,
                 "Metric instrument has the same name as an existing one but differs by description, unit, or instrument type. Measurements from this instrument will still be exported but may result in conflicts.",
                 "Either change the name of the instrument or use MeterProviderBuilder.AddView to resolve the conflict.");
+
+            this.metricStreamNames[metricStreamIdentity.MetricStreamName] = count + 1;
+        }
+        else
+        {
+            this.metricStreamNames[metricStreamIdentity.MetricStreamName] = 1;
         }
     }
 
@@ -254,7 +267,7 @@ private Batch<Metric> GetMetricsBatch()
 
                     if (!metric.Active)
                     {
-                        this.RemoveMetric(ref metric);
+                        this.RemoveMetric(ref metric, i);
                     }
                 }
             }
@@ -268,26 +281,77 @@ private Batch<Metric> GetMetricsBatch()
         }
     }
 
-    private void RemoveMetric(ref Metric? metric)
+    private void RemoveMetric(ref Metric? metric, int index)
     {
-        // TODO: This logic removes the metric. If the same
-        // metric is published again we will create a new metric
-        // for it. If this happens often we will run out of
-        // storage. Instead, should we keep the metric around
-        // and set a new start time + reset its data if it comes
-        // back?
-
         OpenTelemetrySdkEventSource.Log.MetricInstrumentRemoved(metric!.Name, metric.MeterName);
 
-        // Note: This is using TryUpdate and NOT TryRemove because there is a
-        // race condition. If a metric is deactivated and then reactivated in
-        // the same collection cycle
-        // instrumentIdentityToMetric[metric.InstrumentIdentity] may already
-        // point to the new activated metric and not the old deactivated one.
-        this.instrumentIdentityToMetric.TryUpdate(metric.InstrumentIdentity, null, metric);
+        // Note: This removes the entry (key and value) so the dictionary does
+        // not grow without bound as instruments are repeatedly created and
+        // disposed. A value-matching removal is used to guard against a race
+        // condition: if a metric is deactivated and then reactivated in the
+        // same collection cycle, instrumentIdentityToMetric[metric.InstrumentIdentity]
+        // may already point to the new (active) metric. In that case the entry's
+        // value no longer equals the old metric, the removal is a no-op, and the
+        // new metric is retained.
+        var item = new KeyValuePair<MetricStreamIdentity, Metric?>(metric.InstrumentIdentity, metric);
+#if NET
+        this.instrumentIdentityToMetric.TryRemove(item);
+#else
+        ICollection<KeyValuePair<MetricStreamIdentity, Metric?>> instrumentIdentityToMetric = this.instrumentIdentityToMetric;
+        instrumentIdentityToMetric.Remove(item);
+#endif
+
+        // Release the stream-name registration for this metric. When the last
+        // metric using the name is removed the entry is dropped so the
+        // dictionary does not grow without bound as instruments are repeatedly
+        // created and disposed with recycled storage slots.
+        var metricStreamName = metric.InstrumentIdentity.MetricStreamName;
+        lock (this.instrumentCreationLock)
+        {
+            if (this.metricStreamNames.TryGetValue(metricStreamName, out var count))
+            {
+                if (count <= 1)
+                {
+                    this.metricStreamNames.Remove(metricStreamName);
+                }
+                else
+                {
+                    this.metricStreamNames[metricStreamName] = count - 1;
+                }
+            }
+        }
 
         // Note: metric is a reference to the array storage so
         // this clears the metric out of the array.
         metric = null;
+
+        // Make the freed slot available for reuse by a future instrument. This
+        // must happen after the slot has been cleared above so that a concurrent
+        // AddMetricWith*Views call observes a null slot before writing to it.
+        this.availableMetricIndices.Enqueue(index);
+    }
+
+    private bool TryReserveMetricIndex(out int index)
+    {
+        // Note: This is always called while holding instrumentCreationLock, so
+        // access to metricIndex does not need additional synchronization.
+
+        // Reuse a slot freed by a previously removed (deactivated) metric, if
+        // one is available, before consuming a brand new slot.
+        if (this.availableMetricIndices.TryDequeue(out index))
+        {
+            return true;
+        }
+
+        var newIndex = this.metricIndex + 1;
+        if (newIndex >= this.metricLimit)
+        {
+            index = -1;
+            return false;
+        }
+
+        this.metricIndex = newIndex;
+        index = newIndex;
+        return true;
     }
 }

test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryMetricsBuilderExtensionsTests.cs

@@ -235,11 +235,6 @@ public void ReloadOfMetricsViaIConfigurationWithExportCleanupTest(bool useWithMe
 
         AssertSingleMetricWithLongSum(exportedItems);
 
-        var duplicateMetricInstrumentEvents = eventListener.Messages.Where((e) => e.EventId == 38);
-
-        // Note: We currently log a duplicate warning anytime a metric is reactivated.
-        Assert.Single(duplicateMetricInstrumentEvents);
-
         var metricInstrumentDeactivatedEvents = eventListener.Messages.Where((e) => e.EventId == 52);
 
         Assert.Single(metricInstrumentDeactivatedEvents);

test/OpenTelemetry.Tests/Metrics/MeterProviderSdkTests.cs

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 using System.Diagnostics.Metrics;
+using System.Reflection;
 using OpenTelemetry.Internal;
 using OpenTelemetry.Tests;
 
@@ -40,7 +41,7 @@ public void BuilderTypeDoesNotChangeTest()
     [InlineData(true, true)]
     [InlineData(false, false)]
     [InlineData(true, false)]
-    public void TransientMeterExhaustsMetricStorageTest(bool withView, bool forceFlushAfterEachTest)
+    public void TransientMeterMetricStorageIsReclaimedOnCollectTest(bool withView, bool forceFlushAfterEachTest)
     {
         using var eventListener = new TestEventListener(OpenTelemetrySdkEventSource.Log);
 
@@ -73,7 +74,10 @@ public void TransientMeterExhaustsMetricStorageTest(bool withView, bool forceFlu
 
         if (forceFlushAfterEachTest)
         {
-            Assert.Empty(exportedItems);
+            // A collection happened after the first meter was disposed, so its
+            // storage slot was reclaimed and reused by the second meter's
+            // instrument, which is therefore exported rather than dropped.
+            Assert.Single(exportedItems);
         }
         else
         {
@@ -84,7 +88,18 @@ public void TransientMeterExhaustsMetricStorageTest(bool withView, bool forceFlu
 
         var metricInstrumentIgnoredEvents = eventListener.Messages.Where((e) => e.EventId == 33 && (e.Payload?.Count ?? 0) >= 2 && (e.Payload![1] as string) == meterName);
 
-        Assert.Single(metricInstrumentIgnoredEvents);
+        if (forceFlushAfterEachTest)
+        {
+            // Storage was reclaimed between the two meters, so no instrument was dropped.
+            Assert.Empty(metricInstrumentIgnoredEvents);
+        }
+        else
+        {
+            // No collection happened between the two meters, so the first
+            // metric's storage had not yet been reclaimed and the second
+            // instrument was dropped because the stream limit was reached.
+            Assert.Single(metricInstrumentIgnoredEvents);
+        }
 
         void RunTest()
         {
@@ -103,4 +118,98 @@ void RunTest()
             }
         }
     }
+
+    [Fact]
+    public void TransientMeterMetricStorageIsReusedAcrossManyCollectsTest()
+    {
+        using var eventListener = new TestEventListener(OpenTelemetrySdkEventSource.Log);
+
+        var meterName = Utils.GetCurrentMethodName();
+        var exportedItems = new List<Metric>();
+
+        const int MaxMetricStreams = 2;
+
+        using var meterProvider = Sdk.CreateMeterProviderBuilder()
+            .SetMaxMetricStreams(MaxMetricStreams)
+            .AddMeter(meterName)
+            .AddInMemoryExporter(exportedItems)
+            .Build() as MeterProviderSdk;
+
+        Assert.NotNull(meterProvider);
+
+        // Create and dispose many more instruments than the metric stream limit,
+        // collecting after each one so the deactivated metric's slot is reclaimed.
+        for (var i = 0; i < MaxMetricStreams * 10; i++)
+        {
+            exportedItems.Clear();
+
+            using (var meter = new Meter(meterName))
+            {
+                var counter = meter.CreateCounter<int>("Counter");
+                counter.Add(1);
+            }
+
+            meterProvider.ForceFlush();
+
+            Assert.Single(exportedItems);
+            Assert.Equal("Counter", exportedItems[0].Name);
+        }
+
+        // No instrument should ever have been dropped due to exhausted storage.
+        var metricInstrumentIgnoredEvents = eventListener.Messages.Where((e) => e.EventId == 33 && (e.Payload?.Count ?? 0) >= 2 && (e.Payload![1] as string) == meterName);
+
+        Assert.Empty(metricInstrumentIgnoredEvents);
+    }
+
+    [Fact]
+    public void TransientMetricStreamNameRegistrationsAreReleasedTest()
+    {
+        var meterName = Utils.GetCurrentMethodName();
+        var exportedItems = new List<Metric>();
+
+        const int MaxMetricStreams = 2;
+        const int Iterations = MaxMetricStreams * 10;
+
+        using var meterProvider = Sdk.CreateMeterProviderBuilder()
+            .SetMaxMetricStreams(MaxMetricStreams)
+            .AddMeter(meterName)
+            .AddInMemoryExporter(exportedItems)
+            .Build() as MeterProviderSdk;
+
+        Assert.NotNull(meterProvider);
+
+        // Create and dispose many distinctly-named instruments than the metric
+        // stream limit, collecting after each one so the deactivated metric's
+        // slot (and its stream-name registration) is reclaimed. Distinct names
+        // are important: a leak in the stream-name bookkeeping would only show
+        // up when the names differ between iterations.
+        for (var i = 0; i < Iterations; i++)
+        {
+            exportedItems.Clear();
+
+            using (var meter = new Meter(meterName))
+            {
+                var counter = meter.CreateCounter<int>($"Counter{i}");
+                counter.Add(1);
+            }
+
+            meterProvider.ForceFlush();
+
+            Assert.Single(exportedItems);
+            Assert.Equal($"Counter{i}", exportedItems[0].Name);
+        }
+
+        // The stream-name registrations must be released as metrics are removed,
+        // otherwise the reader retains an entry for every distinct name ever seen.
+        var reader = meterProvider.Reader;
+        Assert.NotNull(reader);
+
+        var metricStreamNamesField = typeof(MetricReader).GetField("metricStreamNames", BindingFlags.Instance | BindingFlags.NonPublic);
+        Assert.NotNull(metricStreamNamesField);
+
+        var metricStreamNames = Assert.IsType<System.Collections.ICollection>(metricStreamNamesField.GetValue(reader), exactMatch: false);
+
+        // Every instrument was disposed and collected, so no registration should remain.
+        Assert.Empty(metricStreamNames);
+    }
 }