Skip to content

Conversation

@CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Feb 7, 2024

Follow-up to #5312

Changes

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes

@CodeBlanch CodeBlanch added pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package metrics Metrics signal related labels Feb 7, 2024
@CodeBlanch CodeBlanch requested a review from a team February 7, 2024 21:16
@reyang
Copy link
Member

reyang commented Feb 7, 2024

  • Expose MeterProviderBuilderExtensions.SetCardinalityLimit experimental API (does the same thing as SetMaxMetricPointsPerMetricStream just with a name that will match the spec concept).

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

CodeBlanch and others added 2 commits February 7, 2024 13:53
@CodeBlanch
Copy link
Member Author

CodeBlanch commented Feb 7, 2024

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

My thinking is we already exposed it with SetMaxMetricPointsPerMetricStream so we might as well name it something familiar/relatable/searchable to what the spec does have.

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.

@CodeBlanch CodeBlanch changed the title [sdk-metrics] Standarize on "Cardinality Limit" name [sdk-metrics] Obsolete SetMaxMetricPointsPerMetricStream + standarize on "Cardinality Limit" name Feb 7, 2024
@CodeBlanch
Copy link
Member Author

I had an offline discussion with @reyang. We decided to obsolete SetMaxMetricPointsPerMetricStream and NOT add an alternative global mechanism. Because it isn't part of the spec and it may be too dangerous to have.

@Yun-Ting
Copy link
Contributor

Yun-Ting commented Feb 7, 2024

I feel .SetCardinalityLimit() is a bit confusing to use:

  1. The current implementation of this method is setting a "global" value for the dimensions of each metric of a MeterProvider.
  2. If user really cares about customizing each metric, they should be encouraged to leverage view to achieve that. And the view already has the API to set the cardinality limit.

@Yun-Ting
Copy link
Contributor

Yun-Ting commented Feb 8, 2024

CardinalityLimitofMatchingViewTakesPrecedenceOverMetricProviderWhenBothWereSet can be removed.
Can update to test this scenario instead: #5328 (comment)

@CodeBlanch CodeBlanch merged commit cf00e42 into open-telemetry:main Feb 9, 2024
@CodeBlanch CodeBlanch deleted the sdk-cardinalitylimit-followup branch February 9, 2024 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Documentation related metrics Metrics signal related pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants