fix(metrics): reject usize::MAX as cardinality limit#3506
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3506 +/- ##
=====================================
Coverage 82.8% 82.8%
=====================================
Files 130 130
Lines 27276 27282 +6
=====================================
+ Hits 22609 22614 +5
- Misses 4667 4668 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| - These types are SDK implementation details and should be imported from `opentelemetry_sdk::trace` instead. | ||
| - Fix panics and exploding memory usage from large cardinality limit [#3290][3290] | ||
| - `StreamBuilder::build()` now rejects `usize::MAX` as a cardinality limit | ||
| with a validation error. |
3ddb386
|
|
||
| // Test valid cardinality limits | ||
| let valid_limits = vec![1, 10, 100, 1000]; | ||
| let valid_limits = vec![1, 10, 100, 1000, usize::MAX - 1]; |
There was a problem hiding this comment.
Marking usize::MAX - 1 as valid here is misleading. It avoids the 1 + cardinality_limit overflow, but ValueMap::new() then calls HashMap::with_capacity(usize::MAX), which still fails at allocation. The PR fixes the arithmetic-overflow case correctly, but any sufficiently large limit will still fail loudly when the stream is first used.
Should we either tighten the validation to a sane upper bound, or decouple the initial HashMap capacity from the configured cardinality limit so a high limit doesn't force a huge upfront allocation?
There was a problem hiding this comment.
Exactly this. The fix that was reverted was not centered around the integer overflow. The fix was to avoid the huge (and wasteful) up-front allocation. By reverting my PR, a very real bug is being re-introduced.
There was a problem hiding this comment.
The discussion about disabling cardinality limit (modifying upfront allocation logic) is being tracked in this #3507. It would be worked upon after the 0.32 release.
4 participants
Reverts #3290 and instead rejects
usize::MAXinStreamBuilder::build(). The original integer-overflow panic is fixed at the input boundary, keeping the SDK's no-allocation-on-hot-path design intact.