[sdk-metrics] Obsolete SetMaxMetricPointsPerMetricStream + standarize on "Cardinality Limit" name #5328
Conversation
CodeBlanch
added
pkg:OpenTelemetry
src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
Do we want to expose this at all or we want to point folks to View level limit? Asking this because the spec doesn't ask for it https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#configuration-1 |
…ns.cs Co-authored-by: Yun-Ting Lin <yunl@microsoft.com>
My thinking is we already exposed it with If you are thinking we should just do something like... #if EXPOSE_EXPERIMENTAL_FEATURES
[Obsolete("Use MetricStreamConfiguration.CardinalityLimit instead. This method will be removed in a future version.")]
#endif
public static MeterProviderBuilder SetMaxMetricPointsPerMetricStream(this MeterProviderBuilder meterProviderBuilder, int maxMetricPointsPerMetricStream)I'm also good with that. |
src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
CodeBlanch
changed the title
|
I had an offline discussion with @reyang. We decided to obsolete |
|
I feel
|
|
CardinalityLimitofMatchingViewTakesPrecedenceOverMetricProviderWhenBothWereSet can be removed. |
| // There are four distinct key/value combinations emitted for `MyFruitCounter`: | ||
| // 1. No key/value pair | ||
| // 2. (name:apple, color:red) | ||
| // 3. (name:lemon, color:yellow) | ||
| // 4. (name:apple, color:green) | ||
|
|
||
| // Since the maximum number of `MetricPoint`s allowed is `3`, the SDK will only export measurements for the following three combinations: | ||
| // 1. No key/value pair | ||
| // 2. (name:apple, color:red) | ||
| // 3. (name:lemon, color:yellow) | ||
|
|
||
| MyFruitCounter.Add(1); // Exported (No key/value pair) | ||
| MyFruitCounter.Add(1, new("name", "apple"), new("color", "red")); // Exported | ||
| MyFruitCounter.Add(2, new("name", "lemon"), new("color", "yellow")); // Exported | ||
| MyFruitCounter.Add(1, new("name", "lemon"), new("color", "yellow")); // Exported | ||
| MyFruitCounter.Add(2, new("name", "apple"), new("color", "green")); // Not exported | ||
| MyFruitCounter.Add(5, new("name", "apple"), new("color", "red")); // Exported | ||
| MyFruitCounter.Add(4, new("name", "lemon"), new("color", "yellow")); // Exported | ||
|
|
||
| // There are four distinct key/value combinations emitted for `AnotherFruitCounter`: | ||
| // 1. (name:kiwi) | ||
| // 2. (name:banana, color:yellow) | ||
| // 3. (name:mango, color:yellow) | ||
| // 4. (name:banana, color:green) | ||
|
|
||
| // Since the maximum number of `MetricPoint`s allowed is `3`, the SDK will only export measurements for the following three combinations: | ||
| // 1. No key/value pair (This is a special case. The SDK reserves a `MetricPoint` for it even if it's not explicitly emitted.) | ||
| // 2. (name:kiwi) | ||
| // 3. (name:banana, color:yellow) | ||
|
|
||
| AnotherFruitCounter.Add(4, new KeyValuePair<string, object>("name", "kiwi")); // Exported | ||
| AnotherFruitCounter.Add(1, new("name", "banana"), new("color", "yellow")); // Exported | ||
| AnotherFruitCounter.Add(2, new("name", "mango"), new("color", "yellow")); // Not exported | ||
| AnotherFruitCounter.Add(1, new("name", "mango"), new("color", "yellow")); // Not exported | ||
| AnotherFruitCounter.Add(2, new("name", "banana"), new("color", "green")); // Not exported | ||
| AnotherFruitCounter.Add(5, new("name", "banana"), new("color", "yellow")); // Exported | ||
| AnotherFruitCounter.Add(4, new("name", "mango"), new("color", "yellow")); // Not exported |
There was a problem hiding this comment.
@reyang I might have been too aggressive removing all of this. Do you still want it in there just showing the view instead of the global mechanism?
There was a problem hiding this comment.
No strong opinion here. @utpilla might have different ideas.
There was a problem hiding this comment.
@CodeBlanch We should keep the example here. People have found it useful to understand how configuring the cardinality would affect the export.
| @@ -238,6 +238,9 @@ public static MeterProviderBuilder SetMaxMetricStreams(this MeterProviderBuilder | |||
| /// <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("Use MetricStreamConfiguration.CardinalityLimit via the AddView API instead. This method will be removed in a future version.")] | |||
There was a problem hiding this comment.
| [Obsolete("Use MetricStreamConfiguration.CardinalityLimit via the AddView API instead. This method will be removed in a future version.")] | |
| [Obsolete("Use MetricStreamConfiguration.CardinalityLimit via the AddView API instead. This method will be removed in the next major version bump.")] |
There was a problem hiding this comment.
Or combine.. removed in a future version, along with major version bump of the package.
There was a problem hiding this comment.
I'm not going to do this here because we use similar a bunch of places but good thing for a follow-up!
Follow-up to #5312
Changes
CardinalityLimitallowed for the metric managed by the view. #5312 where a view specifyingCardinalityLimitactually changes the value stored on theMetricReaderwhich will then apply to any metrics recorded after the one which had the view apply: f1b62b0MeterProviderBuilderExtensions.SetMaxMetricPointsPerMetricStreamin favor of the View API added in [sdk] Added support for settingCardinalityLimitallowed for the metric managed by the view. #5312.MeterProvidertoMetricReader.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes