Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Metrics API] Update asynchronous instruments to not allow calling observe outside of a callback #2210

Merged
merged 5 commits into from
Oct 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 10 additions & 14 deletions opentelemetry-sdk/src/metrics/meter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
pipeline::{Pipelines, Resolver},
};

use super::noop::{NoopAsyncInstrument, NoopSyncInstrument};
use super::noop::NoopSyncInstrument;

// maximum length of instrument name
const INSTRUMENT_NAME_MAX_LENGTH: usize = 255;
Expand Down Expand Up @@ -108,7 +108,7 @@
let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit);
if let Err(err) = validation_result {
global::handle_error(err);
return Ok(ObservableCounter::new(Arc::new(NoopAsyncInstrument::new())));
return Ok(ObservableCounter::new());
}

let ms = resolver.measures(
Expand All @@ -120,7 +120,7 @@
)?;

if ms.is_empty() {
return Ok(ObservableCounter::new(Arc::new(NoopAsyncInstrument::new())));
return Ok(ObservableCounter::new());

Check warning on line 123 in opentelemetry-sdk/src/metrics/meter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/meter.rs#L123

Added line #L123 was not covered by tests
}

let observable = Arc::new(Observable::new(ms));
Expand All @@ -131,7 +131,7 @@
.register_callback(move || callback(cb_inst.as_ref()));
}

Ok(ObservableCounter::new(observable))
Ok(ObservableCounter::new())
}

fn create_observable_updown_counter<T>(
Expand All @@ -145,9 +145,7 @@
let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit);
if let Err(err) = validation_result {
global::handle_error(err);
return Ok(ObservableUpDownCounter::new(Arc::new(
NoopAsyncInstrument::new(),
)));
return Ok(ObservableUpDownCounter::new());
}

let ms = resolver.measures(
Expand All @@ -159,9 +157,7 @@
)?;

if ms.is_empty() {
return Ok(ObservableUpDownCounter::new(Arc::new(
NoopAsyncInstrument::new(),
)));
return Ok(ObservableUpDownCounter::new());

Check warning on line 160 in opentelemetry-sdk/src/metrics/meter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/meter.rs#L160

Added line #L160 was not covered by tests
}

let observable = Arc::new(Observable::new(ms));
Expand All @@ -172,7 +168,7 @@
.register_callback(move || callback(cb_inst.as_ref()));
}

Ok(ObservableUpDownCounter::new(observable))
Ok(ObservableUpDownCounter::new())
}

fn create_observable_gauge<T>(
Expand All @@ -186,7 +182,7 @@
let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit);
if let Err(err) = validation_result {
global::handle_error(err);
return Ok(ObservableGauge::new(Arc::new(NoopAsyncInstrument::new())));
return Ok(ObservableGauge::new());
}

let ms = resolver.measures(
Expand All @@ -198,7 +194,7 @@
)?;

if ms.is_empty() {
return Ok(ObservableGauge::new(Arc::new(NoopAsyncInstrument::new())));
return Ok(ObservableGauge::new());

Check warning on line 197 in opentelemetry-sdk/src/metrics/meter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/meter.rs#L197

Added line #L197 was not covered by tests
}

let observable = Arc::new(Observable::new(ms));
Expand All @@ -209,7 +205,7 @@
.register_callback(move || callback(cb_inst.as_ref()));
}

Ok(ObservableGauge::new(observable))
Ok(ObservableGauge::new())
}

fn create_updown_counter<T>(
Expand Down
21 changes: 1 addition & 20 deletions opentelemetry-sdk/src/metrics/noop.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use opentelemetry::{
metrics::{AsyncInstrument, InstrumentProvider, SyncInstrument},
metrics::{InstrumentProvider, SyncInstrument},
KeyValue,
};

Expand Down Expand Up @@ -36,22 +36,3 @@ impl<T> SyncInstrument<T> for NoopSyncInstrument {
// Ignored
}
}

/// A no-op async instrument.
#[derive(Debug, Default)]
pub(crate) struct NoopAsyncInstrument {
_private: (),
}

impl NoopAsyncInstrument {
/// Create a new no-op async instrument
pub(crate) fn new() -> Self {
NoopAsyncInstrument { _private: () }
}
}

impl<T> AsyncInstrument<T> for NoopAsyncInstrument {
fn observe(&self, _value: T, _attributes: &[KeyValue]) {
// Ignored
}
}
5 changes: 3 additions & 2 deletions opentelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@

- Bump MSRV to 1.70 [#2179](https://github.com/open-telemetry/opentelemetry-rust/pull/2179)
- Add `LogRecord::set_trace_context`; an optional method conditional on the `trace` feature for setting trace context on a log record.
- Removed unnecessary public methods named `as_any` from `AsyncInstrument` trait and the implementing instruments: `ObservableCounter`, `ObservableGauge`, and `ObservableUpDownCounter` [#2187](https://github.com/open-telemetry/opentelemetry-rust/issues/2187)
- Introduced `SyncInstrument` trait to replace the individual synchronous instrument traits (`SyncCounter`, `SyncGauge`, `SyncHistogram`, `SyncUpDownCounter`) which are meant for SDK implementation. [#2207](https://github.com/open-telemetry/opentelemetry-rust/issues/2207)
- Removed unnecessary public methods named `as_any` from `AsyncInstrument` trait and the implementing instruments: `ObservableCounter`, `ObservableGauge`, and `ObservableUpDownCounter` [#2187](https://github.com/open-telemetry/opentelemetry-rust/pull/2187)
- Introduced `SyncInstrument` trait to replace the individual synchronous instrument traits (`SyncCounter`, `SyncGauge`, `SyncHistogram`, `SyncUpDownCounter`) which are meant for SDK implementation. [#2207](https://github.com/open-telemetry/opentelemetry-rust/pull/2207)
- Ensured that `observe` method on asynchronous instruments can only be called inside a callback. This was done by removing the implementation of `AsyncInstrument` trait for each of the asynchronous instruments. [#2210](https://github.com/open-telemetry/opentelemetry-rust/pull/2210)

## v0.26.0
Released 2024-Sep-30
Expand Down
19 changes: 9 additions & 10 deletions opentelemetry/src/metrics/instruments/counter.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{metrics::AsyncInstrument, KeyValue};
use crate::KeyValue;
use core::fmt;
use std::sync::Arc;

Expand Down Expand Up @@ -33,12 +33,17 @@ impl<T> Counter<T> {
/// An async instrument that records increasing values.
#[derive(Clone)]
#[non_exhaustive]
pub struct ObservableCounter<T>(Arc<dyn AsyncInstrument<T>>);
pub struct ObservableCounter<T> {
_marker: std::marker::PhantomData<T>,
}

impl<T> ObservableCounter<T> {
/// Create a new observable counter.
pub fn new(inner: Arc<dyn AsyncInstrument<T>>) -> Self {
ObservableCounter(inner)
#[allow(clippy::new_without_default)]
pub fn new() -> Self {
ObservableCounter {
_marker: std::marker::PhantomData,
}
}
}

Expand All @@ -50,9 +55,3 @@ impl<T> fmt::Debug for ObservableCounter<T> {
))
}
}

impl<T> AsyncInstrument<T> for ObservableCounter<T> {
fn observe(&self, measurement: T, attributes: &[KeyValue]) {
self.0.observe(measurement, attributes)
}
}
19 changes: 9 additions & 10 deletions opentelemetry/src/metrics/instruments/gauge.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{metrics::AsyncInstrument, KeyValue};
use crate::KeyValue;
use core::fmt;
use std::sync::Arc;

Expand Down Expand Up @@ -33,7 +33,9 @@ impl<T> Gauge<T> {
/// An async instrument that records independent readings.
#[derive(Clone)]
#[non_exhaustive]
pub struct ObservableGauge<T>(Arc<dyn AsyncInstrument<T>>);
pub struct ObservableGauge<T> {
_marker: std::marker::PhantomData<T>,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Declare the full type name of PhantomData in use?


impl<T> fmt::Debug for ObservableGauge<T>
where
Expand All @@ -47,15 +49,12 @@ where
}
}

impl<M> AsyncInstrument<M> for ObservableGauge<M> {
fn observe(&self, measurement: M, attributes: &[KeyValue]) {
self.0.observe(measurement, attributes)
}
}

impl<T> ObservableGauge<T> {
/// Create a new gauge
pub fn new(inner: Arc<dyn AsyncInstrument<T>>) -> Self {
ObservableGauge(inner)
#[allow(clippy::new_without_default)]
pub fn new() -> Self {
ObservableGauge {
_marker: std::marker::PhantomData,
}
}
}
10 changes: 2 additions & 8 deletions opentelemetry/src/metrics/instruments/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,7 @@ pub type Callback<T> = Box<dyn Fn(&dyn AsyncInstrument<T>) + Send + Sync>;

/// Configuration for building an async instrument.
#[non_exhaustive] // We expect to add more configuration fields in the future
pub struct AsyncInstrumentBuilder<'a, I, M>
where
I: AsyncInstrument<M>,
{
pub struct AsyncInstrumentBuilder<'a, I, M> {
/// Instrument provider is used to create the instrument.
pub instrument_provider: &'a dyn InstrumentProvider,

Expand All @@ -263,10 +260,7 @@ where
_inst: marker::PhantomData<I>,
}

impl<'a, I, M> AsyncInstrumentBuilder<'a, I, M>
where
I: AsyncInstrument<M>,
{
impl<'a, I, M> AsyncInstrumentBuilder<'a, I, M> {
/// Create a new instrument builder
pub(crate) fn new(meter: &'a Meter, name: Cow<'static, str>) -> Self {
AsyncInstrumentBuilder {
Expand Down
19 changes: 9 additions & 10 deletions opentelemetry/src/metrics/instruments/up_down_counter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::KeyValue;
use core::fmt;
use std::sync::Arc;

use super::{AsyncInstrument, SyncInstrument};
use super::SyncInstrument;

/// An instrument that records increasing or decreasing values.
#[derive(Clone)]
Expand Down Expand Up @@ -36,7 +36,9 @@ impl<T> UpDownCounter<T> {
/// An async instrument that records increasing or decreasing values.
#[derive(Clone)]
#[non_exhaustive]
pub struct ObservableUpDownCounter<T>(Arc<dyn AsyncInstrument<T>>);
pub struct ObservableUpDownCounter<T> {
_marker: std::marker::PhantomData<T>,
}

impl<T> fmt::Debug for ObservableUpDownCounter<T>
where
Expand All @@ -52,13 +54,10 @@ where

impl<T> ObservableUpDownCounter<T> {
/// Create a new observable up down counter.
pub fn new(inner: Arc<dyn AsyncInstrument<T>>) -> Self {
ObservableUpDownCounter(inner)
}
}

impl<T> AsyncInstrument<T> for ObservableUpDownCounter<T> {
fn observe(&self, measurement: T, attributes: &[KeyValue]) {
self.0.observe(measurement, attributes)
#[allow(clippy::new_without_default)]
pub fn new() -> Self {
ObservableUpDownCounter {
_marker: std::marker::PhantomData,
}
}
}
28 changes: 7 additions & 21 deletions opentelemetry/src/metrics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,19 +123,15 @@
&self,
_builder: AsyncInstrumentBuilder<'_, ObservableCounter<u64>, u64>,
) -> Result<ObservableCounter<u64>> {
Ok(ObservableCounter::new(Arc::new(
noop::NoopAsyncInstrument::new(),
)))
Ok(ObservableCounter::new())

Check warning on line 126 in opentelemetry/src/metrics/mod.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry/src/metrics/mod.rs#L126

Added line #L126 was not covered by tests
}

/// creates an instrument for recording increasing values via callback.
fn f64_observable_counter(
&self,
_builder: AsyncInstrumentBuilder<'_, ObservableCounter<f64>, f64>,
) -> Result<ObservableCounter<f64>> {
Ok(ObservableCounter::new(Arc::new(
noop::NoopAsyncInstrument::new(),
)))
Ok(ObservableCounter::new())

Check warning on line 134 in opentelemetry/src/metrics/mod.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry/src/metrics/mod.rs#L134

Added line #L134 was not covered by tests
}

/// creates an instrument for recording changes of a value.
Expand Down Expand Up @@ -163,19 +159,15 @@
&self,
_builder: AsyncInstrumentBuilder<'_, ObservableUpDownCounter<i64>, i64>,
) -> Result<ObservableUpDownCounter<i64>> {
Ok(ObservableUpDownCounter::new(Arc::new(
noop::NoopAsyncInstrument::new(),
)))
Ok(ObservableUpDownCounter::new())

Check warning on line 162 in opentelemetry/src/metrics/mod.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry/src/metrics/mod.rs#L162

Added line #L162 was not covered by tests
}

/// creates an instrument for recording changes of a value via callback.
fn f64_observable_up_down_counter(
&self,
_builder: AsyncInstrumentBuilder<'_, ObservableUpDownCounter<f64>, f64>,
) -> Result<ObservableUpDownCounter<f64>> {
Ok(ObservableUpDownCounter::new(Arc::new(
noop::NoopAsyncInstrument::new(),
)))
Ok(ObservableUpDownCounter::new())

Check warning on line 170 in opentelemetry/src/metrics/mod.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry/src/metrics/mod.rs#L170

Added line #L170 was not covered by tests
}

/// creates an instrument for recording independent values.
Expand All @@ -198,29 +190,23 @@
&self,
_builder: AsyncInstrumentBuilder<'_, ObservableGauge<u64>, u64>,
) -> Result<ObservableGauge<u64>> {
Ok(ObservableGauge::new(Arc::new(
noop::NoopAsyncInstrument::new(),
)))
Ok(ObservableGauge::new())

Check warning on line 193 in opentelemetry/src/metrics/mod.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry/src/metrics/mod.rs#L193

Added line #L193 was not covered by tests
}

/// creates an instrument for recording the current value via callback.
fn i64_observable_gauge(
&self,
_builder: AsyncInstrumentBuilder<'_, ObservableGauge<i64>, i64>,
) -> Result<ObservableGauge<i64>> {
Ok(ObservableGauge::new(Arc::new(
noop::NoopAsyncInstrument::new(),
)))
Ok(ObservableGauge::new())

Check warning on line 201 in opentelemetry/src/metrics/mod.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry/src/metrics/mod.rs#L201

Added line #L201 was not covered by tests
}

/// creates an instrument for recording the current value via callback.
fn f64_observable_gauge(
&self,
_builder: AsyncInstrumentBuilder<'_, ObservableGauge<f64>, f64>,
) -> Result<ObservableGauge<f64>> {
Ok(ObservableGauge::new(Arc::new(
noop::NoopAsyncInstrument::new(),
)))
Ok(ObservableGauge::new())

Check warning on line 209 in opentelemetry/src/metrics/mod.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry/src/metrics/mod.rs#L209

Added line #L209 was not covered by tests
}

/// creates an instrument for recording a distribution of values.
Expand Down
21 changes: 1 addition & 20 deletions opentelemetry/src/metrics/noop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
//! has been set. It is expected to have minimal resource utilization and
//! runtime impact.
use crate::{
metrics::{AsyncInstrument, InstrumentProvider, Meter, MeterProvider},
metrics::{InstrumentProvider, Meter, MeterProvider},
KeyValue,
};
use std::sync::Arc;
Expand Down Expand Up @@ -69,22 +69,3 @@ impl<T> SyncInstrument<T> for NoopSyncInstrument {
// Ignored
}
}

/// A no-op async instrument.
#[derive(Debug, Default)]
pub(crate) struct NoopAsyncInstrument {
_private: (),
}

impl NoopAsyncInstrument {
/// Create a new no-op async instrument
pub(crate) fn new() -> Self {
NoopAsyncInstrument { _private: () }
}
}

impl<T> AsyncInstrument<T> for NoopAsyncInstrument {
fn observe(&self, _value: T, _attributes: &[KeyValue]) {
// Ignored
}
}
Loading