Add API support for bound instruments #1444
Conversation
# Conflicts: # opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs
|
The api will change slightly when/if #1421 get in to take an attribute set instead of a slice of key value pairs, but I didn't want to couple both together. Bounded performance might end up getting a bit better since |
| @@ -10,6 +10,15 @@ use std::{any::Any, convert::TryFrom}; | |||
| pub trait SyncCounter<T> { | |||
| /// Records an increment to the counter. | |||
| fn add(&self, value: T, attributes: &[KeyValue]); | |||
|
|
|||
| /// Creates a child counter that's bound to a specific attribute set | |||
| fn bind(&self, attributes: &[KeyValue]) -> Arc<dyn BoundSyncCounter<T> + Send + Sync>; | |||
There was a problem hiding this comment.
As Bound Instrument is not in specs (yet), should we have them under the feature flag (otel_unstable)? Not sure if this is going to add more noise with cfg macros across.
There was a problem hiding this comment.
I think as long as on the API side we mark it then we should be ok ?
I don't believe we need to add everywhere. Mainly on the pub interfaces of API and SDK.
This is where having specific feature flags is useful.
There was a problem hiding this comment.
Mainly on the pub interfaces of API and SDK.
Agree, that's what we followed for sync gauge support in #1410 - only the pub interfaces are encapsulated within the feature flag.
|
|
||
| /// Creates a child counter that's bound to a specific attribute set | ||
| fn bind(&self, attributes: &[KeyValue]) -> Arc<dyn BoundSyncCounter<T> + Send + Sync>; | ||
| } |
There was a problem hiding this comment.
Not sure how relevant is this for the current implementation, but the earlier bound instrument specs also had an unbind() method to release (any) allocated resources - https://github.com/open-telemetry/opentelemetry-specification/blob/v1.1.0/specification/metrics/api.md#bound-instrument-calling-convention
As a consequence of their performance advantage, bound instruments also consume resources in the SDK. Bound instruments MUST support an Unbind() method for users to indicate they are finished with the binding and release the associated resources. Note that Unbind() does
There was a problem hiding this comment.
I saw that in the spec, but figured we didn't need it in Rust because UnBind() is no different than drop.
There was a problem hiding this comment.
How does this affect the Bounded Measure Generator being 'static ?
If at all.
Maybe a test is worthwhile.
There was a problem hiding this comment.
'static is a lifetime, it's not the static keyword. A lifetime only defines the scope of how long a value can last for. In other words it defines how long a value can last, not how long it will last. Almost all owned values have a 'static lifetime because they can be passed to other threads and can be kept alive for the rest of the program's lifetime if you want. However, when they go out of scope they get drop() called, just like any other owned value.
fn do_stuff(counter: Counter<u64>) {
// pre_do_stuff
{
let bound_counter = counter.Bind(some_attributes);
bound_counter.add(5);
} // <-------------------------------------------------------- Drop is here
// other stuff
}Since the bound_counter variable goes out of scope on the Drop is here line, the compiler calls drop() for it. Since drop() doesn't have an explicit implementation, it just calls drop() on all of it's fields, which will drop the Arc, which will either modify the reference count, or drop the underlying bound measure if it's the last instance of it.
I'm not really sure what you would test for. Not only is this all built-in and is fundamental to the whole Rust lifetime system, there's no observable behavior in this PR (there is in the performance oriented PR I have ready to go, but not in this PR).
The only reason 'static is defined in this code is because the Arc is holding a trait, not a concrete object. That means someone could have implemented the BoundSyncCounter<T> trait on a shared reference, in which case it's not valid to pass around because the compiler would not be able to guarantee the shared reference doesn't outlive it's owner. Thus 'static is required to tell the compiler "this is safe to pass around in any context, as this can live up to the end of the application if the consumer wants".
There was a problem hiding this comment.
There was a problem hiding this comment.
This fully utilizes the trait to make sure it's not getting dropped due to non-use: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=87650c8e735c22878046af4c5d567e69
There was a problem hiding this comment.
Like the general direction, kinda surprised not to see much of a performance gain over standard counters. I feel like (not related to this change) we have a lot of Arc spread through the code that maybe we could avoid. A bit of locking our locks so we can lock more locks.
The reason this doesn't provide any performance benefits is partially because #1421 is not yet merged. Since we don't have that change in, we must do a hard clone of the attribute set every time a bound measure is called (and for each measure in the There's no performance benefit too because this PR has bound instruments as a thin wrapper around normal measurements. Once this PR is merged, the next PR fully takes advantage of bound instruments by creating an atomic that's incremented by the bound measure function. This allows the |
KallDrexx
force-pushed
the
bounded_instruments
branch
from
748d5ff
to
e88a102
Compare
KallDrexx
force-pushed
the
bounded_instruments
branch
from
e88a102
to
cd3be86
Compare
Fixes #
Design discussion issue (if applicable) #1374
Changes
This PR adds API support for bound instruments for
CounterandUpDownCounter. Bound instruments are counters which always change the value for a specific and predefined attribute set. The focus on this change is creating the initial API surface for bound instruments, and the mechanisms to allow non-bound instruments to become bound (since there is no direct path from aggregates to counters, it's all through closures).To facilitate counters creating bound instruments that can refer back to their source aggregates, aggregates return a
BoundMeasureGenerator<T>closure that is provided alongside theMeasure<T>for the aggregate. This closure can then be invoked in order to return aBoundMeasure<T>, which provides the mechanism to invoke a measure for the cached attribute set.This PR is not performance focused. It should not cause regressions but using bound instruments from this PR may not cause increased throughput. Performance enhancements that take advantage of bound instruments will come in a follow-up PR once we have an agreed upon API and generation path.
Benchmarks/Stress Tests
Main
PR
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes