Code review.

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs

@@ -353,8 +353,8 @@ public void MetricInstrumentDeactivated(string instrumentName, string meterName)
         this.WriteEvent(52, instrumentName, meterName);
     }
 
-    [Event(53, Message = "A previously deactivated Instrument '{0}', Meter '{1}' has been reactivated.", Level = EventLevel.Informational)]
-    public void MetricInstrumentReactivated(string instrumentName, string meterName)
+    [Event(53, Message = "Instrument '{0}', Meter '{1}' has been removed.", Level = EventLevel.Informational)]
+    public void MetricInstrumentRemoved(string instrumentName, string meterName)
     {
         this.WriteEvent(53, instrumentName, meterName);
     }

src/OpenTelemetry/Metrics/Metric.cs

@@ -27,10 +27,6 @@ public sealed class Metric
 
     internal const int DefaultExponentialHistogramMaxScale = 20;
 
-    internal const int MetricStatusActive = 0;
-    internal const int MetricStatusDeactivating = 1;
-    internal const int MetricStatusInactive = 2;
-
     internal static readonly double[] DefaultHistogramBounds = new double[] { 0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000 };
 
     // Short default histogram bounds. Based on the recommended semantic convention values for http.server.request.duration.
@@ -57,7 +53,6 @@ public sealed class Metric
         ("System.Net.Http", "http.client.connection.duration"),
     };
 
