Cardinality limits are not configurable per-instrument-kind

[dashpole]

I'm opening this after reviewing open-telemetry/opentelemetry-go#7855.

The schema appears to currently support configuring cardinality limits differently based on the instrument kind. This is not part of the metrics SDK specification, and should be removed.

opentelemetry-configuration/snippets/CardinalityLimits_kitchen_sink.yaml

Lines 11 to 18 in 3dbebe2

	default: 100
	counter: 2000
	gauge: 2000
	histogram: 2000
	observable_counter: 2000
	observable_gauge: 2000
	observable_up_down_counter: 2000
	up_down_counter: 2000

The PR above referenced this section as the spec as justification:

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.

But 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.

[petern48]

Did a bit of investigating. Looks like it was introduced here #185 based on that same spec. I found there are more places to fix than just that .yaml.

opentelemetry-configuration/schema-docs.md

Lines 665 to 672 in a0f3b83

	## CardinalityLimits <a id="cardinalitylimits"></a>
	| Property | Type | Required? | Default and Null Behavior | Constraints | Description |
	|---|---|---|---|---|---|
	| `counter` | one of:<br>* `integer`<br>* `null`<br> | `false` | If omitted or null, the value from .default is used. | * `exclusiveMinimum`: `0`<br> | Configure default cardinality limit for counter instruments.<br> |
	| `default` | one of:<br>* `integer`<br>* `null`<br> | `false` | If omitted or null, 2000 is used. | * `exclusiveMinimum`: `0`<br> | Configure default cardinality limit for all instrument types.<br>Instrument-specific cardinality limits take priority.<br> |
	| `gauge` | one of:<br>* `integer`<br>* `null`<br> | `false` | If omitted or null, the value from .default is used. | * `exclusiveMinimum`: `0`<br> | Configure default cardinality limit for gauge instruments.<br> |
	| `histogram` | one of:<br>* `integer`<br>* `null`<br> | `false` | If omitted or null, the value from .default is used. | * `exclusiveMinimum`: `0`<br> | Configure default cardinality limit for histogram instruments.<br> |

It looks like this has been implemented in the Java SDK here and also (very recently) in C++: open-telemetry/opentelemetry-cpp#3820

cc @jack-berg @marcalff (for your knowledge and in case you want to chime in)

[jack-berg]

@petern48 that document is generated based on the YAML schema, so a change to the schema would automatically update it.

@dashpole have you seen 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.

[dashpole]

@jack-berg ack. That makes more sense. I saw the one on the meterprovider and on view, but not that one.