Make MetricPoint reclaim an opt-in experimental feature (#5052)

Author: utpilla

src/OpenTelemetry/CHANGELOG.md

@@ -37,6 +37,13 @@
   `8.0.0`.
   ([#5051](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5051))
 
+* Revert the default behavior of Metrics SDK for Delta aggregation. It would not
+  reclaim unused Metric Points by default. You can enable the SDK to reclaim
+  unused Metric Points by setting the environment variable
+  `OTEL_DOTNET_EXPERIMENTAL_METRICS_RECLAIM_UNUSED_METRIC_POINTS` to `true`
+  before setting up the `MeterProvider`.
+  ([#5052](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5052))
+
 ## 1.7.0-alpha.1
 
 Released 2023-Oct-16

src/OpenTelemetry/Metrics/AggregatorStore.cs

@@ -25,6 +25,7 @@ namespace OpenTelemetry.Metrics;
 internal sealed class AggregatorStore
 {
     internal readonly bool OutputDelta;
+    internal readonly bool ShouldReclaimUnusedMetricPoints;
     internal long DroppedMeasurements = 0;
 
     private static readonly string MetricPointCapHitFixMessage = "Consider opting in for the experimental SDK feature to emit all the throttled metrics under the overflow attribute by setting env variable OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE = true. You could also modify instrumentation to reduce the number of unique key/value pair combinations. Or use Views to drop unwanted tags. Or use MeterProviderBuilder.SetMaxMetricPointsPerMetricStream to set higher limit.";
@@ -81,6 +82,7 @@ internal AggregatorStore(
         AggregationTemporality temporality,
         int maxMetricPoints,
         bool emitOverflowAttribute,
+        bool shouldReclaimUnusedMetricPoints,
         ExemplarFilter? exemplarFilter = null)
     {
         this.name = metricStreamIdentity.InstrumentName;
@@ -122,7 +124,9 @@ internal AggregatorStore(
             reservedMetricPointsCount++;
         }
 
-        if (this.OutputDelta)
+        this.ShouldReclaimUnusedMetricPoints = shouldReclaimUnusedMetricPoints;
+
+        if (this.OutputDelta && shouldReclaimUnusedMetricPoints)
         {
             this.availableMetricPoints = new Queue<int>(maxMetricPoints - reservedMetricPointsCount);
 
@@ -181,7 +185,7 @@ internal int Snapshot()
         this.batchSize = 0;
         if (this.OutputDelta)
         {
-            if (this.reclaimMetricPoints)
+            if (this.ShouldReclaimUnusedMetricPoints && this.reclaimMetricPoints)
             {
                 this.SnapshotDeltaWithMetricPointReclaim();
             }

src/OpenTelemetry/Metrics/MeterProviderSdk.cs

@@ -30,8 +30,10 @@ internal sealed class MeterProviderSdk : MeterProvider
     internal readonly IDisposable? OwnedServiceProvider;
     internal int ShutdownCount;
     internal bool Disposed;
+    internal bool ShouldReclaimUnusedMetricPoints;
 
     private const string EmitOverFlowAttributeConfigKey = "OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE";
+    private const string ReclaimUnusedMetricPointsConfigKey = "OTEL_DOTNET_EXPERIMENTAL_METRICS_RECLAIM_UNUSED_METRIC_POINTS";
 
     private readonly List<object> instrumentations = new();
     private readonly List<Func<Instrument, MetricStreamConfiguration?>> viewConfigs;
@@ -51,6 +53,7 @@ internal MeterProviderSdk(
 
         var config = serviceProvider!.GetRequiredService<IConfiguration>();
         _ = config.TryGetBoolValue(EmitOverFlowAttributeConfigKey, out bool isEmitOverflowAttributeKeySet);
+        _ = config.TryGetBoolValue(ReclaimUnusedMetricPointsConfigKey, out this.ShouldReclaimUnusedMetricPoints);
 
         this.ServiceProvider = serviceProvider!;
 

src/OpenTelemetry/Metrics/Metric.cs

@@ -60,6 +60,7 @@ internal Metric(
         AggregationTemporality temporality,
         int maxMetricPointsPerMetricStream,
         bool emitOverflowAttribute,
+        bool shouldReclaimUnusedMetricPoints,
         ExemplarFilter? exemplarFilter = null)
     {
         this.InstrumentIdentity = instrumentIdentity;
@@ -166,7 +167,7 @@ internal Metric(
             throw new NotSupportedException($"Unsupported Instrument Type: {instrumentIdentity.InstrumentType.FullName}");
         }
 
-        this.aggStore = new AggregatorStore(instrumentIdentity, aggType, temporality, maxMetricPointsPerMetricStream, emitOverflowAttribute, exemplarFilter);
+        this.aggStore = new AggregatorStore(instrumentIdentity, aggType, temporality, maxMetricPointsPerMetricStream, emitOverflowAttribute, shouldReclaimUnusedMetricPoints, exemplarFilter);
         this.Temporality = temporality;
         this.InstrumentDisposed = false;
     }

src/OpenTelemetry/Metrics/MetricPoint.cs

@@ -64,7 +64,7 @@ internal MetricPoint(
         Debug.Assert(aggregatorStore != null, "AggregatorStore was null.");
         Debug.Assert(histogramExplicitBounds != null, "Histogram explicit Bounds was null.");
 
-        if (aggregatorStore!.OutputDelta)
+        if (aggregatorStore!.OutputDelta && aggregatorStore.ShouldReclaimUnusedMetricPoints)
         {
             Debug.Assert(lookupData != null, "LookupData was null.");
         }

src/OpenTelemetry/Metrics/MetricReaderExt.cs

@@ -75,7 +75,8 @@ internal AggregationTemporality GetAggregationTemporality(Type instrumentType)
                 Metric? metric = null;
                 try
                 {
-                    metric = new Metric(metricStreamIdentity, this.GetAggregationTemporality(metricStreamIdentity.InstrumentType), this.maxMetricPointsPerMetricStream, this.emitOverflowAttribute, this.exemplarFilter);
+                    bool shouldReclaimUnusedMetricPoints = this.parentProvider is MeterProviderSdk meterProviderSdk && meterProviderSdk.ShouldReclaimUnusedMetricPoints;
+                    metric = new Metric(metricStreamIdentity, this.GetAggregationTemporality(metricStreamIdentity.InstrumentType), this.maxMetricPointsPerMetricStream, this.emitOverflowAttribute, shouldReclaimUnusedMetricPoints, this.exemplarFilter);
                 }
                 catch (NotSupportedException nse)
                 {
@@ -162,7 +163,8 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric
                 }
                 else
                 {
-                    Metric metric = new(metricStreamIdentity, this.GetAggregationTemporality(metricStreamIdentity.InstrumentType), this.maxMetricPointsPerMetricStream, this.emitOverflowAttribute, this.exemplarFilter);
+                    bool shouldReclaimUnusedMetricPoints = this.parentProvider is MeterProviderSdk meterProviderSdk && meterProviderSdk.ShouldReclaimUnusedMetricPoints;
+                    Metric metric = new(metricStreamIdentity, this.GetAggregationTemporality(metricStreamIdentity.InstrumentType), this.maxMetricPointsPerMetricStream, this.emitOverflowAttribute, shouldReclaimUnusedMetricPoints, this.exemplarFilter);
 
                     this.instrumentIdentityToMetric[metricStreamIdentity] = metric;
                     this.metrics![index] = metric;

test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs

@@ -29,16 +29,15 @@ public abstract class AggregatorTestsBase
     private static readonly MetricStreamIdentity MetricStreamIdentity = new(Instrument, HistogramConfiguration);
 
     private readonly bool emitOverflowAttribute;
+    private readonly bool shouldReclaimUnusedMetricPoints;
     private readonly AggregatorStore aggregatorStore;
 
-    protected AggregatorTestsBase(bool emitOverflowAttribute)
+    protected AggregatorTestsBase(bool emitOverflowAttribute, bool shouldReclaimUnusedMetricPoints)
     {
-        if (emitOverflowAttribute)
-        {
-            this.emitOverflowAttribute = emitOverflowAttribute;
-        }
+        this.emitOverflowAttribute = emitOverflowAttribute;
+        this.shouldReclaimUnusedMetricPoints = shouldReclaimUnusedMetricPoints;
 
-        this.aggregatorStore = new(MetricStreamIdentity, AggregationType.HistogramWithBuckets, AggregationTemporality.Cumulative, 1024, emitOverflowAttribute);
+        this.aggregatorStore = new(MetricStreamIdentity, AggregationType.HistogramWithBuckets, AggregationTemporality.Cumulative, 1024, emitOverflowAttribute, this.shouldReclaimUnusedMetricPoints);
     }
 
     [Fact]
@@ -268,7 +267,8 @@ public void HistogramBucketsDefaultUpdatesForSecondsTest(string meterName, strin
             AggregationType.Histogram,
             AggregationTemporality.Cumulative,
             maxMetricPoints: 1024,
-            this.emitOverflowAttribute);
+            this.emitOverflowAttribute,
+            this.shouldReclaimUnusedMetricPoints);
 
         KnownHistogramBuckets actualHistogramBounds = KnownHistogramBuckets.Default;
         if (aggregatorStore.HistogramBounds == Metric.DefaultHistogramBoundsShortSeconds)
@@ -345,6 +345,7 @@ internal void ExponentialHistogramTests(AggregationType aggregationType, Aggrega
             aggregationTemporality,
             maxMetricPoints: 1024,
             this.emitOverflowAttribute,
+            this.shouldReclaimUnusedMetricPoints,
             exemplarsEnabled ? new AlwaysOnExemplarFilter() : null);
 
         var expectedHistogram = new Base2ExponentialBucketHistogram();
@@ -453,7 +454,8 @@ internal void ExponentialMaxScaleConfigWorks(int? maxScale)
             AggregationType.Base2ExponentialHistogram,
             AggregationTemporality.Cumulative,
             maxMetricPoints: 1024,
-            this.emitOverflowAttribute);
+            this.emitOverflowAttribute,
+            this.shouldReclaimUnusedMetricPoints);
 
         aggregatorStore.Update(10, Array.Empty<KeyValuePair<string, object>>());
 
@@ -529,15 +531,31 @@ private class ThreadArguments
 public class AggregatorTests : AggregatorTestsBase
 {
     public AggregatorTests()
-        : base(false)
+        : base(emitOverflowAttribute: false, shouldReclaimUnusedMetricPoints: false)
     {
     }
 }
 
 public class AggregatorTestsWithOverflowAttribute : AggregatorTestsBase
 {
     public AggregatorTestsWithOverflowAttribute()
-        : base(true)
+        : base(emitOverflowAttribute: true, shouldReclaimUnusedMetricPoints: false)
+    {
+    }
+}
+
+public class AggregatorTestsWithReclaimAttribute : AggregatorTestsBase
+{
+    public AggregatorTestsWithReclaimAttribute()
+        : base(emitOverflowAttribute: false, shouldReclaimUnusedMetricPoints: true)
+    {
+    }
+}
+
+public class AggregatorTestsWithBothReclaimAndOverflowAttributes : AggregatorTestsBase
+{
+    public AggregatorTestsWithBothReclaimAndOverflowAttributes()
+        : base(emitOverflowAttribute: true, shouldReclaimUnusedMetricPoints: true)
     {
     }
 }