-    internal volatile int Status;
     private readonly AggregatorStore aggStore;
 
     internal Metric(
@@ -174,7 +169,6 @@ internal Metric(
 
         this.aggStore = new AggregatorStore(instrumentIdentity, aggType, temporality, maxMetricPointsPerMetricStream, emitOverflowAttribute, shouldReclaimUnusedMetricPoints, exemplarFilter);
         this.Temporality = temporality;
-        this.Status = MetricStatusActive;
     }
 
     /// <summary>
@@ -217,7 +211,7 @@ internal Metric(
     /// </summary>
     internal MetricStreamIdentity InstrumentIdentity { get; private set; }
 
-    internal object StatusLock => this.aggStore;
+    internal bool Active { get; set; } = true;
 
     /// <summary>
     /// Get the metric points for the metric stream.

src/OpenTelemetry/Metrics/MetricReaderExt.cs

@@ -16,6 +16,7 @@
 
 using System.Collections.Concurrent;
 using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.Diagnostics.Metrics;
 using OpenTelemetry.Internal;
 
@@ -50,10 +51,8 @@ internal AggregationTemporality GetAggregationTemporality(Type instrumentType)
         var metricStreamIdentity = new MetricStreamIdentity(instrument, metricStreamConfiguration: null);
         lock (this.instrumentCreationLock)
         {
-            if (this.instrumentIdentityToMetric.TryGetValue(metricStreamIdentity, out var existingMetric))
+            if (this.TryGetExistingMetric(in metricStreamIdentity, out var existingMetric))
             {
-                ActivateMetricIfRequired(existingMetric);
-
                 return existingMetric;
             }
 
@@ -130,10 +129,8 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric
                     continue;
                 }
 
-                if (this.instrumentIdentityToMetric.TryGetValue(metricStreamIdentity, out var existingMetric))
+                if (this.TryGetExistingMetric(in metricStreamIdentity, out var existingMetric))
                 {
-                    ActivateMetricIfRequired(existingMetric);
-
                     metrics.Add(existingMetric);
                     continue;
                 }
@@ -243,38 +240,38 @@ internal void SetMaxMetricPointsPerMetricStream(int maxMetricPointsPerMetricStre
 
     private static void DeactivateMetric(Metric metric)
     {
-        metric.Status = Metric.MetricStatusDeactivating;
-
-        // TODO: Should we set end time here?
-
-        OpenTelemetrySdkEventSource.Log.MetricInstrumentDeactivated(
-            metric.Name,
-            metric.MeterName);
+        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?
+            metric.Active = false;
+
+            OpenTelemetrySdkEventSource.Log.MetricInstrumentDeactivated(
+                metric.Name,
+                metric.MeterName);
+        }
     }
 
-    private static void ActivateMetricIfRequired(Metric metric)
+    private bool TryGetExistingMetric(in MetricStreamIdentity metricStreamIdentity, [NotNullWhen(true)] out Metric? existingMetric)
     {
-        if (metric.Status != Metric.MetricStatusActive)
+        if (!this.instrumentIdentityToMetric.TryGetValue(metricStreamIdentity, out existingMetric)
+            || !existingMetric.Active)
         {
-            lock (metric.StatusLock)
-            {
-                metric.Status = Metric.MetricStatusActive;
-            }
-
-            // TODO: Should we set start time here? Should we reset the data
-            // points? What if we are activating before an export/collect has
-            // had a chance to export the previously collected data?
-
-            OpenTelemetrySdkEventSource.Log.MetricInstrumentReactivated(
-                metric.Name,
-                metric.MeterName);
+            return false;
         }
+
+        return true;
     }
 
     private void CreateOrUpdateMetricStreamRegistration(in MetricStreamIdentity metricStreamIdentity)
     {
         if (!this.metricStreamNames.Add(metricStreamIdentity.MetricStreamName))
         {
+            // TODO: If a metric is deactivated and then reactivated we log the
+            // same warning as if it was a duplicate.
             OpenTelemetrySdkEventSource.Log.DuplicateMetricInstrument(
                 metricStreamIdentity.InstrumentName,
                 metricStreamIdentity.MeterName,
@@ -295,26 +292,20 @@ private Batch<Metric> GetMetricsBatch()
             int metricCountCurrentBatch = 0;
             for (int i = 0; i < target; i++)
             {
-                var metric = this.metrics![i];
+                ref var metric = ref this.metrics![i];
                 if (metric != null)
                 {
-                    var metricStatus = metric.Status;
-                    if (metricStatus != Metric.MetricStatusActive)
-                    {
-                        if (metricStatus == Metric.MetricStatusInactive)
-                        {
-                            continue;
-                        }
-
-                        this.CompleteMetricDeactivation(metric);
-                    }
-
                     int metricPointSize = metric.Snapshot();
 
                     if (metricPointSize > 0)
                     {
                         this.metricsCurrentBatch![metricCountCurrentBatch++] = metric;
                     }
+
+                    if (!metric.Active)
+                    {
+                        this.RemoveMetric(ref metric);
+                    }
                 }
             }
 
@@ -327,18 +318,24 @@ private Batch<Metric> GetMetricsBatch()
         }
     }
 
-    private void CompleteMetricDeactivation(Metric metric)
+    private void RemoveMetric(ref Metric? metric)
     {
-        lock (metric.StatusLock)
-        {
-            if (metric.Status != Metric.MetricStatusDeactivating)
-            {
-                // Note: If we fall here it means the metric was reactivated
-                // while we were waiting on the lock.
-                return;
-            }
+        Debug.Assert(metric != null, "metric was null");
 
-            metric.Status = Metric.MetricStatusInactive;
-        }
+        // 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);
+
+        var result = this.instrumentIdentityToMetric.TryRemove(metric.InstrumentIdentity, out var _);
+        Debug.Assert(result, "result was false");
+
+        // Note: metric is a reference to the array storage so
+        // this clears the metric out of the array.
+        metric = null;
     }
 }

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

@@ -149,21 +149,20 @@ public void ReloadOfMetricsViaIConfigurationWithExportCleanupTest(bool useWithMe
 
         meterProvider.ForceFlush();
 
-        AssertSingleMetricWithLongSum(
-            exportedItems,
-            expectedValue: temporalityPreference == MetricReaderTemporalityPreference.Delta ? 1 : 2);
+        AssertSingleMetricWithLongSum(exportedItems);
 
         var duplicateMetricInstrumentEvents = inMemoryEventListener.Events.Where((e) => e.EventId == 38);
 
-        Assert.Empty(duplicateMetricInstrumentEvents);
+        // Note: We currently log a duplicate warning anytime a metric is reactivated.
+        Assert.Single(duplicateMetricInstrumentEvents);
 
         var metricInstrumentDeactivatedEvents = inMemoryEventListener.Events.Where((e) => e.EventId == 52);
 
         Assert.Single(metricInstrumentDeactivatedEvents);
 
-        var metricInstrumentReactivatedEvents = inMemoryEventListener.Events.Where((e) => e.EventId == 53);
+        var metricInstrumentRemovedEvents = inMemoryEventListener.Events.Where((e) => e.EventId == 53);
 
-        Assert.Single(metricInstrumentReactivatedEvents);
+        Assert.Single(metricInstrumentRemovedEvents);
     }
 
     [Theory]
@@ -205,27 +204,49 @@ public void ReloadOfMetricsViaIConfigurationWithoutExportCleanupTest(bool useWit
 
         meterProvider.ForceFlush();
 
-        AssertSingleMetricWithLongSum(exportedItems, expectedValue: 2);
+        // Note: We end up with 2 of the same metric being exported. This is
+        // because the current behavior when something is deactivated is to
+        // remove the metric. The next publish creates a new metric.
+        Assert.Equal(2, exportedItems.Count);
+
+        AssertMetricWithLongSum(exportedItems[0]);
+        AssertMetricWithLongSum(exportedItems[1]);
+
+        exportedItems.Clear();
+
+        counter.Add(1);
+
+        meterProvider.ForceFlush();
+
+        AssertSingleMetricWithLongSum(
+            exportedItems,
+            expectedValue: temporalityPreference == MetricReaderTemporalityPreference.Delta ? 1 : 2);
 
         var duplicateMetricInstrumentEvents = inMemoryEventListener.Events.Where((e) => e.EventId == 38);
 
-        Assert.Empty(duplicateMetricInstrumentEvents);
+        // Note: We currently log a duplicate warning anytime a metric is reactivated.
+        Assert.Single(duplicateMetricInstrumentEvents);
 
         var metricInstrumentDeactivatedEvents = inMemoryEventListener.Events.Where((e) => e.EventId == 52);
 
         Assert.Single(metricInstrumentDeactivatedEvents);
 
-        var metricInstrumentReactivatedEvents = inMemoryEventListener.Events.Where((e) => e.EventId == 53);
+        var metricInstrumentRemovedEvents = inMemoryEventListener.Events.Where((e) => e.EventId == 53);
 
-        Assert.Single(metricInstrumentReactivatedEvents);
+        Assert.Single(metricInstrumentRemovedEvents);
     }
 
     private static void AssertSingleMetricWithLongSum(List<Metric> exportedItems, long expectedValue = 1)
     {
         Assert.Single(exportedItems);
 
+        AssertMetricWithLongSum(exportedItems[0], expectedValue);
+    }
+
+    private static void AssertMetricWithLongSum(Metric metric, long expectedValue = 1)
+    {
         List<MetricPoint> metricPoints = new();
-        foreach (ref readonly var mp in exportedItems[0].GetMetricPoints())
+        foreach (ref readonly var mp in metric.GetMetricPoints())
         {
             metricPoints.Add(mp);
         }

test/OpenTelemetry.Tests/Metrics/MeterProviderSdkTest.cs

@@ -15,6 +15,7 @@
 // </copyright>
 
 using System.Diagnostics.Metrics;
+using OpenTelemetry.Internal;
 using OpenTelemetry.Tests;
 using Xunit;
 
@@ -49,10 +50,14 @@ public void BuilderTypeDoesNotChangeTest()
     }
 
     [Theory]
-    [InlineData(false)]
-    [InlineData(true)]
-    public void TransientMeterReusesMetricTest(bool withView)
+    [InlineData(false, true)]
+    [InlineData(true, true)]
+    [InlineData(false, false)]
+    [InlineData(true, false)]
+    public void TransientMeterExhaustsMetricStorageTest(bool withView, bool forceFlushAfterEachTest)
     {
+        using var inMemoryEventListener = new InMemoryEventListener(OpenTelemetrySdkEventSource.Log);
+
         var meterName = Utils.GetCurrentMethodName();
         var exportedItems = new List<Metric>();
 
@@ -72,8 +77,29 @@ public void TransientMeterReusesMetricTest(bool withView)
         Assert.NotNull(meterProvider);
 
         RunTest();
+
+        if (forceFlushAfterEachTest)
+        {
+            Assert.Single(exportedItems);
+        }
+
         RunTest();
 
+        if (forceFlushAfterEachTest)
+        {
+            Assert.Empty(exportedItems);
+        }
+        else
+        {
+            meterProvider.ForceFlush();
+
+            Assert.Single(exportedItems);
+        }
+
+        var metricInstrumentIgnoredEvents = inMemoryEventListener.Events.Where((e) => e.EventId == 33);
+
+        Assert.Single(metricInstrumentIgnoredEvents);
+
         void RunTest()
         {
             exportedItems.Clear();
@@ -85,9 +111,10 @@ void RunTest()
 
             meter.Dispose();
 
-            meterProvider.ForceFlush();
-
-            Assert.Single(exportedItems);
+            if (forceFlushAfterEachTest)
+            {
+                meterProvider.ForceFlush();
+            }
         }
     }
 }