From c06c04bb127baaacd9aa4cee328107c078a7d2c3 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 15 Nov 2023 06:58:31 -0800 Subject: [PATCH] Remove API for Creating Histograms with signed integers. (#1371) As per the OTel [specs], the value to be recorded with histogram instrument SHOULD be non-negative. Removing the existing method to record signed integer values. > The value is expected to be non-negative. This API SHOULD be documented in a way to communicate to users that > this value is expected to be non-negative. This API SHOULD NOT validate this value, that is left to implementations > of the API. [specs]: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#histogram --- opentelemetry-prometheus/src/lib.rs | 2 +- .../tests/integration_test.rs | 14 +++++------ opentelemetry-sdk/benches/metric.rs | 8 +++---- opentelemetry-sdk/src/metrics/meter.rs | 24 ------------------- opentelemetry/CHANGELOG.md | 3 ++- .../src/metrics/instruments/histogram.rs | 12 ---------- opentelemetry/src/metrics/meter.rs | 21 ---------------- opentelemetry/src/metrics/mod.rs | 10 -------- 8 files changed, 14 insertions(+), 80 deletions(-) diff --git a/opentelemetry-prometheus/src/lib.rs b/opentelemetry-prometheus/src/lib.rs index 804f69dea6..bb637f52b2 100644 --- a/opentelemetry-prometheus/src/lib.rs +++ b/opentelemetry-prometheus/src/lib.rs @@ -27,7 +27,7 @@ //! .with_description("Counts things") //! .init(); //! let histogram = meter -//! .i64_histogram("a.histogram") +//! .u64_histogram("a.histogram") //! .with_description("Records values") //! .init(); //! diff --git a/opentelemetry-prometheus/tests/integration_test.rs b/opentelemetry-prometheus/tests/integration_test.rs index 0deb0d3105..aff4c43a99 100644 --- a/opentelemetry-prometheus/tests/integration_test.rs +++ b/opentelemetry-prometheus/tests/integration_test.rs @@ -492,7 +492,7 @@ fn duplicate_metrics() { name: "no_conflict_two_histograms", record_metrics: Box::new(|meter_a, meter_b| { let foo_a = meter_a - .i64_histogram("foo") + .u64_histogram("foo") .with_unit(Unit::new("By")) .with_description("meter histogram foo") .init(); @@ -500,7 +500,7 @@ fn duplicate_metrics() { foo_a.record(100, &[KeyValue::new("A", "B")]); let foo_b = meter_b - .i64_histogram("foo") + .u64_histogram("foo") .with_unit(Unit::new("By")) .with_description("meter histogram foo") .init(); @@ -564,7 +564,7 @@ fn duplicate_metrics() { name: "conflict_help_two_histograms", record_metrics: Box::new(|meter_a, meter_b| { let bar_a = meter_a - .i64_histogram("bar") + .u64_histogram("bar") .with_unit(Unit::new("By")) .with_description("meter a bar") .init(); @@ -572,7 +572,7 @@ fn duplicate_metrics() { bar_a.record(100, &[KeyValue::new("A", "B")]); let bar_b = meter_b - .i64_histogram("bar") + .u64_histogram("bar") .with_unit(Unit::new("By")) .with_description("meter b bar") .init(); @@ -635,7 +635,7 @@ fn duplicate_metrics() { name: "conflict_unit_two_histograms", record_metrics: Box::new(|meter_a, meter_b| { let bar_a = meter_a - .i64_histogram("bar") + .u64_histogram("bar") .with_unit(Unit::new("By")) .with_description("meter histogram bar") .init(); @@ -643,7 +643,7 @@ fn duplicate_metrics() { bar_a.record(100, &[KeyValue::new("A", "B")]); let bar_b = meter_b - .i64_histogram("bar") + .u64_histogram("bar") .with_unit(Unit::new("ms")) .with_description("meter histogram bar") .init(); @@ -692,7 +692,7 @@ fn duplicate_metrics() { foo_a.add(100, &[KeyValue::new("A", "B")]); let foo_histogram_a = meter_a - .i64_histogram("foo") + .u64_histogram("foo") .with_unit(Unit::new("By")) .with_description("meter histogram foo") .init(); diff --git a/opentelemetry-sdk/benches/metric.rs b/opentelemetry-sdk/benches/metric.rs index 052295c279..d018634e04 100644 --- a/opentelemetry-sdk/benches/metric.rs +++ b/opentelemetry-sdk/benches/metric.rs @@ -349,7 +349,7 @@ fn counters(c: &mut Criterion) { const MAX_BOUND: usize = 100000; -fn bench_histogram(bound_count: usize) -> (SharedReader, Histogram) { +fn bench_histogram(bound_count: usize) -> (SharedReader, Histogram) { let mut bounds = vec![0; bound_count]; #[allow(clippy::needless_range_loop)] for i in 0..bounds.len() { @@ -373,7 +373,7 @@ fn bench_histogram(bound_count: usize) -> (SharedReader, Histogram) { } let mtr = builder.build().meter("test_meter"); let hist = mtr - .i64_histogram(format!("histogram_{}", bound_count)) + .u64_histogram(format!("histogram_{}", bound_count)) .init(); (r, hist) @@ -393,7 +393,7 @@ fn histograms(c: &mut Criterion) { format!("V,{},{},{}", bound_size, attr_size, i), )) } - let value: i64 = rng.gen_range(0..MAX_BOUND).try_into().unwrap(); + let value: u64 = rng.gen_range(0..MAX_BOUND).try_into().unwrap(); group.bench_function( format!("Record{}Attrs{}bounds", attr_size, bound_size), |b| b.iter(|| hist.record(value, &attributes)), @@ -414,7 +414,7 @@ fn benchmark_collect_histogram(b: &mut Bencher, n: usize) { .meter("sdk/metric/bench/histogram"); for i in 0..n { - let h = mtr.i64_histogram(format!("fake_data_{i}")).init(); + let h = mtr.u64_histogram(format!("fake_data_{i}")).init(); h.record(1, &[]); } diff --git a/opentelemetry-sdk/src/metrics/meter.rs b/opentelemetry-sdk/src/metrics/meter.rs index b025a9152d..5eaeba7745 100644 --- a/opentelemetry-sdk/src/metrics/meter.rs +++ b/opentelemetry-sdk/src/metrics/meter.rs @@ -444,24 +444,6 @@ impl InstrumentProvider for SdkMeter { .map(|i| Histogram::new(Arc::new(i))) } - fn i64_histogram( - &self, - name: Cow<'static, str>, - description: Option>, - unit: Option, - ) -> Result> { - validate_instrument_config(name.as_ref(), unit.as_ref(), self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.i64_resolver); - - p.lookup( - InstrumentKind::Histogram, - name, - description, - unit.unwrap_or_default(), - ) - .map(|i| Histogram::new(Arc::new(i))) - } - fn register_callback( &self, insts: &[Arc], @@ -819,7 +801,6 @@ mod tests { ); assert(meter.f64_histogram(name.into(), None, None).map(|_| ())); assert(meter.u64_histogram(name.into(), None, None).map(|_| ())); - assert(meter.i64_histogram(name.into(), None, None).map(|_| ())); } // (unit, expected error) @@ -909,11 +890,6 @@ mod tests { .u64_histogram("test".into(), None, unit.clone()) .map(|_| ()), ); - assert( - meter - .i64_histogram("test".into(), None, unit.clone()) - .map(|_| ()), - ); } } } diff --git a/opentelemetry/CHANGELOG.md b/opentelemetry/CHANGELOG.md index 078c0f315b..e1b57c8105 100644 --- a/opentelemetry/CHANGELOG.md +++ b/opentelemetry/CHANGELOG.md @@ -13,9 +13,10 @@ gains, and avoids `IndexMap` dependency. This affects `body` and `attributes` of ### Removed -Removed `OrderMap` type as there was no requirement to use this over regular +- Removed `OrderMap` type as there was no requirement to use this over regular `HashMap`. [#1353](https://github.com/open-telemetry/opentelemetry-rust/pull/1353) +- Remove API for Creating Histograms with signed integers. [#1371](https://github.com/open-telemetry/opentelemetry-rust/pull/1371) ## [v0.21.0](https://github.com/open-telemetry/opentelemetry-rust/compare/v0.20.0...v0.21.0) diff --git a/opentelemetry/src/metrics/instruments/histogram.rs b/opentelemetry/src/metrics/instruments/histogram.rs index af27636c14..c6246ebee2 100644 --- a/opentelemetry/src/metrics/instruments/histogram.rs +++ b/opentelemetry/src/metrics/instruments/histogram.rs @@ -60,15 +60,3 @@ impl TryFrom>> for Histogram { ) } } - -impl TryFrom>> for Histogram { - type Error = MetricsError; - - fn try_from(builder: InstrumentBuilder<'_, Histogram>) -> Result { - builder.meter.instrument_provider.i64_histogram( - builder.name, - builder.description, - builder.unit, - ) - } -} diff --git a/opentelemetry/src/metrics/meter.rs b/opentelemetry/src/metrics/meter.rs index d2bef62b72..a66a77e55c 100644 --- a/opentelemetry/src/metrics/meter.rs +++ b/opentelemetry/src/metrics/meter.rs @@ -241,19 +241,6 @@ pub trait MeterProvider { /// .as_ref(), /// ); /// -/// // i64 histogram -/// let i64_histogram = meter.i64_histogram("my_i64_histogram").init(); -/// -/// // Record measurements using the histogram instrument record() -/// i64_histogram.record( -/// 1, -/// [ -/// KeyValue::new("mykey1", "myvalue1"), -/// KeyValue::new("mykey2", "myvalue2"), -/// ] -/// .as_ref(), -/// ); -/// /// // u64 histogram /// let u64_histogram = meter.u64_histogram("my_u64_histogram").init(); /// @@ -386,14 +373,6 @@ impl Meter { InstrumentBuilder::new(self, name.into()) } - /// creates an instrument builder for recording a distribution of values. - pub fn i64_histogram( - &self, - name: impl Into>, - ) -> InstrumentBuilder<'_, Histogram> { - InstrumentBuilder::new(self, name.into()) - } - /// Registers a callback to be called during the collection of a measurement /// cycle. /// diff --git a/opentelemetry/src/metrics/mod.rs b/opentelemetry/src/metrics/mod.rs index 07b71fc0e1..4241363411 100644 --- a/opentelemetry/src/metrics/mod.rs +++ b/opentelemetry/src/metrics/mod.rs @@ -238,16 +238,6 @@ pub trait InstrumentProvider { Ok(Histogram::new(Arc::new(noop::NoopSyncInstrument::new()))) } - /// creates an instrument for recording a distribution of values. - fn i64_histogram( - &self, - _name: Cow<'static, str>, - _description: Option>, - _unit: Option, - ) -> Result> { - Ok(Histogram::new(Arc::new(noop::NoopSyncInstrument::new()))) - } - /// Captures the function that will be called during data collection. /// /// It is only valid to call `observe` within the scope of the passed function.