Skip to content

Commit

Permalink
Remove API for Creating Histograms with signed integers. (#1371)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lalitb committed Nov 15, 2023
1 parent a2c0dd8 commit c06c04b
Show file tree
Hide file tree
Showing 8 changed files with 14 additions and 80 deletions.
2 changes: 1 addition & 1 deletion opentelemetry-prometheus/src/lib.rs
Expand Up @@ -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();
//!
Expand Down
14 changes: 7 additions & 7 deletions opentelemetry-prometheus/tests/integration_test.rs
Expand Up @@ -492,15 +492,15 @@ 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();

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();
Expand Down Expand Up @@ -564,15 +564,15 @@ 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();

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();
Expand Down Expand Up @@ -635,15 +635,15 @@ 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();

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();
Expand Down Expand Up @@ -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();
Expand Down
8 changes: 4 additions & 4 deletions opentelemetry-sdk/benches/metric.rs
Expand Up @@ -349,7 +349,7 @@ fn counters(c: &mut Criterion) {

const MAX_BOUND: usize = 100000;

fn bench_histogram(bound_count: usize) -> (SharedReader, Histogram<i64>) {
fn bench_histogram(bound_count: usize) -> (SharedReader, Histogram<u64>) {
let mut bounds = vec![0; bound_count];
#[allow(clippy::needless_range_loop)]
for i in 0..bounds.len() {
Expand All @@ -373,7 +373,7 @@ fn bench_histogram(bound_count: usize) -> (SharedReader, Histogram<i64>) {
}
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)
Expand All @@ -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)),
Expand All @@ -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, &[]);
}

Expand Down
24 changes: 0 additions & 24 deletions opentelemetry-sdk/src/metrics/meter.rs
Expand Up @@ -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<Cow<'static, str>>,
unit: Option<Unit>,
) -> Result<Histogram<i64>> {
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<dyn Any>],
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -909,11 +890,6 @@ mod tests {
.u64_histogram("test".into(), None, unit.clone())
.map(|_| ()),
);
assert(
meter
.i64_histogram("test".into(), None, unit.clone())
.map(|_| ()),
);
}
}
}
3 changes: 2 additions & 1 deletion opentelemetry/CHANGELOG.md
Expand Up @@ -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)

Expand Down
12 changes: 0 additions & 12 deletions opentelemetry/src/metrics/instruments/histogram.rs
Expand Up @@ -60,15 +60,3 @@ impl TryFrom<InstrumentBuilder<'_, Histogram<u64>>> for Histogram<u64> {
)
}
}

impl TryFrom<InstrumentBuilder<'_, Histogram<i64>>> for Histogram<i64> {
type Error = MetricsError;

fn try_from(builder: InstrumentBuilder<'_, Histogram<i64>>) -> Result<Self, Self::Error> {
builder.meter.instrument_provider.i64_histogram(
builder.name,
builder.description,
builder.unit,
)
}
}
21 changes: 0 additions & 21 deletions opentelemetry/src/metrics/meter.rs
Expand Up @@ -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();
///
Expand Down Expand Up @@ -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<Cow<'static, str>>,
) -> InstrumentBuilder<'_, Histogram<i64>> {
InstrumentBuilder::new(self, name.into())
}

/// Registers a callback to be called during the collection of a measurement
/// cycle.
///
Expand Down
10 changes: 0 additions & 10 deletions opentelemetry/src/metrics/mod.rs
Expand Up @@ -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<Cow<'static, str>>,
_unit: Option<Unit>,
) -> Result<Histogram<i64>> {
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.
Expand Down

0 comments on commit c06c04b

Please sign in to comment.