[sdk-metrics] Stablize MetricPoint reclaim feature for delta aggregation (open-telemetry#5956)

Author: xiang17

docs/metrics/README.md

@@ -402,14 +402,10 @@ is used, it is possible to choose a smaller cardinality limit by allowing the
 SDK to reclaim unused metric points.
 
 > [!NOTE]
-> Reclaim unused metric points feature was introduced in OpenTelemetry .NET
-  [1.7.0-alpha.1](../../src/OpenTelemetry/CHANGELOG.md#170-alpha1). It is
-  currently an experimental feature which can be turned on by setting the
-  environment variable
-  `OTEL_DOTNET_EXPERIMENTAL_METRICS_RECLAIM_UNUSED_METRIC_POINTS=true`. Once the
-  [OpenTelemetry
-  Specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#overflow-attribute)
-  become stable, this feature will be turned on by default.
+> Metric points reclaim is the default and only behavior for Delta Aggregation
+  Temporality starting with version 1.10.0. In version 1.7.0 - 1.9.0, it was an
+  experimental feature that could be enabled by setting the environment variable
+  `OTEL_DOTNET_EXPERIMENTAL_METRICS_RECLAIM_UNUSED_METRIC_POINTS=true`.
 
 ### Memory Preallocation
 

src/OpenTelemetry/CHANGELOG.md

@@ -6,6 +6,17 @@ Notes](../../RELEASENOTES.md).
 
 ## Unreleased
 
+* Promote MetricPoint reclaim feature for delta aggregation from experimental to
+  stable.
+  Previously, it is an experimental feature which can be turned on by setting
+  the environment variable
+  `OTEL_DOTNET_EXPERIMENTAL_METRICS_RECLAIM_UNUSED_METRIC_POINTS=true`.
+  Now that the [OpenTelemetry
+  Specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#overflow-attribute)
+  has become stable. The feature is the default and the only allowed behavior
+  without the need to set an environment variable.
+  ([#5956](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5956))
+
 ## 1.10.0-rc.1
 
 Released 2024-Nov-01

src/OpenTelemetry/Metrics/AggregatorStore.cs

@@ -20,7 +20,6 @@ internal sealed class AggregatorStore
     internal readonly HashSet<string>? TagKeysInteresting;
 #endif
     internal readonly bool OutputDelta;
-    internal readonly bool OutputDeltaWithUnusedMetricPointReclaimEnabled;
     internal readonly int NumberOfMetricPoints;
     internal readonly ConcurrentDictionary<Tags, LookupData>? TagsToMetricPointIndexDictionaryDelta;
     internal readonly Func<ExemplarReservoir?>? ExemplarReservoirFactory;
@@ -61,7 +60,6 @@ internal AggregatorStore(
         AggregationType aggType,
         AggregationTemporality temporality,
         int cardinalityLimit,
-        bool shouldReclaimUnusedMetricPoints,
         ExemplarFilterType? exemplarFilter = null,
         Func<ExemplarReservoir?>? exemplarReservoirFactory = null)
     {
@@ -110,9 +108,8 @@ internal AggregatorStore(
         // Newer attributes should be added starting at the index: 2
         this.metricPointIndex = 1;
 
-        this.OutputDeltaWithUnusedMetricPointReclaimEnabled = shouldReclaimUnusedMetricPoints && this.OutputDelta;
-
-        if (this.OutputDeltaWithUnusedMetricPointReclaimEnabled)
+        // Always reclaim unused MetricPoints for Delta aggregation temporality
+        if (this.OutputDelta)
         {
             this.availableMetricPoints = new Queue<int>(cardinalityLimit);
 
@@ -184,15 +181,10 @@ internal void Update(double value, ReadOnlySpan<KeyValuePair<string, object?>> t
     internal int Snapshot()
     {
         this.batchSize = 0;
-        if (this.OutputDeltaWithUnusedMetricPointReclaimEnabled)
+        if (this.OutputDelta)
         {
             this.SnapshotDeltaWithMetricPointReclaim();
         }
-        else if (this.OutputDelta)
-        {
-            var indexSnapshot = Math.Min(this.metricPointIndex, this.NumberOfMetricPoints - 1);
-            this.SnapshotDelta(indexSnapshot);
-        }
         else
         {
             var indexSnapshot = Math.Min(this.metricPointIndex, this.NumberOfMetricPoints - 1);
@@ -203,28 +195,6 @@ internal int Snapshot()
         return this.batchSize;
     }
 
-    internal void SnapshotDelta(int indexSnapshot)
-    {
-        for (int i = 0; i <= indexSnapshot; i++)
-        {
-            ref var metricPoint = ref this.metricPoints[i];
-            if (metricPoint.MetricPointStatus == MetricPointStatus.NoCollectPending)
-            {
-                continue;
-            }
-
-            this.TakeMetricPointSnapshot(ref metricPoint, outputDelta: true);
-
-            this.currentMetricPointBatch[this.batchSize] = i;
-            this.batchSize++;
-        }
-
-        if (this.EndTimeInclusive != default)
-        {
-            this.StartTimeExclusive = this.EndTimeInclusive;
-        }
-    }
-
     internal void SnapshotDeltaWithMetricPointReclaim()
     {
         // Index = 0 is reserved for the case where no dimensions are provided.

src/OpenTelemetry/Metrics/MeterProviderSdk.cs

@@ -13,15 +13,13 @@ namespace OpenTelemetry.Metrics;
 
 internal sealed class MeterProviderSdk : MeterProvider
 {
-    internal const string ReclaimUnusedMetricPointsConfigKey = "OTEL_DOTNET_EXPERIMENTAL_METRICS_RECLAIM_UNUSED_METRIC_POINTS";
     internal const string ExemplarFilterConfigKey = "OTEL_METRICS_EXEMPLAR_FILTER";
     internal const string ExemplarFilterHistogramsConfigKey = "OTEL_DOTNET_EXPERIMENTAL_METRICS_EXEMPLAR_FILTER_HISTOGRAMS";
 
     internal readonly IServiceProvider ServiceProvider;
     internal readonly IDisposable? OwnedServiceProvider;
     internal int ShutdownCount;
     internal bool Disposed;
-    internal bool ReclaimUnusedMetricPoints;
     internal ExemplarFilterType? ExemplarFilter;
     internal ExemplarFilterType? ExemplarFilterForHistograms;
     internal Action? OnCollectObservableInstruments;
@@ -73,7 +71,7 @@ internal MeterProviderSdk(
         this.viewConfigs = state.ViewConfigs;
 
         OpenTelemetrySdkEventSource.Log.MeterProviderSdkEvent(
-            $"MeterProvider configuration: {{MetricLimit={state.MetricLimit}, CardinalityLimit={state.CardinalityLimit}, ReclaimUnusedMetricPoints={this.ReclaimUnusedMetricPoints}, ExemplarFilter={this.ExemplarFilter}, ExemplarFilterForHistograms={this.ExemplarFilterForHistograms}}}.");
+            $"MeterProvider configuration: {{MetricLimit={state.MetricLimit}, CardinalityLimit={state.CardinalityLimit}, ExemplarFilter={this.ExemplarFilter}, ExemplarFilterForHistograms={this.ExemplarFilterForHistograms}}}.");
 
         foreach (var reader in state.Readers)
         {
@@ -84,7 +82,6 @@ internal MeterProviderSdk(
             reader.ApplyParentProviderSettings(
                 state.MetricLimit,
                 state.CardinalityLimit,
-                this.ReclaimUnusedMetricPoints,
                 this.ExemplarFilter,
                 this.ExemplarFilterForHistograms);
 
@@ -483,11 +480,6 @@ protected override void Dispose(bool disposing)
 
     private void ApplySpecificationConfigurationKeys(IConfiguration configuration)
     {
-        if (configuration.TryGetBoolValue(OpenTelemetrySdkEventSource.Log, ReclaimUnusedMetricPointsConfigKey, out this.ReclaimUnusedMetricPoints))
-        {
-            OpenTelemetrySdkEventSource.Log.MeterProviderSdkEvent("Reclaim unused metric point feature enabled via configuration.");
-        }
-
         var hasProgrammaticExemplarFilterValue = this.ExemplarFilter.HasValue;
 
         if (configuration.TryGetStringValue(ExemplarFilterConfigKey, out var configValue))

src/OpenTelemetry/Metrics/Metric.cs

@@ -70,7 +70,6 @@ internal Metric(
         MetricStreamIdentity instrumentIdentity,
         AggregationTemporality temporality,
         int cardinalityLimit,
-        bool shouldReclaimUnusedMetricPoints,
         ExemplarFilterType? exemplarFilter = null,
         Func<ExemplarReservoir?>? exemplarReservoirFactory = null)
     {
@@ -192,7 +191,6 @@ internal Metric(
             aggType,
             temporality,
             cardinalityLimit,
-            shouldReclaimUnusedMetricPoints,
             exemplarFilter,
             exemplarReservoirFactory);
         this.Temporality = temporality;

src/OpenTelemetry/Metrics/MetricPoint/MetricPoint.cs

@@ -46,7 +46,7 @@ internal MetricPoint(
     {
         Debug.Assert(aggregatorStore != null, "AggregatorStore was null.");
         Debug.Assert(histogramExplicitBounds != null, "Histogram explicit Bounds was null.");
-        Debug.Assert(!aggregatorStore!.OutputDeltaWithUnusedMetricPointReclaimEnabled || lookupData != null, "LookupData was null.");
+        Debug.Assert(!aggregatorStore!.OutputDelta || lookupData != null, "LookupData was null.");
 
         this.aggType = aggType;
         this.Tags = new ReadOnlyTagCollection(tagKeysAndValues);
@@ -1086,7 +1086,7 @@ private void CompleteUpdate()
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     private void CompleteUpdateWithoutMeasurement()
     {
-        if (this.aggregatorStore.OutputDeltaWithUnusedMetricPointReclaimEnabled)
+        if (this.aggregatorStore.OutputDelta)
         {
             Interlocked.Decrement(ref this.ReferenceCount);
         }

src/OpenTelemetry/Metrics/Reader/MetricReaderExt.cs

@@ -22,7 +22,6 @@ public abstract partial class MetricReader
     private Metric?[]? metrics;
     private Metric[]? metricsCurrentBatch;
     private int metricIndex = -1;
-    private bool reclaimUnusedMetricPoints;
     private ExemplarFilterType? exemplarFilter;
     private ExemplarFilterType? exemplarFilterForHistograms;
 
@@ -81,7 +80,6 @@ internal virtual List<Metric> AddMetricWithNoViews(Instrument instrument)
                         metricStreamIdentity,
                         this.GetAggregationTemporality(metricStreamIdentity.InstrumentType),
                         this.cardinalityLimit,
-                        this.reclaimUnusedMetricPoints,
                         exemplarFilter);
                 }
                 catch (NotSupportedException nse)
@@ -162,7 +160,6 @@ internal virtual List<Metric> AddMetricWithViews(Instrument instrument, List<Met
                         metricStreamIdentity,
                         this.GetAggregationTemporality(metricStreamIdentity.InstrumentType),
                         metricStreamConfig?.CardinalityLimit ?? this.cardinalityLimit,
-                        this.reclaimUnusedMetricPoints,
                         exemplarFilter,
                         metricStreamConfig?.ExemplarReservoirFactory);
 
@@ -181,15 +178,13 @@ internal virtual List<Metric> AddMetricWithViews(Instrument instrument, List<Met
     internal void ApplyParentProviderSettings(
         int metricLimit,
         int cardinalityLimit,
-        bool reclaimUnusedMetricPoints,
         ExemplarFilterType? exemplarFilter,
         ExemplarFilterType? exemplarFilterForHistograms)
     {
         this.metricLimit = metricLimit;
         this.metrics = new Metric[metricLimit];
         this.metricsCurrentBatch = new Metric[metricLimit];
         this.cardinalityLimit = cardinalityLimit;
-        this.reclaimUnusedMetricPoints = reclaimUnusedMetricPoints;
         this.exemplarFilter = exemplarFilter;
         this.exemplarFilterForHistograms = exemplarFilterForHistograms;
     }

test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs

@@ -16,14 +16,11 @@ public abstract class AggregatorTestsBase
     private static readonly ExplicitBucketHistogramConfiguration HistogramConfiguration = new() { Boundaries = Metric.DefaultHistogramBounds };
     private static readonly MetricStreamIdentity MetricStreamIdentity = new(Instrument, HistogramConfiguration);
 
-    private readonly bool shouldReclaimUnusedMetricPoints;
     private readonly AggregatorStore aggregatorStore;
 
-    protected AggregatorTestsBase(bool shouldReclaimUnusedMetricPoints)
+    protected AggregatorTestsBase()
     {
-        this.shouldReclaimUnusedMetricPoints = shouldReclaimUnusedMetricPoints;
-
-        this.aggregatorStore = new(MetricStreamIdentity, AggregationType.HistogramWithBuckets, AggregationTemporality.Cumulative, 1024, this.shouldReclaimUnusedMetricPoints);
+        this.aggregatorStore = new(MetricStreamIdentity, AggregationType.HistogramWithBuckets, AggregationTemporality.Cumulative, 1024);
     }
 
     [Fact]
@@ -250,8 +247,7 @@ public void HistogramBucketsDefaultUpdatesForSecondsTest(string meterName, strin
             metricStreamIdentity,
             AggregationType.Histogram,
             AggregationTemporality.Cumulative,
-            cardinalityLimit: 1024,
-            this.shouldReclaimUnusedMetricPoints);
+            cardinalityLimit: 1024);
 
         KnownHistogramBuckets actualHistogramBounds = KnownHistogramBuckets.Default;
         if (aggregatorStore.HistogramBounds == Metric.DefaultHistogramBoundsShortSeconds)
@@ -327,7 +323,6 @@ internal void ExponentialHistogramTests(AggregationType aggregationType, Aggrega
             aggregationType,
             aggregationTemporality,
             cardinalityLimit: 1024,
-            this.shouldReclaimUnusedMetricPoints,
             exemplarsEnabled ? ExemplarFilterType.AlwaysOn : null);
 
         var expectedHistogram = new Base2ExponentialBucketHistogram();
@@ -435,8 +430,7 @@ internal void ExponentialMaxScaleConfigWorks(int? maxScale)
             metricStreamIdentity,
             AggregationType.Base2ExponentialHistogram,
             AggregationTemporality.Cumulative,
-            cardinalityLimit: 1024,
-            this.shouldReclaimUnusedMetricPoints);
+            cardinalityLimit: 1024);
 
         aggregatorStore.Update(10, Array.Empty<KeyValuePair<string, object?>>());
 
@@ -520,15 +514,7 @@ public ThreadArguments(MetricPoint histogramPoint, ManualResetEvent mreToEnsureA
 public class AggregatorTests : AggregatorTestsBase
 {
     public AggregatorTests()
-        : base(shouldReclaimUnusedMetricPoints: false)
-    {
-    }
-}
-
-public class AggregatorTestsWithReclaimAttribute : AggregatorTestsBase
-{
-    public AggregatorTestsWithReclaimAttribute()
-        : base(shouldReclaimUnusedMetricPoints: true)
+        : base()
     {
     }
 }

test/OpenTelemetry.Tests/Metrics/MetricApiTestsBase.cs

@@ -23,8 +23,8 @@ public abstract class MetricApiTestsBase : MetricTestsBase
     private static readonly int NumberOfMetricUpdateByEachThread = 100000;
     private readonly ITestOutputHelper output;
 
-    protected MetricApiTestsBase(ITestOutputHelper output, bool shouldReclaimUnusedMetricPoints)
-        : base(BuildConfiguration(shouldReclaimUnusedMetricPoints))
+    protected MetricApiTestsBase(ITestOutputHelper output)
+        : base(BuildConfiguration())
     {
         this.output = output;
     }
@@ -1703,15 +1703,9 @@ public void GaugeHandlesNoNewMeasurementsCorrectlyWithTemporality(MetricReaderTe
         }
     }
 
-    internal static IConfiguration BuildConfiguration(bool shouldReclaimUnusedMetricPoints)
+    internal static IConfiguration BuildConfiguration()
     {
         var configurationData = new Dictionary<string, string?>();
-
-        if (shouldReclaimUnusedMetricPoints)
-        {
-            configurationData[ReclaimUnusedMetricPointsConfigKey] = "true";
-        }
-
         return new ConfigurationBuilder()
             .AddInMemoryCollection(configurationData)
             .Build();
@@ -1888,15 +1882,7 @@ public UpdateThreadArguments(ManualResetEvent mreToBlockUpdateThread, ManualRese
 public class MetricApiTest : MetricApiTestsBase
 {
     public MetricApiTest(ITestOutputHelper output)
-        : base(output, shouldReclaimUnusedMetricPoints: false)
-    {
-    }
-}
-
-public class MetricApiTestWithReclaimAttribute : MetricApiTestsBase
-{
-    public MetricApiTestWithReclaimAttribute(ITestOutputHelper output)
-        : base(output, shouldReclaimUnusedMetricPoints: true)
+        : base(output)
     {
     }
 }