client_rust icon indicating copy to clipboard operation
client_rust copied to clipboard

Consider removing text::Encode in favour of serde::Serialize

Open nox opened this issue 1 year ago • 4 comments

Serde's Serialize trait is implemented for dozens of types in the Rust ecosystem, its derive code is fairly well optimised, and it is the most popular serialization framework we have for Rust.

What do you think of removing the Encode trait in favour of Serialize, to get all the nice things there are in Serde for free?

nox avatar Sep 20 '22 13:09 nox

I would be in favor of that. I should have thought of it before.

@nox would you like to write a first proposal? Before you go all the way, I would suggest writing a small proof-of-concept first to make sure using serde is a viable path forward.

mxinden avatar Sep 21 '22 13:09 mxinden

Sure, I'll write a struct Bridge<T>(T) that implements Encode using T's implementation of Serialize, in a separate crate.

nox avatar Sep 21 '22 14:09 nox

I pushed https://github.com/nox/serde_prometheus_labels earlier today.

nox avatar Sep 23 '22 12:09 nox

I published serde_prometheus_labels which provides its own Family<Labels, Metric> type using serde as the encoder.

nox avatar Oct 10 '22 16:10 nox

How would this interact with a problem like the one in #75? I mean, () also implements Serialize/Deserialize but you probably shouldn't be able to use () as a label, for instance. So I'm not sure Encode can be entirely replaced by serde's traits, as serde's traits are basically implemented for too many types that aren't suitable.

Victor-N-Suadicani avatar Oct 19 '22 09:10 Victor-N-Suadicani

I mean, () also implements Serialize/Deserialize but you probably shouldn't be able to use () as a label, for instance.

Why not? () should mean "no labels at all".

nox avatar Oct 19 '22 11:10 nox

So I'm not sure Encode can be entirely replaced by serde's traits, as serde's traits are basically implemented for too many types that aren't suitable.

I agree to this, but I would rather rely on serde and not be blocked on some missing implementation. It made the job for us, that's why I published that stuff as a separate crate.

nox avatar Oct 19 '22 11:10 nox

With https://github.com/prometheus/client_rust/pull/105 I don't think we should proceed here, i.e. I don't see the value of replacing the new traits.

@nox I am closing here for now. Please comment here in case I am missing something.

The above said I do believe implementing EncodeLabelSet, EncodeLabel, EncodeLabelKey and EncodeLabelValue for more types in std would be helpful.

mxinden avatar Jan 02 '23 16:01 mxinden