tikv / rust-prometheus

Prometheus instrumentation library for Rust applications
Apache License 2.0
1.05k stars 182 forks source link

Allow into/from inner `Arc` conversions #472

Open tyranron opened 1 year ago

tyranron commented 1 year ago

Overview

This PR adds:

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 Arced 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 Arcs 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

Checklist

lucab commented 1 year ago

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?

tyranron commented 1 year ago

@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.

lucab commented 1 year ago

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.

tyranron commented 1 year ago

@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.