Code review.

docs/diagnostics/experimental-apis/OTEL1003.md

@@ -4,7 +4,6 @@
 
 This is an Experimental API diagnostic covering the following API:
 
-* `MeterProviderBuilderExtensions.SetCardinalityLimit`
 * `MetricStreamConfiguration.CardinalityLimit.get`
 * `MetricStreamConfiguration.CardinalityLimit.set`
 
@@ -25,17 +24,6 @@ From the specification:
 We are exposing these APIs experimentally until the specification declares them
 stable.
 
-### Setting the default cardinality limit for the MeterProvider
-
-There is nothing in the OpenTelemetry Specification currently for defining the
-default cardinality limit for a MeterProvider.
-
-```csharp
-using var meterProvider = Sdk.CreateMeterProviderBuilder()
-    .SetCardinalityLimit(5000)
-    .Build();
-```
-
 ### Setting cardinality limit for a specific Metric via the View API
 
 The OpenTelemetry Specification defines the [cardinality

src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt

@@ -31,7 +31,6 @@ static OpenTelemetry.Logs.LoggerProviderBuilderExtensions.SetResourceBuilder(thi
 static OpenTelemetry.Logs.LoggerProviderExtensions.AddProcessor(this OpenTelemetry.Logs.LoggerProvider! provider, OpenTelemetry.BaseProcessor<OpenTelemetry.Logs.LogRecord!>! processor) -> OpenTelemetry.Logs.LoggerProvider!
 static OpenTelemetry.Logs.LoggerProviderExtensions.ForceFlush(this OpenTelemetry.Logs.LoggerProvider! provider, int timeoutMilliseconds = -1) -> bool
 static OpenTelemetry.Logs.LoggerProviderExtensions.Shutdown(this OpenTelemetry.Logs.LoggerProvider! provider, int timeoutMilliseconds = -1) -> bool
-static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.SetCardinalityLimit(this OpenTelemetry.Metrics.MeterProviderBuilder! meterProviderBuilder, int cardinalityLimit) -> OpenTelemetry.Metrics.MeterProviderBuilder!
 static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.SetExemplarFilter(this OpenTelemetry.Metrics.MeterProviderBuilder! meterProviderBuilder, OpenTelemetry.Metrics.ExemplarFilter! exemplarFilter) -> OpenTelemetry.Metrics.MeterProviderBuilder!
 static OpenTelemetry.OpenTelemetryBuilderSdkExtensions.WithLogging(this OpenTelemetry.IOpenTelemetryBuilder! builder) -> OpenTelemetry.IOpenTelemetryBuilder!
 static OpenTelemetry.OpenTelemetryBuilderSdkExtensions.WithLogging(this OpenTelemetry.IOpenTelemetryBuilder! builder, System.Action<OpenTelemetry.Logs.LoggerProviderBuilder!>! configure) -> OpenTelemetry.IOpenTelemetryBuilder!

src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderExtensions.cs

@@ -1,9 +1,6 @@
 // Copyright The OpenTelemetry Authors
 // SPDX-License-Identifier: Apache-2.0
 
-#if EXPOSE_EXPERIMENTAL_FEATURES
-using System.ComponentModel;
-#endif
 #if NET6_0_OR_GREATER
 using System.Diagnostics.CodeAnalysis;
 #endif
@@ -233,13 +230,16 @@ public static MeterProviderBuilder SetMaxMetricStreams(this MeterProviderBuilder
     /// This limits the number of unique combinations of key/value pairs used
     /// for reporting measurements.
     /// </summary>
-    /// <inheritdoc cref="SetCardinalityLimit(MeterProviderBuilder, int)"/>
+    /// <remarks>
+    /// If a particular key/value pair combination is used at least once,
+    /// it will contribute to the limit for the life of the process.
+    /// This may change in the future. See: https://github.com/open-telemetry/opentelemetry-dotnet/issues/2360.
+    /// </remarks>
     /// <param name="meterProviderBuilder"><see cref="MeterProviderBuilder"/>.</param>
     /// <param name="maxMetricPointsPerMetricStream">Maximum number of metric points allowed per metric stream.</param>
     /// <returns>The supplied <see cref="MeterProviderBuilder"/> for chaining.</returns>
 #if EXPOSE_EXPERIMENTAL_FEATURES
-    [Obsolete("Call SetCardinalityLimit instead. This method will be removed in a future version.")]
-    [EditorBrowsable(EditorBrowsableState.Never)]
+    [Obsolete("Use MetricStreamConfiguration.CardinalityLimit via the AddView API instead. This method will be removed in a future version.")]
 #endif
     public static MeterProviderBuilder SetMaxMetricPointsPerMetricStream(this MeterProviderBuilder meterProviderBuilder, int maxMetricPointsPerMetricStream)
     {
@@ -249,7 +249,7 @@ public static MeterProviderBuilder SetMaxMetricPointsPerMetricStream(this MeterP
         {
             if (builder is MeterProviderBuilderSdk meterProviderBuilderSdk)
             {
-                meterProviderBuilderSdk.SetMaxMetricPointsPerMetricStream(maxMetricPointsPerMetricStream);
+                meterProviderBuilderSdk.SetDefaultCardinalityLimit(maxMetricPointsPerMetricStream);
             }
         });
 
@@ -317,40 +317,6 @@ public static MeterProvider Build(this MeterProviderBuilder meterProviderBuilder
         throw new NotSupportedException($"Build is not supported on '{meterProviderBuilder?.GetType().FullName ?? "null"}' instances.");
     }
 
-#if EXPOSE_EXPERIMENTAL_FEATURES
-    /// <summary>
-    /// Sets a positive integer value defining the maximum number of
-    /// data points allowed for metrics managed by the MeterProvider.
-    /// </summary>
-    /// <remarks>
-    /// <para><b>WARNING</b>: This is an experimental API which might change or
-    /// be removed in the future. Use at your own risk.</para>
-    /// </remarks>
-    /// <param name="meterProviderBuilder"><see cref="MeterProviderBuilder"/>.</param>
-    /// <param name="cardinalityLimit">Cardinality limit.</param>
-    /// <returns>The supplied <see cref="MeterProviderBuilder"/> for chaining.</returns>
-#if NET8_0_OR_GREATER
-    [Experimental(DiagnosticDefinitions.CardinalityLimitExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)]
-#endif
-    public
-#else
-    internal
-#endif
-        static MeterProviderBuilder SetCardinalityLimit(this MeterProviderBuilder meterProviderBuilder, int cardinalityLimit)
-    {
-        Guard.ThrowIfOutOfRange(cardinalityLimit, min: 1, max: int.MaxValue);
-
-        meterProviderBuilder.ConfigureBuilder((sp, builder) =>
-        {
-            if (builder is MeterProviderBuilderSdk meterProviderBuilderSdk)
-            {
-                meterProviderBuilderSdk.SetCardinalityLimit(cardinalityLimit);
-            }
-        });
-
-        return meterProviderBuilder;
-    }
-
 #if EXPOSE_EXPERIMENTAL_FEATURES
     /// <summary>
     /// Sets the <see cref="ExemplarFilter"/> to be used for this provider.

src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderSdk.cs

@@ -15,8 +15,8 @@ namespace OpenTelemetry.Metrics;
 /// </summary>
 internal sealed class MeterProviderBuilderSdk : MeterProviderBuilder, IMeterProviderBuilder
 {
-    public const int MaxMetricsDefault = 1000;
-    public const int CardinalityLimitDefault = 2000;
+    public const int DefaultMaxMetricStreams = 1000;
+    public const int DefaultCardinalityLimit = 2000;
     private const string DefaultInstrumentationVersion = "1.0.0.0";
 
     private readonly IServiceProvider serviceProvider;
@@ -49,9 +49,9 @@ public MeterProviderBuilderSdk(IServiceProvider serviceProvider)
 
     public List<Func<Instrument, MetricStreamConfiguration?>> ViewConfigs { get; } = new();
 
-    public int MaxMetricStreams { get; private set; } = MaxMetricsDefault;
+    public int MaxMetricStreams { get; private set; } = DefaultMaxMetricStreams;
 
-    public int CardinalityLimit { get; private set; } = CardinalityLimitDefault;
+    public int CardinalityLimit { get; private set; } = DefaultCardinalityLimit;
 
     /// <summary>
     /// Returns whether the given instrument name is valid according to the specification.
@@ -193,7 +193,7 @@ public MeterProviderBuilder SetMaxMetricStreams(int maxMetricStreams)
         return this;
     }
 
-    public MeterProviderBuilder SetCardinalityLimit(int cardinalityLimit)
+    public MeterProviderBuilder SetDefaultCardinalityLimit(int cardinalityLimit)
     {
         this.CardinalityLimit = cardinalityLimit;
 

src/OpenTelemetry/Metrics/MetricStreamConfiguration.cs

@@ -109,10 +109,8 @@ public string[]? TagKeys
     /// <para>Spec reference: <see
     /// href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#cardinality-limits">Cardinality
     /// limits</see>.</para>
-    /// Note: If not set, the MeterProvider cardinality limit value will be
-    /// used, which defaults to 2000. Call <see
-    /// cref="MeterProviderBuilderExtensions.SetCardinalityLimit"/>
-    /// to configure the MeterProvider default.
+    /// Note: If not set the default MeterProvider cardinality limit of 2000
+    /// will apply.
     /// </remarks>
 #if NET8_0_OR_GREATER
     [Experimental(DiagnosticDefinitions.CardinalityLimitExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)]

test/OpenTelemetry.Tests/Metrics/MetricApiTestsBase.cs

@@ -1423,26 +1423,26 @@ int MetricPointCount()
         // for no tag point!
         // This may be changed later.
         counterLong.Add(10);
-        for (int i = 0; i < MeterProviderBuilderSdk.CardinalityLimitDefault + 1; i++)
+        for (int i = 0; i < MeterProviderBuilderSdk.DefaultCardinalityLimit + 1; i++)
         {
             counterLong.Add(10, new KeyValuePair<string, object>("key", "value" + i));
         }
 
         meterProvider.ForceFlush(MaxTimeToAllowForFlush);
-        Assert.Equal(MeterProviderBuilderSdk.CardinalityLimitDefault, MetricPointCount());
+        Assert.Equal(MeterProviderBuilderSdk.DefaultCardinalityLimit, MetricPointCount());
 
         exportedItems.Clear();
         counterLong.Add(10);
-        for (int i = 0; i < MeterProviderBuilderSdk.CardinalityLimitDefault + 1; i++)
+        for (int i = 0; i < MeterProviderBuilderSdk.DefaultCardinalityLimit + 1; i++)
         {
             counterLong.Add(10, new KeyValuePair<string, object>("key", "value" + i));
         }
 
         meterProvider.ForceFlush(MaxTimeToAllowForFlush);
-        Assert.Equal(MeterProviderBuilderSdk.CardinalityLimitDefault, MetricPointCount());
+        Assert.Equal(MeterProviderBuilderSdk.DefaultCardinalityLimit, MetricPointCount());
 
         counterLong.Add(10);
-        for (int i = 0; i < MeterProviderBuilderSdk.CardinalityLimitDefault + 1; i++)
+        for (int i = 0; i < MeterProviderBuilderSdk.DefaultCardinalityLimit + 1; i++)
         {
             counterLong.Add(10, new KeyValuePair<string, object>("key", "value" + i));
         }
@@ -1453,7 +1453,7 @@ int MetricPointCount()
         counterLong.Add(10, new KeyValuePair<string, object>("key", "valueC"));
         exportedItems.Clear();
         meterProvider.ForceFlush(MaxTimeToAllowForFlush);
-        Assert.Equal(MeterProviderBuilderSdk.CardinalityLimitDefault, MetricPointCount());
+        Assert.Equal(MeterProviderBuilderSdk.DefaultCardinalityLimit, MetricPointCount());
     }
 
     [Fact]

test/OpenTelemetry.Tests/Metrics/MetricOverflowAttributeTestsBase.cs

@@ -174,7 +174,7 @@ public void MetricOverflowAttributeIsRecordedCorrectlyForCounter(MetricReaderTem
         counter.Add(10); // Record measurement for zero tags
 
         // Max number for MetricPoints available for use when emitted with tags
-        int maxMetricPointsForUse = MeterProviderBuilderSdk.CardinalityLimitDefault - 2;
+        int maxMetricPointsForUse = MeterProviderBuilderSdk.DefaultCardinalityLimit - 2;
 
         for (int i = 0; i < maxMetricPointsForUse; i++)
         {
@@ -325,7 +325,7 @@ public void MetricOverflowAttributeIsRecordedCorrectlyForHistogram(MetricReaderT
         histogram.Record(10); // Record measurement for zero tags
 
         // Max number for MetricPoints available for use when emitted with tags
-        int maxMetricPointsForUse = MeterProviderBuilderSdk.CardinalityLimitDefault - 2;
+        int maxMetricPointsForUse = MeterProviderBuilderSdk.DefaultCardinalityLimit - 2;
 
         for (int i = 0; i < maxMetricPointsForUse; i++)
         {

test/OpenTelemetry.Tests/Metrics/MetricPointReclaimTestsBase.cs

@@ -316,7 +316,7 @@ public override ExportResult Export(in Batch<Metric> batch)
                 }
 
                 // This is to ensure that the lookup dictionary does not have unbounded growth
-                Assert.True(metricPointLookupDictionary.Count <= (MeterProviderBuilderSdk.CardinalityLimitDefault * 2));
+                Assert.True(metricPointLookupDictionary.Count <= (MeterProviderBuilderSdk.DefaultCardinalityLimit * 2));
 
                 foreach (ref readonly var metricPoint in metric.GetMetricPoints())
                 {

test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs

@@ -928,7 +928,7 @@ public void CardinalityLimitofMatchingViewTakesPrecedenceOverMetricProviderWhenB
 
         using var container = this.BuildMeterProvider(out var meterProvider, builder => builder
             .AddMeter(meter.Name)
-            .SetCardinalityLimit(3)
+            .SetMaxMetricPointsPerMetricStream(3)
             .AddView((instrument) =>
             {
                 return new MetricStreamConfiguration() { Name = "MetricStreamA", CardinalityLimit = 10000 };