sdk/metric: Support specifying cardinality limits per instrument kinds

[petern48]

Previously, we only had WithCardinalityLimit(), which adds a global cardinality limit.

This PR adds a new API on the reader WithCardinalityLimitSelector that can be used to specify limits per instrument kinds.

spec

schema

closes #7786

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.7%. Comparing base (e604da5) to head (313de28).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
sdk/metric/reader.go	70.0%	3 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #7855   +/-   ##
=====================================
  Coverage   81.7%   81.7%           
=====================================
  Files        308     308           
  Lines      23632   23655   +23     
=====================================
+ Hits       19320   19343   +23     
- Misses      3928    3929    +1     
+ Partials     384     383    -1     

Files with missing lines	Coverage Δ
sdk/metric/config.go	95.1% <ø> (ø)
sdk/metric/manual_reader.go	83.8% <100.0%> (+0.7%)	⬆️
sdk/metric/periodic_reader.go	87.0% <100.0%> (+0.2%)	⬆️
sdk/metric/pipeline.go	90.2% <100.0%> (+0.1%)	⬆️
sdk/metric/reader.go	93.6% <70.0%> (-6.4%)	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dashpole]

This does not follow what is in the specification.

If there is no matching view, but the MetricReader defines a default
cardinality limit value based on the instrument an aggregation is created
for, that value SHOULD be used.

This does not suggest that the metric reader needs configuration allowing the cardinality limit to be changed per-instrument-kind. It only suggests that the default value chosen by the SDK can be based on the instrument.

The correct specification for this configuration is here

aggregation_cardinality_limit: A positive integer value defining the
maximum number of data points allowed to be emitted in a collection cycle by
a single instrument. See cardinality limits, below.

Users can provide an aggregation_cardinality_limit, but it is up to their
discretion. Therefore, the stream configuration parameter needs to be
structured to accept an aggregation_cardinality_limit, but MUST NOT
obligate a user to provide one. If the user does not provide an
aggregation_cardinality_limit value, the MeterProvider MUST apply the
default aggregation cardinality limit the MetricReader is
configured with.

[dashpole]

I've opened open-telemetry/opentelemetry-configuration#524 to fix this in the configuration.

[petern48]

Got it, thanks. I largely just blindly followed the issue, but I definitely also felt it was strange based on the spec. Left a comment on the issue you created, we'll see where it goes, considering other sdks have implemented it.

[dashpole]

Ok, so this needs to be configuration on the metric reader, not the MeterProvider.

See https://github.com/open-telemetry/opentelemetry-specification/blame/49845849d2d8df07059f82033f39e96c561927cf/specification/metrics/sdk.md#L1282.

• The default aggregation cardinality limit (optional) to use, a function of instrument kind. If not configured, a default value of 2000 SHOULD be used.

So this should be a function that takes an instrument kind, and returns a cardinality limit.

[petern48]

gentle bump @dashpole. Could you review again?

[petern48]

Three approvals already. Are we waiting on anything else before merging?