Skip to content
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

Allow into/from inner Arc conversions #472

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tyranron
Copy link
Contributor

@tyranron tyranron commented Dec 14, 2022

Overview

This PR adds:

  • into_arc_value() and from_arc_value() methods to GenericCounter and GenericGauge types. These methods are unsafe as allow bypassing counter/gauge semantics.
  • into_arc_core() and from_arc_core() methods to Histogram type. These methods are safe, as the HistogramCore public API doesn't allow bypassing Histogram semantics in any way.
  • #[repr(transparent)] attribute to GenericCounter, GenericGauge and Histogram, so they can be safely transmuted into its inner Arc 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 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

  • I did intentionally use this: Self argument instead of just self in into* methods, mimicking the Arc::into_raw(), to additionally discourage library users from using those methods.

Checklist

  • Documentation added
  • Tests are added (do we need ones here?)
  • CHANGELOG entry is added

@lucab
Copy link
Member

lucab commented Dec 15, 2022

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
Copy link
Contributor Author

@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
Copy link
Member

lucab commented Dec 15, 2022

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants