encoding/: Adopt serde style encoding #105
Conversation
src/encoding.rs
Outdated
| #[cfg(feature = "protobuf")] | ||
| pub mod proto; | ||
| pub mod text; | ||
|
|
||
| // TODO: Rename to MetricEncode to be consistent with MetricEncoder. |
There was a problem hiding this comment.
Everything we do are metrics in here, yes? Can we drop the Metric prefix?
| Protobuf(proto::LabelKeyEncoder<'a>), | ||
| } | ||
|
|
||
| impl<'a> From<text::LabelKeyEncoder<'a>> for LabelKeyEncoder<'a> { |
There was a problem hiding this comment.
I'd expect that these are only used within this crate (and not exported publicly) so I am not sure we need these impls.
| let (key, value) = self; | ||
|
|
||
| let mut label_key_encoder = encoder.encode_label_key(); | ||
| key.encode(&mut label_key_encoder)?; |
There was a problem hiding this comment.
In serde, I believe this is flipped: The serializer has functions for writing certain types, like Serializer::serialize_str: https://docs.rs/serde/latest/serde/trait.Serializer.html#tymethod.serialize_str
Would it make sense to do the same thing here? Our "data format" are metrics so I think it would make sense if an encoder has a function "write_label" and you can pass a string to it.
I still need to dig into this in detail so excuse if it doesn't make any sense!
src/metrics/counter.rs
Outdated
| ) -> Result<(), std::fmt::Error> | ||
| where | ||
| S: EncodeLabelSet, | ||
| CounterValue: EncodeValue, |
There was a problem hiding this comment.
Instead of dispatching to the value here, we could have one function on MetricEncoder per type and we'd probably just need to macro_rules the counter definition for each supported value type.
Not sure if worth it but I think it would reduce some indirection in dispatching between all these things.
|
I had a closer look at this and I found the the decomposition of the functions on I think this could drastically simplify, how often we need to dispatch and e.g. also solves the issue that the protobuf encoding does not know about suffixes. On the flip side, this will decrease the flexibility a bit because users cannot make arbitrary suffixes anymore. This feature is a bit of an illusion anyway because it only works with the text encoding. |
Thanks for looking into this! I think that makes sense. Would you agree with me suggesting that we can do this as a follow up @thomaseizinger? I would like this pull request to make the architectural change only. Breaking individual methods can happen in a one-by-one case later on, ideally in a deprecate-then-remove style in case users already depend on it. |
I suggested it because I think it would really simplify the implementation and thus ease the review of this PR. I get the desire to keep the PR small. Unfortunately, we have to add a lot of code to retain some of the interfaces. I think this PR could be smaller if we just break those. |
Gave this more thought and tested it out. See e030f76. As you predicted, this simplifies things a lot. As always, thanks @thomaseizinger for the great suggestion. There are still a couple of TODOs, but I see light at the end of the tunnel. |
That is great to hear! I think we can do even better though. Let me know what you think of thomaseizinger@d2ad9df. |
Wonderful simplification. Thanks for proposing a patch. Only issue I see is that it makes certain invalid states possible at compile time. See thomaseizinger@d2ad9df#r90329584. |
|
I sent a PR here: mxinden#1 |
Adopt encoding style similar to serde. `EncodeXXX` for a type that can be encoded (see serde's `Serialize`) and `XXXEncoder` for a supported encoding i.e. text and protobuf (see serde's `Serializer`). - Compatible with Rust's additive features. Enabling the `protobuf` feature does not change type signatures. - `EncodeMetric` is trait object safe, and thus `Registry` can use dynamic dispatch. - Implement a single encoding trait `EncodeMetric` per metric type instead of one implementation per metric AND per encoding format. - Leverage `std::fmt::Write` instead of `std::io::Write`. The OpenMetrics text format has to be unicode compliant. The OpenMetrics Protobuf format requires labels to be valid strings. Signed-off-by: Max Inden <mail@max-inden.de>
|
Is there a reason this is adopting a serde style rather than directly using Serde (#97)? The motivation behind that would be that serde already has a large ecosystem behind it, with plenty of helpers to construct Serialize implementations like |
|
Not every type that implements Maybe we could use serde internally but even that is tricky because protobuf doesn't map cleanly to serde so I am struggling to see the benefit! How do you expect the ecosystem support of serde to benefit this prometheus client implementation? |
|
That makes sense, thanks for the explanation. My main interest was from more advanced |
|
Each Prometheus metric encoding would map to a unique Serde data format, all distinct from e.g. raw Protobufs or whatever. Only a well-defined set of encodings (data formats) would be acceptable representations of metrics. One couldn't |
Can you please explain further what you are trying to achieve? Like, post the type you are trying to write (with serde annotations) and the corresponding prometheus text encoding that you are expecting. |
|
Sure. Sorry I was vague, I didn't want to derail the PR too much. Basically
the issue is we have a bunch of metrics which all share the same set of ~15
common labels, but also have some metric-specific ones. It would be nice to
have a shared common struct for the common labels, but the nesting causes
issues.
```rust
struct Common {
field_a: String,
field_b: String,
field_c: String
}
struct Metric1 {
unique_field: String
#[serde(flatten)]
common: Common
}
struct Metric2 {
unique_field_2: String
#[serde(flatten)]
common: Common
}
```
Desired metrics:
```
metric1{field_a="foo", field_b="bar", field_c="baz",
unique_field="something"}
metric2{field_a="foo", field_b="bar", field_c="baz",
unique_field_2="something_else"}
```
…On Tue, Dec 6, 2022 at 5:48 AM Thomas Eizinger ***@***.***> wrote:
That makes sense, thanks for the explanation. My main interest was from
more advanced derive support; in particular I want #[serde(flatten)] for
now, but possibly other customizations in the future. While these could be
solved by alternative mechanisms, it seemed nice to just piggy-back on Serde
Can you please explain further what you are trying to achieve? Like, post
the type you are trying to write (with serde annotations) and the
corresponding prometheus text encoding that you are expecting.
—
Reply to this email directly, view it on GitHub
<#105 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEYGXJVUSOFEJPNGMPGWUTWL472XANCNFSM6AAAAAARGMWH5U>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Okay, thanks for sharing this. It makes sense that we support something like this but we can't do it via With the features introduced in this PR, it will be possible to implement a |
|
In real world we have about 10 metrics with 15 common labels so it ends up being a bit of duplication. Anyhow, I don't want to derail this PR. Thanks for the help and explaination, I'll open an issue to track adding a flatten attribute. |
That would be my preference as well. @howardjohn let's discuss how to proceed once you created the issue. |
|
I will merge here. I tested this in conjunction with #82 on https://github.com/mxinden/kademlia-exporter/. While #82 still needs some work, this is ready to go. I suggest we cut an |
|
I opened #117 to discuss further. Thanks everyone. I may be able to submit a PR myself if I can find time + figure it out |
Adopt encoding style similar to serde.
EncodeXXXfor a type that can beencoded (see serde's
Serialize) andXXXEncoderfor a supported encoding i.e.text and protobuf (see serde's
Serializer).protobuffeature doesnot change type signatures.
EncodeMetricis trait object safe, and thusRegistrycan use dynamic dispatch.EncodeMetricper metric type instead ofone implementation per metric AND per encoding format.
std::fmt::Writeinstead ofstd::io::Write. The OpenMetrics textformat has to be unicode compliant. The OpenMetrics Protobuf format requires
labels to be valid strings.
Builds on top of #100.
Status: It compiles. Still a couple of rough edges. Tests don't yet compile.