Skip to content

Conversation

@utpilla
Copy link
Contributor

@utpilla utpilla commented Oct 14, 2024

All the asynchronous instruments implement the trait AsyncInstrument which allows users to call observe method outside of the callback

let observable_counter = meter
        .u64_observable_counter("my_observable_counter")
        .with_callback(|observer| {
            observer.observe(
                100,
                &[
                    KeyValue::new("mykey1", "myvalue1"),
                    KeyValue::new("mykey2", "myvalue2"),
                ],
            )
        })
        .init();

observable_counter.observe(500, &[]); // This should not be allowed

Changes

  • This PR removes the implementation of AsyncInstrument for each of the asynchronous instruments to not allow calling observe outside of a callback (as a side benefit, we also reduce the public API surface)
  • That also means that we can completely remove AsyncInstrument from each of asynchronous instruments as it was only used for implementation of itself

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes

@utpilla utpilla requested a review from a team as a code owner October 14, 2024 02:56
@codecov
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 64.28571% with 10 lines in your changes missing coverage. Please review.

Project coverage is 79.2%. Comparing base (16c0e10) to head (aaea102).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
opentelemetry/src/metrics/mod.rs 0.0% 7 Missing ⚠️
opentelemetry-sdk/src/metrics/meter.rs 66.6% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2210     +/-   ##
=======================================
+ Coverage   79.1%   79.2%   +0.1%     
=======================================
  Files        122     122             
  Lines      21042   21009     -33     
=======================================
- Hits       16645   16644      -1     
+ Misses      4397    4365     -32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Lets mention in changelog as this is breaking (though unlikely to affect anyone in practice)

#[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?

@lalitb lalitb merged commit 8d84a76 into open-telemetry:main Oct 15, 2024
23 of 24 checks passed
@utpilla utpilla changed the title [Metrics-API] Update asynchronous instruments to not allow calling observe outside of a callback [Metrics API] Update asynchronous instruments to not allow calling observe outside of a callback Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants