[sdk-metrics] Promote overflow attribute from experimental to stable (o…

…pen-telemetry#5909) Co-authored-by: Reiley Yang <[email protected]> Co-authored-by: Mikel Blanchard <[email protected]>
CodeBlanch · Oct 25, 2024 · 9f41ead · 9f41ead
1 parent 5efc683
commit 9f41ead
Show file tree

Hide file tree

Showing 12 changed files with 60 additions and 257 deletions.
diff --git a/docs/metrics/README.md b/docs/metrics/README.md
@@ -10,11 +10,13 @@
   * [Instruments](#instruments)
 * [MeterProvider Management](#meterprovider-management)
 * [Memory Management](#memory-management)
+  * [Example](#example)
   * [Pre-Aggregation](#pre-aggregation)
   * [Cardinality Limits](#cardinality-limits)
   * [Memory Preallocation](#memory-preallocation)
 * [Metrics Correlation](#metrics-correlation)
 * [Metrics Enrichment](#metrics-enrichment)
+* [Common issues that lead to missing metrics](#common-issues-that-lead-to-missing-metrics)
 
 </details>
 
@@ -386,22 +388,13 @@ and the `MetricStreamConfiguration.CardinalityLimit` setting. Refer to this
 [doc](../../docs/metrics/customizing-the-sdk/README.md#changing-the-cardinality-limit-for-a-metric)
 for more information.
 
-Given a metric, once the cardinality limit is reached, any new measurement which
-cannot be independently aggregated because of the limit will be dropped or
-aggregated using the [overflow
-attribute](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#overflow-attribute)
-(if enabled). When NOT using the overflow attribute feature a warning is written
-to the [self-diagnostic log](../../src/OpenTelemetry/README.md#self-diagnostics)
-the first time an overflow is detected for a given metric.
-
-> [!NOTE]
-> Overflow attribute was introduced in OpenTelemetry .NET
-  [1.6.0-rc.1](../../src/OpenTelemetry/CHANGELOG.md#160-rc1). It is currently an
-  experimental feature which can be turned on by setting the environment
-  variable `OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE=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.
+Given a metric, once the cardinality limit is reached, any new measurement
+that could not be independently aggregated will be aggregated using the
+[overflow attribute](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#overflow-attribute).
+In versions prior to 1.10.0, the default behavior when cardinality limit was
+reached was to drop the measurement. Users had the ability to opt-in to use
+overflow attribute instead, but this behavior is the default and the only
+allowed behavior starting with version 1.10.0.
 
 When [Delta Aggregation
 Temporality](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/data-model.md#temporality)

diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md
@@ -6,6 +6,31 @@ Notes](../../RELEASENOTES.md).
 
 ## Unreleased
 
+* Promoted overflow attribute from experimental to stable and removed the
+  `OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE` environment variable.
+
+  **Previous Behavior:**
+  By default, when the cardinality limit was reached, measurements were dropped,
+  and an internal log was emitted the first time this occurred. Users could
+  opt-in to experimental overflow attribute feature with
+  `OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE=true`.
+  With this setting, the SDK would use an overflow attribute
+  (`otel.metric.overflow = true`) to aggregate measurements instead of dropping
+  measurements. No internal log was emitted in this case.
+
+  **New Behavior:**
+  The SDK now always uses the overflow attribute (`otel.metric.overflow = true`)
+  to aggregate measurements when the cardinality limit is reached. The previous
+  approach of dropping measurements has been removed. No internal logs are
+  emitted when the limit is hit.
+
+  The default cardinality limit remains 2000 per metric. To set the cardinality
+  limit for an individual metric, use the [changing cardinality limit for a
+  Metric](../../docs/metrics/customizing-the-sdk/README.md#changing-the-cardinality-limit-for-a-metric).
+
+  There is NO ability to revert to old behavior.
+  ([#5909](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5909))
+
 ## 1.10.0-beta.1
 
 Released 2024-Sep-30

diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs
@@ -22,13 +22,11 @@ internal sealed class AggregatorStore
     internal readonly bool OutputDelta;
     internal readonly bool OutputDeltaWithUnusedMetricPointReclaimEnabled;
     internal readonly int NumberOfMetricPoints;
-    internal readonly bool EmitOverflowAttribute;
     internal readonly ConcurrentDictionary<Tags, LookupData>? TagsToMetricPointIndexDictionaryDelta;
     internal readonly Func<ExemplarReservoir?>? ExemplarReservoirFactory;
     internal long DroppedMeasurements = 0;
 
     private const ExemplarFilterType DefaultExemplarFilter = ExemplarFilterType.AlwaysOff;
-    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.";
     private static readonly Comparison<KeyValuePair<string, object?>> DimensionComparisonDelegate = (x, y) => x.Key.CompareTo(y.Key);
 
     private readonly Lock lockZeroTags = new();
@@ -42,7 +40,6 @@ internal sealed class AggregatorStore
         new();
 
     private readonly string name;
-    private readonly string metricPointCapHitMessage;
     private readonly MetricPoint[] metricPoints;
     private readonly int[] currentMetricPointBatch;
     private readonly AggregationType aggType;
@@ -56,7 +53,6 @@ internal sealed class AggregatorStore
 
     private int metricPointIndex = 0;
     private int batchSize = 0;
-    private int metricCapHitMessageLogged;
     private bool zeroTagMetricPointInitialized;
     private bool overflowTagMetricPointInitialized;
 
@@ -65,7 +61,6 @@ internal AggregatorStore(
         AggregationType aggType,
         AggregationTemporality temporality,
         int cardinalityLimit,
-        bool emitOverflowAttribute,
         bool shouldReclaimUnusedMetricPoints,
         ExemplarFilterType? exemplarFilter = null,
         Func<ExemplarReservoir?>? exemplarReservoirFactory = null)
@@ -77,7 +72,6 @@ internal AggregatorStore(
         // Previously, these were included within the original cardinalityLimit, but now they are explicitly added to enhance clarity.
         this.NumberOfMetricPoints = cardinalityLimit + 2;
 
-        this.metricPointCapHitMessage = $"Maximum MetricPoints limit reached for this Metric stream. Configured limit: {cardinalityLimit}";
         this.metricPoints = new MetricPoint[this.NumberOfMetricPoints];
         this.currentMetricPointBatch = new int[this.NumberOfMetricPoints];
         this.aggType = aggType;
@@ -105,8 +99,6 @@ internal AggregatorStore(
             this.tagsKeysInterestingCount = hs.Count;
         }
 
-        this.EmitOverflowAttribute = emitOverflowAttribute;
-
         this.exemplarFilter = exemplarFilter ?? DefaultExemplarFilter;
         Debug.Assert(
             this.exemplarFilter == ExemplarFilterType.AlwaysOff
@@ -245,17 +237,14 @@ internal void SnapshotDeltaWithMetricPointReclaim()
             this.batchSize++;
         }
 
-        if (this.EmitOverflowAttribute)
+        // TakeSnapshot for the MetricPoint for overflow
+        ref var metricPointForOverflow = ref this.metricPoints[1];
+        if (metricPointForOverflow.MetricPointStatus != MetricPointStatus.NoCollectPending)
         {
-            // TakeSnapshot for the MetricPoint for overflow
-            ref var metricPointForOverflow = ref this.metricPoints[1];
-            if (metricPointForOverflow.MetricPointStatus != MetricPointStatus.NoCollectPending)
-            {
-                this.TakeMetricPointSnapshot(ref metricPointForOverflow, outputDelta: true);
+            this.TakeMetricPointSnapshot(ref metricPointForOverflow, outputDelta: true);
 
-                this.currentMetricPointBatch[this.batchSize] = 1;
-                this.batchSize++;
-            }
+            this.currentMetricPointBatch[this.batchSize] = 1;
+            this.batchSize++;
         }
 
         // Index 0 and 1 are reserved for no tags and overflow
@@ -994,16 +983,8 @@ private void UpdateLongMetricPoint(int metricPointIndex, long value, ReadOnlySpa
         if (metricPointIndex < 0)
         {
             Interlocked.Increment(ref this.DroppedMeasurements);
-
-            if (this.EmitOverflowAttribute)
-            {
-                this.InitializeOverflowTagPointIfNotInitialized();
-                this.metricPoints[1].Update(value);
-            }
-            else if (Interlocked.CompareExchange(ref this.metricCapHitMessageLogged, 1, 0) == 0)
-            {
-                OpenTelemetrySdkEventSource.Log.MeasurementDropped(this.name, this.metricPointCapHitMessage, MetricPointCapHitFixMessage);
-            }
+            this.InitializeOverflowTagPointIfNotInitialized();
+            this.metricPoints[1].Update(value);
 
             return;
         }
@@ -1049,16 +1030,8 @@ private void UpdateDoubleMetricPoint(int metricPointIndex, double value, ReadOnl
         if (metricPointIndex < 0)
         {
             Interlocked.Increment(ref this.DroppedMeasurements);
-
-            if (this.EmitOverflowAttribute)
-            {
-                this.InitializeOverflowTagPointIfNotInitialized();
-                this.metricPoints[1].Update(value);
-            }
-            else if (Interlocked.CompareExchange(ref this.metricCapHitMessageLogged, 1, 0) == 0)
-            {
-                OpenTelemetrySdkEventSource.Log.MeasurementDropped(this.name, this.metricPointCapHitMessage, MetricPointCapHitFixMessage);
-            }
+            this.InitializeOverflowTagPointIfNotInitialized();
+            this.metricPoints[1].Update(value);
 
             return;
         }

diff --git 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 EmitOverFlowAttributeConfigKey = "OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE";
     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";
@@ -22,7 +21,6 @@ internal sealed class MeterProviderSdk : MeterProvider
     internal readonly IDisposable? OwnedServiceProvider;
     internal int ShutdownCount;
     internal bool Disposed;
-    internal bool EmitOverflowAttribute;
     internal bool ReclaimUnusedMetricPoints;
     internal ExemplarFilterType? ExemplarFilter;
     internal ExemplarFilterType? ExemplarFilterForHistograms;
@@ -75,7 +73,7 @@ internal MeterProviderSdk(
         this.viewConfigs = state.ViewConfigs;
 
         OpenTelemetrySdkEventSource.Log.MeterProviderSdkEvent(
-            $"MeterProvider configuration: {{MetricLimit={state.MetricLimit}, CardinalityLimit={state.CardinalityLimit}, EmitOverflowAttribute={this.EmitOverflowAttribute}, ReclaimUnusedMetricPoints={this.ReclaimUnusedMetricPoints}, ExemplarFilter={this.ExemplarFilter}, ExemplarFilterForHistograms={this.ExemplarFilterForHistograms}}}.");
+            $"MeterProvider configuration: {{MetricLimit={state.MetricLimit}, CardinalityLimit={state.CardinalityLimit}, ReclaimUnusedMetricPoints={this.ReclaimUnusedMetricPoints}, ExemplarFilter={this.ExemplarFilter}, ExemplarFilterForHistograms={this.ExemplarFilterForHistograms}}}.");
 
         foreach (var reader in state.Readers)
         {
@@ -86,7 +84,6 @@ internal MeterProviderSdk(
             reader.ApplyParentProviderSettings(
                 state.MetricLimit,
                 state.CardinalityLimit,
-                this.EmitOverflowAttribute,
                 this.ReclaimUnusedMetricPoints,
                 this.ExemplarFilter,
                 this.ExemplarFilterForHistograms);
@@ -486,11 +483,6 @@ protected override void Dispose(bool disposing)
 
     private void ApplySpecificationConfigurationKeys(IConfiguration configuration)
     {
-        if (configuration.TryGetBoolValue(OpenTelemetrySdkEventSource.Log, EmitOverFlowAttributeConfigKey, out this.EmitOverflowAttribute))
-        {
-            OpenTelemetrySdkEventSource.Log.MeterProviderSdkEvent("Overflow attribute feature enabled via configuration.");
-        }
-
         if (configuration.TryGetBoolValue(OpenTelemetrySdkEventSource.Log, ReclaimUnusedMetricPointsConfigKey, out this.ReclaimUnusedMetricPoints))
         {
             OpenTelemetrySdkEventSource.Log.MeterProviderSdkEvent("Reclaim unused metric point feature enabled via configuration.");

diff --git 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 emitOverflowAttribute,
         bool shouldReclaimUnusedMetricPoints,
         ExemplarFilterType? exemplarFilter = null,
         Func<ExemplarReservoir?>? exemplarReservoirFactory = null)
@@ -193,7 +192,6 @@ internal Metric(
             aggType,
             temporality,
             cardinalityLimit,
-            emitOverflowAttribute,
             shouldReclaimUnusedMetricPoints,
             exemplarFilter,
             exemplarReservoirFactory);

diff --git 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 emitOverflowAttribute;
     private bool reclaimUnusedMetricPoints;
     private ExemplarFilterType? exemplarFilter;
     private ExemplarFilterType? exemplarFilterForHistograms;
@@ -82,7 +81,6 @@ internal virtual List<Metric> AddMetricWithNoViews(Instrument instrument)
                         metricStreamIdentity,
                         this.GetAggregationTemporality(metricStreamIdentity.InstrumentType),
                         this.cardinalityLimit,
-                        this.emitOverflowAttribute,
                         this.reclaimUnusedMetricPoints,
                         exemplarFilter);
                 }
@@ -164,7 +162,6 @@ internal virtual List<Metric> AddMetricWithViews(Instrument instrument, List<Met
                         metricStreamIdentity,
                         this.GetAggregationTemporality(metricStreamIdentity.InstrumentType),
                         metricStreamConfig?.CardinalityLimit ?? this.cardinalityLimit,
-                        this.emitOverflowAttribute,
                         this.reclaimUnusedMetricPoints,
                         exemplarFilter,
                         metricStreamConfig?.ExemplarReservoirFactory);
@@ -184,7 +181,6 @@ internal virtual List<Metric> AddMetricWithViews(Instrument instrument, List<Met
     internal void ApplyParentProviderSettings(
         int metricLimit,
         int cardinalityLimit,
-        bool emitOverflowAttribute,
         bool reclaimUnusedMetricPoints,
         ExemplarFilterType? exemplarFilter,
         ExemplarFilterType? exemplarFilterForHistograms)
@@ -193,7 +189,6 @@ internal void ApplyParentProviderSettings(
         this.metrics = new Metric[metricLimit];
         this.metricsCurrentBatch = new Metric[metricLimit];
         this.cardinalityLimit = cardinalityLimit;
-        this.emitOverflowAttribute = emitOverflowAttribute;
         this.reclaimUnusedMetricPoints = reclaimUnusedMetricPoints;
         this.exemplarFilter = exemplarFilter;
         this.exemplarFilterForHistograms = exemplarFilterForHistograms;

diff --git a/test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs b/test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs
@@ -16,16 +16,14 @@ public abstract class AggregatorTestsBase
     private static readonly ExplicitBucketHistogramConfiguration HistogramConfiguration = new() { Boundaries = Metric.DefaultHistogramBounds };
     private static readonly MetricStreamIdentity MetricStreamIdentity = new(Instrument, HistogramConfiguration);
 
-    private readonly bool emitOverflowAttribute;
     private readonly bool shouldReclaimUnusedMetricPoints;
     private readonly AggregatorStore aggregatorStore;
 
-    protected AggregatorTestsBase(bool emitOverflowAttribute, bool shouldReclaimUnusedMetricPoints)
+    protected AggregatorTestsBase(bool shouldReclaimUnusedMetricPoints)
     {
-        this.emitOverflowAttribute = emitOverflowAttribute;
         this.shouldReclaimUnusedMetricPoints = shouldReclaimUnusedMetricPoints;
 
-        this.aggregatorStore = new(MetricStreamIdentity, AggregationType.HistogramWithBuckets, AggregationTemporality.Cumulative, 1024, emitOverflowAttribute, this.shouldReclaimUnusedMetricPoints);
+        this.aggregatorStore = new(MetricStreamIdentity, AggregationType.HistogramWithBuckets, AggregationTemporality.Cumulative, 1024, this.shouldReclaimUnusedMetricPoints);
     }
 
     [Fact]
@@ -253,7 +251,6 @@ public void HistogramBucketsDefaultUpdatesForSecondsTest(string meterName, strin
             AggregationType.Histogram,
             AggregationTemporality.Cumulative,
             cardinalityLimit: 1024,
-            this.emitOverflowAttribute,
             this.shouldReclaimUnusedMetricPoints);
 
         KnownHistogramBuckets actualHistogramBounds = KnownHistogramBuckets.Default;
@@ -330,7 +327,6 @@ internal void ExponentialHistogramTests(AggregationType aggregationType, Aggrega
             aggregationType,
             aggregationTemporality,
             cardinalityLimit: 1024,
-            this.emitOverflowAttribute,
             this.shouldReclaimUnusedMetricPoints,
             exemplarsEnabled ? ExemplarFilterType.AlwaysOn : null);
 
@@ -440,7 +436,6 @@ internal void ExponentialMaxScaleConfigWorks(int? maxScale)
             AggregationType.Base2ExponentialHistogram,
             AggregationTemporality.Cumulative,
             cardinalityLimit: 1024,
-            this.emitOverflowAttribute,
             this.shouldReclaimUnusedMetricPoints);
 
         aggregatorStore.Update(10, Array.Empty<KeyValuePair<string, object?>>());
@@ -525,31 +520,15 @@ public ThreadArguments(MetricPoint histogramPoint, ManualResetEvent mreToEnsureA
 public class AggregatorTests : AggregatorTestsBase
 {
     public AggregatorTests()
-        : base(emitOverflowAttribute: false, shouldReclaimUnusedMetricPoints: false)
-    {
-    }
-}
-
-public class AggregatorTestsWithOverflowAttribute : AggregatorTestsBase
-{
-    public AggregatorTestsWithOverflowAttribute()
-        : base(emitOverflowAttribute: true, shouldReclaimUnusedMetricPoints: false)
+        : base(shouldReclaimUnusedMetricPoints: false)
     {
     }
 }
 
 public class AggregatorTestsWithReclaimAttribute : AggregatorTestsBase
 {
     public AggregatorTestsWithReclaimAttribute()
-        : base(emitOverflowAttribute: false, shouldReclaimUnusedMetricPoints: true)
-    {
-    }
-}
-
-public class AggregatorTestsWithBothReclaimAndOverflowAttributes : AggregatorTestsBase
-{
-    public AggregatorTestsWithBothReclaimAndOverflowAttributes()
-        : base(emitOverflowAttribute: true, shouldReclaimUnusedMetricPoints: true)
+        : base(shouldReclaimUnusedMetricPoints: true)
     {
     }
 }