Protobuf support

[ackintosh]

Closes #46

📝 Todos

• [x] Implement encoding
• [x] Implement unit tests
• [x] Documentation
• [x] Fix how to manage the dependencies of .proto files (noted below)
• [x] Implement procedural macro with reference to derive-text-encode
• [x] Benchmark code
• [ ] ~~Integration test on lighthouse~~
  • I found out that we need another enhancement for this client in order to test on lighthouse. (https://github.com/sigp/lighthouse/issues/2956#issuecomment-1086415339)
  • So I wouldn't (can't) test on lighthouse at this time.

FYI: I'm working to use this Protobuf support, in sigp/gossipsub-testground#4, in order to record metrics whose gossipsub-libp2p to InfluxDB.

---

(Edit: Resolved) How to manage the dependencies of .proto files

This PR introduces a openmetrics_data_model.proto file dependency. We can get the file from here.

For now, I've copied the .proto file manually and added it to git. However, I think that is not good as it could happen operational errors. So we need to consider how to manage the .proto file.

1. Using Buf

• Doc: https://docs.buf.build/tour/add-a-dependency#depend-on-googleapis
• This looks good, I've just read the doc though.
• However, in order to use Buf as dependency management, openmetrics_data_model.proto file should be pushed to BSR, which is a proto file registry.
  • There's not the proto file in BSR: https://buf.build/OpenObservability 🙅‍♂️

2. Download the proto file via build.rs

• Instead of using something tools like Buf, we can download the proto file via build.rs.
• For example: https://github.com/sigp/lighthouse/blob/stable/common/deposit_contract/build.rs#L93-L95
  • download the file from the GitHub repository, with a specific commit hash
  • verify the checksum of the file downloaded

~~I couldn't find another way, I think 2. Download the proto file via build.rs is good.~~

Edit: I changed my mind, manually coping is suitable: https://github.com/prometheus/client_rust/pull/47#issuecomment-1100676043

[ackintosh]

For now I get the compile error:

error[E0277]: the trait bound `Box<dyn text::SendEncodeMetric>: proto::EncodeMetric` is not satisfied
   --> src/encoding/proto.rs:151:33
    |
