diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md index 8eb169cddc..cb9a9e7921 100644 --- a/opentelemetry-sdk/CHANGELOG.md +++ b/opentelemetry-sdk/CHANGELOG.md @@ -47,7 +47,8 @@ - **Breaking** Moved the following SDK sampling types from `opentelemetry::trace` to `opentelemetry_sdk::trace` [#3277][3277]: - `SamplingDecision`, `SamplingResult` - 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. - Fix Histogram boundaries being ignored in the presence of views [#3312][3312] - `TracerProviderBuilder::with_sampler` allows to pass boxed instance of `ShouldSample` [#3313][3313] - Fix ObservableCounter and ObservableUpDownCounter now correctly report only data points from the current measurement cycle, removing stale attribute combinations that are no longer observed. [#3248][3248] @@ -58,7 +59,6 @@ [3227]: https://github.com/open-telemetry/opentelemetry-rust/pull/3227 [3277]: https://github.com/open-telemetry/opentelemetry-rust/pull/3277 -[3290]: https://github.com/open-telemetry/opentelemetry-rust/pull/3290 [3312]: https://github.com/open-telemetry/opentelemetry-rust/pull/3312 [3248]: https://github.com/open-telemetry/opentelemetry-rust/pull/3248 [3262]: https://github.com/open-telemetry/opentelemetry-rust/pull/3262 diff --git a/opentelemetry-sdk/src/metrics/instrument.rs b/opentelemetry-sdk/src/metrics/instrument.rs index 54a10722b3..00788fea62 100644 --- a/opentelemetry-sdk/src/metrics/instrument.rs +++ b/opentelemetry-sdk/src/metrics/instrument.rs @@ -259,6 +259,11 @@ impl StreamBuilder { if limit == 0 { return Err("Cardinality limit must be greater than 0".into()); } + // Reject usize::MAX because the SDK's internal HashMap capacity + // is sized as `1 + cardinality_limit`, which would overflow. + if limit == usize::MAX { + return Err("Cardinality limit must be less than usize::MAX".into()); + } } // Validate bucket boundaries if using ExplicitBucketHistogram @@ -544,8 +549,24 @@ mod tests { "Expected cardinality limit validation error message" ); + // Test usize::MAX (invalid — would overflow internal `1 + limit` capacity) + let builder = StreamBuilder::new() + .with_name("valid_name") + .with_cardinality_limit(usize::MAX); + + let result = builder.build(); + assert!( + result.is_err(), + "Expected error for usize::MAX cardinality limit" + ); + assert_eq!( + result.err().unwrap().to_string(), + "Cardinality limit must be less than usize::MAX", + "Expected cardinality limit usize::MAX error message" + ); + // Test valid cardinality limits - let valid_limits = vec![1, 10, 100, 1000]; + let valid_limits = vec![1, 10, 100, 1000, usize::MAX - 1]; for limit in valid_limits { let builder = StreamBuilder::new() .with_name("valid_name") diff --git a/opentelemetry-sdk/src/metrics/internal/mod.rs b/opentelemetry-sdk/src/metrics/internal/mod.rs index 5314bb36c1..6bb4900fdc 100644 --- a/opentelemetry-sdk/src/metrics/internal/mod.rs +++ b/opentelemetry-sdk/src/metrics/internal/mod.rs @@ -8,7 +8,6 @@ mod sum; use core::fmt; #[cfg(not(target_has_atomic = "64"))] use portable_atomic::{AtomicI64, AtomicU64}; -use std::cmp::min; use std::collections::{HashMap, HashSet}; use std::ops::{Add, AddAssign, Sub}; use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; @@ -25,7 +24,6 @@ use opentelemetry::otel_debug; use opentelemetry::{otel_warn, KeyValue}; use super::data::{AggregatedMetrics, MetricData}; -use super::pipeline::DEFAULT_CARDINALITY_LIMIT; // TODO Replace it with LazyLock once it is stable pub(crate) static STREAM_OVERFLOW_ATTRIBUTES: OnceLock> = OnceLock::new(); @@ -113,9 +111,7 @@ where fn new(config: A::InitConfig, cardinality_limit: usize) -> Self { ValueMap { - trackers: RwLock::new(HashMap::with_capacity( - 1 + min(DEFAULT_CARDINALITY_LIMIT, cardinality_limit), - )), + trackers: RwLock::new(HashMap::with_capacity(1 + cardinality_limit)), no_attribute_tracker: Arc::new(TrackerEntry::new(&config)), count: AtomicUsize::new(0), config, @@ -833,14 +829,6 @@ mod tests { assert!(f64::abs(0.0 - value2) < 0.0001, "Incorrect second value"); } - #[test] - fn large_cardinality_limit() { - // This is a regression test for panics that used to occur for large cardinality limits - - // Should not panic - let _value_map = ValueMap::>::new((), usize::MAX); - } - #[test] fn stale_entry_evicts_both_unsorted_and_sorted_keys() { // ValueMap stores two HashMap keys per attribute set: one for the insertion diff --git a/opentelemetry-sdk/src/metrics/pipeline.rs b/opentelemetry-sdk/src/metrics/pipeline.rs index cf08027462..b8e8ebdd3a 100644 --- a/opentelemetry-sdk/src/metrics/pipeline.rs +++ b/opentelemetry-sdk/src/metrics/pipeline.rs @@ -50,7 +50,7 @@ impl fmt::Debug for Pipeline { /// Single or multi-instrument callbacks type GenericCallback = Arc; -pub(crate) const DEFAULT_CARDINALITY_LIMIT: usize = 2000; +const DEFAULT_CARDINALITY_LIMIT: usize = 2000; #[derive(Default)] struct PipelineInner {