From 112e17f50e09583b99a2e73800e6e3a37ad77a6c Mon Sep 17 00:00:00 2001 From: xiang17 Date: Thu, 7 Nov 2024 15:03:16 -0800 Subject: [PATCH] [sdk-metrics] Stablize MetricPoint reclaim feature for delta aggregation (#5956) --- docs/metrics/README.md | 12 ++-- src/OpenTelemetry/CHANGELOG.md | 11 ++++ src/OpenTelemetry/Metrics/AggregatorStore.cs | 36 +---------- src/OpenTelemetry/Metrics/MeterProviderSdk.cs | 10 +-- src/OpenTelemetry/Metrics/Metric.cs | 2 - .../Metrics/MetricPoint/MetricPoint.cs | 4 +- .../Metrics/Reader/MetricReaderExt.cs | 5 -- .../Metrics/AggregatorTestsBase.cs | 24 ++------ .../Metrics/MetricApiTestsBase.cs | 22 ++----- .../MetricOverflowAttributeTestsBase.cs | 45 +++----------- .../Metrics/MetricPointReclaimTestsBase.cs | 61 ------------------- .../Metrics/MetricSnapshotTestsBase.cs | 15 +---- .../Metrics/MetricTestsBase.cs | 2 - 13 files changed, 40 insertions(+), 209 deletions(-) diff --git a/docs/metrics/README.md b/docs/metrics/README.md index 0394f63451b..31bbf1bbb64 100644 --- a/docs/metrics/README.md +++ b/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 diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 1f263f8443d..48f2553a008 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/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 diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index b7d560b559f..59214c3f090 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -20,7 +20,6 @@ internal sealed class AggregatorStore internal readonly HashSet? TagKeysInteresting; #endif internal readonly bool OutputDelta; - internal readonly bool OutputDeltaWithUnusedMetricPointReclaimEnabled; internal readonly int NumberOfMetricPoints; internal readonly ConcurrentDictionary? TagsToMetricPointIndexDictionaryDelta; internal readonly Func? ExemplarReservoirFactory; @@ -61,7 +60,6 @@ internal AggregatorStore( AggregationType aggType, AggregationTemporality temporality, int cardinalityLimit, - bool shouldReclaimUnusedMetricPoints, ExemplarFilterType? exemplarFilter = null, Func? 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(cardinalityLimit); @@ -184,15 +181,10 @@ internal void Update(double value, ReadOnlySpan> 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. diff --git a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs index a83666bb4c3..a07187de4ef 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs @@ -13,7 +13,6 @@ 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"; @@ -21,7 +20,6 @@ internal sealed class MeterProviderSdk : MeterProvider 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)) diff --git a/src/OpenTelemetry/Metrics/Metric.cs b/src/OpenTelemetry/Metrics/Metric.cs index 38abf4cc622..ac73955dda6 100644 --- a/src/OpenTelemetry/Metrics/Metric.cs +++ b/src/OpenTelemetry/Metrics/Metric.cs @@ -70,7 +70,6 @@ internal Metric( MetricStreamIdentity instrumentIdentity, AggregationTemporality temporality, int cardinalityLimit, - bool shouldReclaimUnusedMetricPoints, ExemplarFilterType? exemplarFilter = null, Func? exemplarReservoirFactory = null) { @@ -192,7 +191,6 @@ internal Metric( aggType, temporality, cardinalityLimit, - shouldReclaimUnusedMetricPoints, exemplarFilter, exemplarReservoirFactory); this.Temporality = temporality; diff --git a/src/OpenTelemetry/Metrics/MetricPoint/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint/MetricPoint.cs index b3d171c9ebb..8c0ad5951fc 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint/MetricPoint.cs +++ b/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); } diff --git a/src/OpenTelemetry/Metrics/Reader/MetricReaderExt.cs b/src/OpenTelemetry/Metrics/Reader/MetricReaderExt.cs index 194383a37a8..6074dd072f6 100644 --- a/src/OpenTelemetry/Metrics/Reader/MetricReaderExt.cs +++ b/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 AddMetricWithNoViews(Instrument instrument) metricStreamIdentity, this.GetAggregationTemporality(metricStreamIdentity.InstrumentType), this.cardinalityLimit, - this.reclaimUnusedMetricPoints, exemplarFilter); } catch (NotSupportedException nse) @@ -162,7 +160,6 @@ internal virtual List AddMetricWithViews(Instrument instrument, List AddMetricWithViews(Instrument instrument, List>()); @@ -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() { } } diff --git a/test/OpenTelemetry.Tests/Metrics/MetricApiTestsBase.cs b/test/OpenTelemetry.Tests/Metrics/MetricApiTestsBase.cs index 9c6735f7bcb..67b55caff09 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricApiTestsBase.cs +++ b/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(); - - 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) { } } diff --git a/test/OpenTelemetry.Tests/Metrics/MetricOverflowAttributeTestsBase.cs b/test/OpenTelemetry.Tests/Metrics/MetricOverflowAttributeTestsBase.cs index 6145de4d98a..9f05f4277bf 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricOverflowAttributeTestsBase.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricOverflowAttributeTestsBase.cs @@ -13,22 +13,14 @@ namespace OpenTelemetry.Metrics.Tests; public abstract class MetricOverflowAttributeTestsBase { - private readonly bool shouldReclaimUnusedMetricPoints; private readonly Dictionary configurationData = new() { }; private readonly IConfiguration configuration; - public MetricOverflowAttributeTestsBase(bool shouldReclaimUnusedMetricPoints) + public MetricOverflowAttributeTestsBase() { - this.shouldReclaimUnusedMetricPoints = shouldReclaimUnusedMetricPoints; - - if (shouldReclaimUnusedMetricPoints) - { - this.configurationData[MetricTestsBase.ReclaimUnusedMetricPointsConfigKey] = "true"; - } - this.configuration = new ConfigurationBuilder() .AddInMemoryCollection(this.configurationData) .Build(); @@ -139,15 +131,8 @@ public void MetricOverflowAttributeIsRecordedCorrectlyForCounter(MetricReaderTem int expectedSum; // Number of metric points that were available before the 2500 measurements were made = 2000 (max MetricPoints) - if (this.shouldReclaimUnusedMetricPoints) - { - // If unused metric points are reclaimed, then number of metric points dropped = 2500 - 2000 = 500 - expectedSum = 2500; // 500 * 5 - } - else - { - expectedSum = 12500; // 2500 * 5 - } + // Because unused metric points are reclaimed, number of metric points dropped = 2500 - 2000 = 500 + expectedSum = 2500; // 500 * 5 Assert.Equal(expectedSum, overflowMetricPoint.GetSumLong()); } @@ -291,17 +276,9 @@ public void MetricOverflowAttributeIsRecordedCorrectlyForHistogram(MetricReaderT int expectedSum; // Number of metric points that were available before the 2500 measurements were made = 2000 (max MetricPoints) - if (this.shouldReclaimUnusedMetricPoints) - { - // If unused metric points are reclaimed, then number of metric points dropped = 2500 - 2000 = 500 - expectedCount = 500; - expectedSum = 2500; // 500 * 5 - } - else - { - expectedCount = 2500; - expectedSum = 12500; // 2500 * 5 - } + // Because unused metric points are reclaimed, number of metric points dropped = 2500 - 2000 = 500 + expectedCount = 500; + expectedSum = 2500; // 500 * 5 Assert.Equal(expectedCount, overflowMetricPoint.GetHistogramCount()); Assert.Equal(expectedSum, overflowMetricPoint.GetHistogramSum()); @@ -345,15 +322,7 @@ public void MetricOverflowAttributeIsRecordedCorrectlyForHistogram(MetricReaderT public class MetricOverflowAttributeTests : MetricOverflowAttributeTestsBase { public MetricOverflowAttributeTests() - : base(false) - { - } -} - -public class MetricOverflowAttributeTestsWithReclaimAttribute : MetricOverflowAttributeTestsBase -{ - public MetricOverflowAttributeTestsWithReclaimAttribute() - : base(true) + : base() { } } diff --git a/test/OpenTelemetry.Tests/Metrics/MetricPointReclaimTestsBase.cs b/test/OpenTelemetry.Tests/Metrics/MetricPointReclaimTestsBase.cs index effe208eda3..b6f6dfcd442 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricPointReclaimTestsBase.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricPointReclaimTestsBase.cs @@ -15,7 +15,6 @@ public abstract class MetricPointReclaimTestsBase { private readonly Dictionary configurationData = new() { - [MetricTestsBase.ReclaimUnusedMetricPointsConfigKey] = "true", }; private readonly IConfiguration configuration; @@ -27,66 +26,6 @@ protected MetricPointReclaimTestsBase() .Build(); } - [Theory] - [InlineData("false", false)] - [InlineData("False", false)] - [InlineData("FALSE", false)] - [InlineData("true", true)] - [InlineData("True", true)] - [InlineData("TRUE", true)] - public void TestReclaimAttributeConfigWithEnvVar(string value, bool isReclaimAttributeKeySet) - { - // Clear the environment variable value first - Environment.SetEnvironmentVariable(MetricTestsBase.ReclaimUnusedMetricPointsConfigKey, null); - - // Set the environment variable to the value provided in the test input - Environment.SetEnvironmentVariable(MetricTestsBase.ReclaimUnusedMetricPointsConfigKey, value); - - var exportedItems = new List(); - - var meter = new Meter(Utils.GetCurrentMethodName()); - - using var meterProvider = Sdk.CreateMeterProviderBuilder() - .AddMeter(meter.Name) - .AddInMemoryExporter(exportedItems) - .Build(); - - var meterProviderSdk = meterProvider as MeterProviderSdk; - Assert.NotNull(meterProviderSdk); - Assert.Equal(isReclaimAttributeKeySet, meterProviderSdk.ReclaimUnusedMetricPoints); - } - - [Theory] - [InlineData("false", false)] - [InlineData("False", false)] - [InlineData("FALSE", false)] - [InlineData("true", true)] - [InlineData("True", true)] - [InlineData("TRUE", true)] - public void TestReclaimAttributeConfigWithOtherConfigProvider(string value, bool isReclaimAttributeKeySet) - { - var exportedItems = new List(); - - var meter = new Meter(Utils.GetCurrentMethodName()); - - using var meterProvider = Sdk.CreateMeterProviderBuilder() - .ConfigureServices(services => - { - var configuration = new ConfigurationBuilder() - .AddInMemoryCollection(new Dictionary { [MetricTestsBase.ReclaimUnusedMetricPointsConfigKey] = value }) - .Build(); - - services.AddSingleton(configuration); - }) - .AddMeter(meter.Name) - .AddInMemoryExporter(exportedItems) - .Build(); - - var meterProviderSdk = meterProvider as MeterProviderSdk; - Assert.NotNull(meterProviderSdk); - Assert.Equal(isReclaimAttributeKeySet, meterProviderSdk.ReclaimUnusedMetricPoints); - } - [Theory] [InlineData(false)] [InlineData(true)] diff --git a/test/OpenTelemetry.Tests/Metrics/MetricSnapshotTestsBase.cs b/test/OpenTelemetry.Tests/Metrics/MetricSnapshotTestsBase.cs index 54b8ed95868..d3c2a1b4339 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricSnapshotTestsBase.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricSnapshotTestsBase.cs @@ -16,10 +16,9 @@ public abstract class MetricSnapshotTestsBase { private readonly IConfiguration configuration; - protected MetricSnapshotTestsBase(bool shouldReclaimUnusedMetricPoints) + protected MetricSnapshotTestsBase() { - this.configuration = MetricApiTestsBase.BuildConfiguration( - shouldReclaimUnusedMetricPoints); + this.configuration = MetricApiTestsBase.BuildConfiguration(); } [Fact] @@ -297,15 +296,7 @@ public void VerifySnapshot_ExponentialHistogram() public class MetricSnapshotTests : MetricSnapshotTestsBase { public MetricSnapshotTests() - : base(shouldReclaimUnusedMetricPoints: false) - { - } -} - -public class MetricSnapshotTestsWithReclaimAttribute : MetricSnapshotTestsBase -{ - public MetricSnapshotTestsWithReclaimAttribute() - : base(shouldReclaimUnusedMetricPoints: true) + : base() { } } diff --git a/test/OpenTelemetry.Tests/Metrics/MetricTestsBase.cs b/test/OpenTelemetry.Tests/Metrics/MetricTestsBase.cs index 746c3b6aeb3..9023575db66 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricTestsBase.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricTestsBase.cs @@ -16,8 +16,6 @@ namespace OpenTelemetry.Metrics.Tests; public class MetricTestsBase { - public const string ReclaimUnusedMetricPointsConfigKey = "OTEL_DOTNET_EXPERIMENTAL_METRICS_RECLAIM_UNUSED_METRIC_POINTS"; - protected readonly IConfiguration? configuration; protected MetricTestsBase()