[OpenTelemetry] Fix metric storage leak

src/OpenTelemetry/CHANGELOG.md

@@ -24,6 +24,10 @@ Notes](../../RELEASENOTES.md).
   recorded.
   ([#7427](https://github.com/open-telemetry/opentelemetry-dotnet/pull/7427))
 
+* 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

@@ -16,6 +16,8 @@ public abstract partial class MetricReader
     private readonly HashSet<string> 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.");
                 }
@@ -254,7 +257,7 @@ private Batch<Metric> GetMetricsBatch()
 
                     if (!metric.Active)
                     {
-                        this.RemoveMetric(ref metric);
+                        this.RemoveMetric(ref metric, i);
                     }
                 }
             }
@@ -268,26 +271,57 @@ 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
 
         // 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.Tests/Metrics/MeterProviderSdkTests.cs

@@ -40,7 +40,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 +73,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 +87,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 +117,46 @@ 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);
+    }
 }