Switch from protobuf to prost

[Folyd]

Related issue: #384

[hdost]

Looks good, but you may need to rebase.

[hdost]

@breeswish I think it's in your hands now 😅

[mxinden]

I am in favor of moving to prost. Thanks for putting in all the work.

With the move to prost we seem to loose a lot of type safety, moving many checks, e.g. whether a counter is Some when type Counter is set, from compile time to runtime. The approach taken in this pull request, is to gracefully use the default value. I don't think this is a valid approach as it sacrifices a lot of safety we gain through the type system.

I see two ways forward:

1. Handle each check at runtime properly, returning a Result:Err in case there is a mismatch. While a valid option, I expect this to be very noisy.
2. Introduce an intermediate data representation like suggested in the issue which represents all constraints in the data model itself, thus checked at compile time. This intermediary data representation would be created through Collector::collect and then be used in encoder/pb.rs and encoder/text.rs with the former going from intermediate data representation to the current representation generated by prost.

@Folyd does the above make sense to you? If not, I am happy to extend my expatiation. If it does, let me know what you think.

[mxinden]

Bummer that we need this intermediary buffer. We could change the Encoder trait, requiring writer to also implement BufMut. But that would require a new dependency bytes on our public interface.

[Folyd]

Exactly. Actually, I did change to the BufMut trait in my local branch. I'm haven't submit that commit right now. What do you think? @breeswish @hdost

[breezewish]

Let's keep non-protobuf feature clean.. This immediate buffer only allocate once each encode call and I think the performance seems to be fine.

[mxinden]

Why default to an empty string here? I don't think a metric without a name is valid. I would expect to return an error instead. What do you think?

[Folyd]

This just a simple migration from mf.get_name().

    pub fn get_name(&self) -> &str {
        match self.name.as_ref() {
            Some(v) => &v,
            None => "",
        }
    }

I'm in favor of returning an error. 👌

[mxinden]

I don't think we should default here, but instead skip encoding the TYPE line, or return an error, as we would fail in the match metric_type later on anyways, what do you think?

[Folyd]

Good point. Thanks.

[mxinden]

If metric type is Counter, but m.counter is None, I would expect an error to be thrown instead of silently using 0.0. Am I missing something?

[Folyd]

Simply migration too:

    pub fn get_counter(&self) -> &Counter {
        self.counter.as_ref().unwrap_or_else(|| Counter::default_instance())
    }

Also, I think it's a good choice to return an error.

[Folyd]

Hi @mxinden. Thanks for reviewing.

Introduce an intermediate data representation like suggested in the issue which represents all constraints in the data model itself, thus checked at compile time. This intermediary data representation would be created through Collector::collect and then be used in encoder/pb.rs and encoder/text.rs with the former going from intermediate data representation to the current representation generated by post.

I'm afraid I'm not 100% get what the intermediary data representation means and how it works. Could you please give more detail? Thanks a lot. 😸

[mxinden]

Hi @mxinden. Thanks for reviewing.

Introduce an intermediate data representation like suggested in the issue which represents all constraints in the data model itself, thus checked at compile time. This intermediary data representation would be created through Collector::collect and then be used in encoder/pb.rs and encoder/text.rs with the former going from intermediate data representation to the current representation generated by post.

I'm afraid I'm not 100% get what the intermediary data representation means and how it works. Could you please give more detail? Thanks a lot. smile_cat

The below summarizes the data flow used on master as well as this pull request:

1. Each metric (e.g. GenericCounter) tracks its state internally in some non-consistent memory format.
2. On call to Collector::collect each metric returns its data in the uniform structure Vec<proto::MetricFamily.
3. The Vec<proto::MetricFamily> is passed to Encoder::encode.
   1. The TextEncoder iterates the Vec<proto::MetricFamily>, encodes each and writes the corresponding bytes out to the writer.
   2. The ProtobufEncoder can work with proto::MetricFamily directly, encoding it into the writer.

The proto::MetricFamily data layout differs between protobuf and prost. Namely that the latter makes extensive use of Option. As you noticed in text.rs handling the case of None for each property is cumbersome and error prone, we loose the guarantees that the Rust type system gives us.

My suggestion is to introduce our own data layout which each metric type would return in their implementation of Collector::collect (step 2). This layout would enforce all Prometheus format constraints at compile time (e.g. each MetricFamily having a name thus name: String and not name: Option<String>). TextEncoder could directly work with this format. ProtobufEncoder would need to transform it into the proto::MetricFamily format, which is easy as e.g. going from String to Option<String> requires no handling of the None case.

Does the above make sense to you @Folyd? If so, what do you think?

[breezewish]

About the intermediate data type, I have another idea. Can we implement a MetricFamily that is simply a Vec of all metrics and implement two Encoder (text, protobuf) for this simple MetricFamily? In this way, we no longer need to transform from a memory layout to a plain modal when we use the text format. For the protobuf format, protobuf structs still need to be filled (so that the workflow is memory layouot -> protobuf struct -> serialize). What do you think @mxinden