rust-prometheus
rust-prometheus copied to clipboard
Allow into/from inner `Arc` conversions
Overview
This PR adds:
-
into_arc_value()
andfrom_arc_value()
methods toGenericCounter
andGenericGauge
types. These methods areunsafe
as allow bypassing counter/gauge semantics. -
into_arc_core()
andfrom_arc_core()
methods toHistogram
type. These methods are safe, as theHistogramCore
public API doesn't allow bypassingHistogram
semantics in any way. -
#[repr(transparent)]
attribute toGenericCounter
,GenericGauge
andHistogram
, so they can be safely transmuted into its innerArc
and back.
Motivation
While implementing the metrics-prometheus
crate, I have the situation where the metrics
crate requires the returned metric being Arc
-wrapped (because returns to users Arc<dyn Trait>
essentially), and so, pushing me to introduce a completely redundant allocation on the hot path accessing the metric, because prometheus
metric types have Arc
ed semantics anyway.
First, I've tried to approach this on the metrics
crate's side, but had no luck, as its API returns Arc<dyn Trait>
to user code, so creating an Arc
cannot be really bypassed here.
Then, I've looked at prometheus
metric type's structure and realized that I can unwrap it into its inner Arc
, and then, in the Trait
implementation, wrap back into the original metric type. The whole thing doesn't leave the library boundary, so users won't be able to misuse the unwrapped Arc
s anyhow.
As the bonus, it will allow me to solve the synchronization issue from #471, but without exposing safe APIs to prometheus
users which do not follow metric type's semantics.
Notes
- I did intentionally use
this: Self
argument instead of justself
ininto*
methods, mimicking theArc::into_raw()
, to additionally discourage library users from using those methods.
Checklist
- [x] Documentation added
- [x] ~~Tests are added~~ (do we need ones here?)
- [x] CHANGELOG entry is added
Thanks for the patch. I'm still slightly unconvinced on the from_arc_*()
methods, but let me sleep a bit longer on that. The rest of this PR looks good, although a bit contrived.
I'd even say the into_arc_*()
methods may not need to be marked unsafe
, as I don't see any major abuse coming from those. If we are concerned about confused users, we could mark everything with #[doc(hidden)]
.
It is my understanding that the correctness of this relies on Value
being a private type, right?
@lucab
If we are concerned about confused users, we could mark everything with `#[doc(hidden)].
This way, the people who really need this (like me), may not find it at all.
To be honest, for my needs, adding #[repr(transparent)]
is just enough, as allows me to safely mem::transmute()
back and forth. However, I thought that my use case is not unique, and other people building an ecosystem or library glues, may need this too, so having this visible in API may help them to come up with a solution.
It is my understanding that the safety of this relies on
Value
being a private type, right?
Not sure, I've understood you. You mean my use case, or the proposed API of prometheus
crate?
Value
is a public type in prometheus
crate, which powers both GenericCounter
and GenericGauge
types. Using it directly will allow easily (I'd say even accidentally) to bypass semantics of the GenericCounter
/GenericGauge
it represents (making a counter negative, for example), that's why I think obtaining it from a metric type, or building a metric type from it, should be unsafe
, despite it has nothing to do with the memory safety itself.
In my use case, I need to return to library users a type being essentially an Arc<dyn metrics::CounterFn>
, where metrics::CounterFn
's API itself doesn't allow misusing a counter anyhow. So, I just do the following transformation: prometeus::IntCounter
-> Arc<Value<AtomicU64>>
-> Arc<Mine<Value<AtomicU64>>>
-> Arc<dyn metrics::CounterFn>
, where Mine
is a #[repr(transparent)]
wrapper for bypassing orphan rules and implementing metrics::CounterFn
. This way, I just reuse Arc
inside prometeus::IntCounter
without introducing an allocation of new Arc
, and don't expose anything fragile to library users.
I meant on the proposed API for the prometheus
crate.
Perhaps I'm misunderstanding something, but I think that currently prometheus::value::Value
is not a public type, because the module prometheus::value
is not public.
Did you already try to assemble all of this together back in metrics-prometheus
? Does it work?
Anyway, If the transparent+transmute dance is enough to make your usecase working, then I think we can stick to that for the moment in this PR.
@lucab
because the module
prometheus::value
is not public.
Ooops, I didn't notice that. Thought it was public as the type was pub
itself. 🤔 (#[warn(unreachable_pub)]
habit)
Yup, that's unfortunate, as even for mem::transmute()
without into_*
/from_*
methods, we still need to name the Value
type.
Did you already try to assemble all of this together back in
metrics-prometheus
? Does it work?
Not yet. I'll craft the PR referring this one and write back.