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

Switch from protobuf to prost #387

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

Switch from protobuf to prost #387

wants to merge 12 commits into from

Conversation

Folyd
Copy link
Contributor

@Folyd Folyd commented Jan 26, 2021

Related issue: #384

Signed-off-by: Folyd <[email protected]>
@Folyd Folyd changed the title WIP: Switch from protobuf to prost Switch from protobuf to prost Jan 30, 2021
Folyd added 2 commits February 7, 2021 11:38
…t the code having to be checked into git.

Signed-off-by: Folyd <[email protected]>
@hdost
Copy link

hdost commented Feb 9, 2021

Looks good, but you may need to rebase.

@hdost
Copy link

hdost commented Feb 10, 2021

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

Copy link
Contributor

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -31,7 +31,9 @@ impl Encoder for ProtobufEncoder {
for mf in metric_families {
// Fail-fast checks.
check_metric_family(mf)?;
mf.write_length_delimited_to_writer(writer)?;
let mut buf = vec![];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if !help.is_empty() {
let name = match &mf.name {
Some(v) => &**v,
None => "",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.r#type
.map(MetricType::from_i32)
.flatten()
.unwrap_or_default();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Thanks.

MetricType::Counter => {
let value = match &m.counter {
Some(v) => v.value.unwrap_or_default(),
None => 0.0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Folyd commented Feb 10, 2021

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

mxinden commented Feb 17, 2021

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?

Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

4 participants