[Feature]: STREAM_CARDINALITY_LIMIT should be config

[domyway]

Related Problems?

No response

Describe the solution you'd like:

I got some warning log like :
OpenTelemetry metrics error occurred. Metrics error: Warning: Maximum data points for metric stream exceeded. Entry added to overflow.

I use histogram and it has many labels , I find the log code here:

pub(crate) fn is_under_cardinality_limit(size: usize) -> bool {
    size < STREAM_CARDINALITY_LIMIT as usize - 1
}

it should be config

Considered Alternatives

No response

Additional Context

No response

[cijothomas]

Thanks for reporting this. We are aware of this #1065 can be used for tracking.
(This is a p0 before stable, and I expect to land this in upcoming release ~ 2 weeks out)

[domyway]

Thanks for reporting this. We are aware of this #1065 can be used for tracking. (This is a p0 before stable, and I expect to land this in upcoming release ~ 2 weeks out)

HI, How is it going~~

[cijothomas]

Thanks for reporting this. We are aware of this #1065 can be used for tracking. (This is a p0 before stable, and I expect to land this in upcoming release ~ 2 weeks out)

HI, How is it going~~

Working through a major refactoring of the Metrics sdk! A lot has been done to fix perf issues, but not yet reached the stage where we have addressed this issue. Though it is delayed, it is still the top feature that must be added in my mind!

[cijothomas]

Update:
The spec is still experimental (open-telemetry/opentelemetry-specification#3904), and no sign of moving it to stable in next few weeks.

I think its best to do the following for OTel Rust:

1. Move the cardinality-capping under opt-in feature flag. i.e By default, there won't be cardinality capping.
2. If opted-in, provide ability to customize the default cardinality on a MetricReader level. (i.e not on per metric level using Views.) This is because we are not 100% sure if Views can be part of 1st stable release or not.
3. Once the spec is stable, remove feature-flag, and make this the default.
4. At this point, provide ability to customize per metric.

If all agree to this, we can make this happen for 0.26 release (~<2 weeks)

[utpilla]

Update: The spec is still experimental (open-telemetry/opentelemetry-specification#3904), and no sign of moving it to stable in next few weeks.

I think its best to do the following for OTel Rust:

1. Move the cardinality-capping under opt-in feature flag. i.e By default, there won't be cardinality capping.
2. If opted-in, provide ability to customize the default cardinality on a MetricReader level. (i.e not on per metric level using Views.) This is because we are not 100% sure if Views can be part of 1st stable release or not.
3. Once the spec is stable, remove feature-flag, and make this the default.
4. At this point, provide ability to customize per metric.

If all agree to this, we can make this happen for 0.26 release (~<2 weeks)

That sounds reasonable.

[cijothomas]

There is some movement in spec, so will check the spec status and re-update here:
open-telemetry/opentelemetry-specification#4222

[cijothomas]

There is some movement in spec, so will check the spec status and re-update here: open-telemetry/opentelemetry-specification#4222

The spec is stable, I change my position about this. Here's the new proposal

1. Do this by default (no feature flags required as the spec is already stable)
2. Keep the configuration at Reader level.
3. Do not offer View based config to control cardinality per metric in the initial release, consider Views feature itself might not be part of 1.0
4. Add 3 post 1.0.

[cijothomas]

1,2,4 will be added in 0.27.1 milestone. Its additive change only, and we are about to declare the Metrics API as RC tomorow, so this got lower priority.
0.27.1 is coming in < 2 weeks, so not that far out.

[frittentheke]

The now stable spec (https://github.com/open-telemetry/opentelemetry-specification/blob/v1.39.0/specification/metrics/sdk.md#cardinality-limits) now says that:

SDKs SHOULD support being configured with a cardinality limit.

I totally understand that this is already accepted as a feature in general and tracked also via #1065.

But @cijothomas since you removed the target milestone, could you kindly give a update if the capability to configure the cardinality limit would be targeting milestone 0.28 then or when it could be expected?

[cijothomas]

@frittentheke Thanks for calling this out. Yes this is a miss from my side. I just added it to Metric SDK stable milestone, but hopefully we can do it in 0.28 itself.

This is not only an important gap, but a blocker from going stable.

[cijothomas]

Moving to 0.28, the plan is to remove the cardinality cap feature completely now, and add it back when we have View feature also stable.

[timbotnik]

@cijothomas do you have a sense of the performance implications of removing this? One worry is that it would result in higher RAM requirements for those who would have hit the limit previously.

[cijothomas]

@cijothomas do you have a sense of the performance implications of removing this? One worry is that it would result in higher RAM requirements for those who would have hit the limit previously.

Yes that is true. Without this feature in the SDK, there is practically no limit to the amount of memory Metric SDK can take up.
If user does something like counter.Add(1, KeyValue::new("key", UUID::new())); repeatedly, the app would get killed due to OOM at some point. (Most likely almost all other languages are operating this way, as the spec around cardinality cap is <1 year old.)

This is a critical capability to be added back before 1.0 stable. Without the ability to configure the limit, it severely limits the usability also as 2000 default is not sufficient for many use cases; hence removed this for 0.28.