-
Notifications
You must be signed in to change notification settings - Fork 502
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: implement batch observer #429
Conversation
This patch adds an implementation of the metrics batch observer. The API is not identical to the example in [the spec](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/metrics/api.md#batch-observer) as it is inconvenient to register metrics after they are moved into the observer callback in rust. Instead the `Meter::batch_observer` method accepts a closure in which instruments can be registered before being moved into the callback. ```rust meter.batch_observer(|batch| { let inst = batch.u64_sum_observer("example").init(); move |result| { result.observe(&[KeyValue::new("a", "1")], inst.observation(42)]); } }); ```
Codecov Report
@@ Coverage Diff @@
## master #429 +/- ##
==========================================
+ Coverage 48.76% 48.85% +0.08%
==========================================
Files 66 66
Lines 5438 5453 +15
==========================================
+ Hits 2652 2664 +12
- Misses 2786 2789 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know the semantics of this code, so just some style remarks.
The nested closure patterns seems a little ugly, but I can't think of a good way around it right now.
{ | ||
let observer = builder(BatchObserver::new(self))?; | ||
self.core | ||
.new_batch_observer(AsyncRunner::Batch(Box::new(observer))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably just inline the call to builder()
here, so you don't have to name the intermediate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting this specifically?
self.core
.new_batch_observer(AsyncRunner::Batch(Box::new(builder(BatchObserver::new(
self,
))?)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Maybe the way rustfmt treats it's not actually an improvement in this case...
{ | ||
let observer = builder(BatchObserver::new(self)); | ||
self.core | ||
.new_batch_observer(AsyncRunner::Batch(Box::new(observer))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
@djc yeah it's not ideal, not sure that there is better syntax for this type of setup currently though. Can iterate on this API if someone comes up with a cleaner way of expressing this as metrics still are not stable. |
This patch adds an implementation of the metrics batch observer. The API is not identical to the example in the spec as it is inconvenient to register metrics after they are moved into the observer callback in rust. Instead the
Meter::batch_observer
method accepts a closure in which instruments can be registered before being moved into the callback.