151 |         println!("{:?}", encode(&registry));
    |                          ------ ^^^^^^^^^ the trait `proto::EncodeMetric` is not implemented for `Box<dyn text::SendEncodeMetric>`
    |                          |
    |                          required by a bound introduced by this call
    |
    = help: the following implementations were found:
              <Box<(dyn proto::EncodeMetric + 'static)> as proto::EncodeMetric>
              <Box<(dyn proto::SendEncodeMetric + 'static)> as proto::EncodeMetric>
note: required by a bound in `proto::encode`
   --> src/encoding/proto.rs:13:8
    |
11  | pub fn encode<M>(registry: &Registry<M>) -> openmetrics_data_model::MetricSet
    |        ------ required by a bound in this
12  | where
13  |     M: EncodeMetric,
    |        ^^^^^^^^^^^^ required by this bound in `proto::encode`

---

error[E0277]: the trait bound Box<dyn text::SendEncodeMetric>

This error message refers to SendEncodeMetric trait in the "text", not "proto".

I think, it seems this definition of Registry somehow could relates the error.

I'm not sure how solve the compile error. 🤔

@mxinden I would appreciate it if you could give me some advice. 🙇

[mxinden]

@mxinden I would appreciate it if you could give me some advice. bow

https://github.com/ackintosh/client_rust/pull/1 should fix your issue @ackintosh. Let me know if that works for you.

[ackintosh]

It works, thank you @mxinden !

[mxinden]

Please ping me once this is ready for a first review @ackintosh. Thanks for the help!

[ackintosh]

Most of what I need to implement are finished, there are small (maybe) TODO tasks though.

@mxinden

• Please review this PR.
• I would appreciate your thoughts/suggestions/alternatives about How to manage the dependencies of .proto files. 🙏

[mxinden]

This is great news @ackintosh. Unfortunately I will be offline for a bit, thus I will likely not get to it next week :/

[mxinden]

I couldn't find another way, I think 2. Download the proto file via build.rs is good. memo

I would like the prometheus crate to be able to build offline, i.e. once one downloaded all dependencies from crates.io to not require internet access. Downloading the file on each build would thus not work.

[mxinden]

Using Buf

This is an interesting suggestion. What do you think of moving it to https://github.com/OpenObservability/OpenMetrics/ as it concerns the whole project?

[mxinden]

For now, I've copied the .proto file manually and added into git.

I don't expect any breaking changes to the .proto file, and thus think this is a valid option.

[ackintosh]

Using Buf

This is an interesting suggestion. What do you think of moving it to https://github.com/OpenObservability/OpenMetrics/ as it concerns the whole project?

💡 I posted an issue to OpenMetrics just now: https://github.com/OpenObservability/OpenMetrics/issues/253

[ackintosh]

I couldn't find another way, I think 2. Download the proto file via build.rs is good. memo

I would like the prometheus crate to be able to build offline, i.e. once one downloaded all dependencies from crates.io to not require internet access. Downloading the file on each build would thus not work.

Ahh, that makes sense. 💡

[ackintosh]

FYI: I'm planning that use the Protobuf support, in gossipsub-testground, in order to record metrics whose gossipsub-libp2p to InfluxDB.

[ackintosh]

@mxinden I have added two tests and some fixes, in anticipation of use in libp2p-gossipsub. In libp2p-gossipsub, Registry treats Family, Counter, and Histogram.

Tests added:

1. encode_family_counter_histogram
2. encode_family_and_counter_and_histogram

However, the second test (encode_family_and_counter_and_histogram) is failing and I'm not sure how fix it. 🤔 I'm tackling this, I would appreciate it if you could help me with this. 🙏

[ackintosh]

Btw, I'm doing a benchmark and comparing iter version and vec version. I'll share you result of it later.

[ackintosh]

@mxinden

Btw, I'm doing a benchmark and comparing iter version and vec version.

I created a benchmark and test (to count allocations) on my personal sandbox repo. We can switch the iter version and vec version to test, some changes are required though.

• Benchmark code
• Test code to count allocations

And compared the results between iter version and vec version:

Benchmark

The iterator version (baseline-iter) is a bit slow.

$ critcmp baseline-iter baseline-vec

group     baseline-iter                          baseline-vec
-----     -------------                          ------------
encode    1.02      6.6±0.06ms        ? ?/sec    1.00      6.4±0.24ms        ? ?/sec

Allocation count

• iter: 124214
• vec: 124114

---

The slowness is related to this proc macro implementation.

As far as I investigated, I think from_generator may improve that but it is a nightly-only experimental API.

Please let me know if you have any ideas. 🙏

[ackintosh]

Now I'm stucking on these issues:

1. https://github.com/prometheus/client_rust/pull/47#issuecomment-1213637603
2. https://github.com/prometheus/client_rust/pull/47#issuecomment-1214564100

Hmm... how about reverting to return a vector? @mxinden

[mxinden]

I took another look. What do you think of https://github.com/ackintosh/client_rust/pull/6?

• It does not require an associated type.
• It does not force any allocations, not even on the implementation of Family.
• It significantly reduces the type signatures, both on the implementor and the consumer side.
• Tests all pass.

What do you think @ackintosh?

(I am sorry for leading you astray suggesting to use an Iterator.)

[ackintosh]

Thank you @mxinden! It's awesome. ✨ I have merged ackintosh#6 and made some updates according to the changes in ackintosh#6.

I run benchmark again in the same way, and the result was definitely good.

Benchmark

$ critcmp baseline-vec baseline-max

group     baseline-max                           baseline-vec
-----     ------------                           ------------
encode    1.00      6.1±0.14ms        ? ?/sec    1.05      6.4±0.24ms        ? ?/sec

Allocation count

• vec: 124114
• now: 114014

---

No worries, the experiments to use an Iterator was so exciting and I have learned a lot from them. 😃

[ackintosh]

TODOs on this PR have been done. ✅

[ackintosh]

https://github.com/prometheus/client_rust/pull/47/checks?check_run_id=8046799148

DCO There are 49 commits incorrectly signed off. This means that the author(s) of these commits failed to include a Signed-off-by line in their commit message.

I need to rebase this branch to add Signed-off-by lines.

[ackintosh]

• Bump the crate version and add a changelog entry https://github.com/prometheus/client_rust/pull/47/commits/250db460857b5ee1baadb4913ae07413968574bb
• Rebased this branch according to the DCO check https://github.com/prometheus/client_rust/pull/47#issuecomment-1229102302

[ackintosh]

Rebased this branch according to the DCO check https://github.com/prometheus/client_rust/pull/47#issuecomment-1229102302

Hmm, I noticed that the commit history was broken due to the rebasing... sorry.

I'll create another pull request and close this. 🙇

[ackintosh]

I have created https://github.com/prometheus/client_rust/pull/83