Allow CoreMeter implementations to use their own custom attribute reference
#608
Conversation
Sushisource
force-pushed
the
metrics-api-refine
branch
from
81caf2f to
c058d04
Compare
core-api/src/telemetry/metrics.rs
Outdated
| new_hole: BufferAttributes, | ||
| existing_hole: Option<BufferAttributes>, | ||
| attributes: Vec<MetricKeyValue>, |
There was a problem hiding this comment.
| new_hole: BufferAttributes, | |
| existing_hole: Option<BufferAttributes>, | |
| attributes: Vec<MetricKeyValue>, | |
| populate_into: BufferAttributes, | |
| // Do not mutate append_from | |
| append_from: Option<BufferAttributes>, | |
| new_attributes: Vec<MetricKeyValue>, |
Was a bit confused by terminology at first, but I think I have it right here (sorry the term "hole" confused me). Arguably we could split BufferAttributes into read/write traits, but I think this is fine.
I will have to see if I can use a https://docs.rs/pyo3/latest/pyo3/types/struct.PyDict.html based CustomMetricAttributes implementation to "set" here. I will have to think if I will just use a basic impl CustomMetricAttributes for HashMap<String, MetricValue> { (well map wrapper since I own neither side) or something more advanced. Probably the latter, so this code LGTM.
So I suppose when implementing, as I walk these events, it's at that time I will call populate_into.set.
There was a problem hiding this comment.
Yes, that's when you'd do it.
| /// implement this marker trait | ||
| pub trait BufferInstrumentRef {} | ||
| /// A lazy reference to a metrics buffer instrument | ||
| pub type LazyBufferInstrument<T> = LazyRef<Arc<T>>; |
There was a problem hiding this comment.
Can we put a type constraint on T that it has to implement BufferInstrumentRef? Or maybe I can have that constraint above? Or rather, is it at least expected that T implements BufferInstrumentRef?
There was a problem hiding this comment.
EDIT: Nevermind, I see this constraint is now available on the CoreMeter impl
core/src/telemetry/metrics.rs
Outdated
|
|
||
| impl<I> CoreMeter for MetricsCallBuffer<I> | ||
| where | ||
| I: Debug + Send + Sync + Clone + 'static, |
There was a problem hiding this comment.
Hrmm, so I guess if I'm tying my non-cloneable Python metric's instrument instance to this, I will have to wrap it in Arc or similar? It doesn't seem like CustomMetricAttributes has to implement Clone. I don't think it's a problem though.
There was a problem hiding this comment.
The trait doesn't require it, but the places where it's used do (when they need it) which is fairly typical practice
3 participants
What was changed
Added
CustomMetricAttributestrait / variant toMetricAttributesto allow langs to have a simple reference based approach to allocating their own attributes.Why?
Allows destroying backing attributes on
Dropof the thing implementingCustomMetricAttributesChecklist
It may no longer be worth keeping a buffer at all, or, if we do, I need to re-implement it to work in terms of refs.
Closes
How was this tested:
Existing tests
Any docs updates needed?