Protobuf support #47

ackintosh · 2022-02-24T14:22:05Z

Closes #46

This PR adds protobuf support to the client, which provides functionality to encode metrics to an instance of the OpenMetrics protobuf format.

        let mut registry = Registry::default();

        // ...

        // Encode metrics to an instance of the OpenMetrics protobuf format.
        let metric_set: openmetrics_data_model::MetricSet = encode(&registry);

        for family in metric_set.metric_families {
            println!(family.name);

            for metric in family.metrics {
                // ...
            }
        }

📝 Todos

Implement encoding
Implement unit tests
Documentation
Fix how to manage the dependencies of .proto files (noted below)
Implement procedural macro with reference to derive-text-encode
Benchmark code
~~Integration test on lighthouse~~
- I found out that we need another enhancement for this client in order to test on lighthouse. (Replace prometheus client with the official one sigp/lighthouse#2956 (comment))
- 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.

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 🙅‍♂️

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: #47 (comment)

mxinden · 2022-02-28T17:36:32Z

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

ackintosh#1 should fix your issue @ackintosh. Let me know if that works for you.

ackintosh · 2022-03-07T04:24:41Z

It works, thank you @mxinden !

mxinden · 2022-03-14T19:12:47Z

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

ackintosh · 2022-04-07T14:22:23Z

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 · 2022-04-08T19:14:04Z

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

mxinden

Thanks for the work @ackintosh! I have a couple of high-level comments. I will take a more in-depth look.

Cargo.toml

src/encoding/proto.rs

mxinden · 2022-04-16T14:28:34Z

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 · 2022-04-16T14:29:26Z

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 · 2022-04-16T14:29:58Z

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 · 2022-08-06T02:13:09Z

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: prometheus/OpenMetrics#253

ackintosh · 2022-08-06T02:27:09Z

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 · 2022-08-06T02:43:49Z

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

mxinden · 2022-08-20T02:45:59Z

I took another look. What do you think of ackintosh#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 · 2022-08-23T06:37:53Z

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 · 2022-08-23T07:24:14Z

TODOs on this PR have been done. ✅

mxinden

Great progress here.

Would you mind bumping the crate version and adding a changelog entry? As far as I can tell, this does not introduce any breaking changes and could thus be released as v0.18.1.

mxinden · 2022-08-26T05:36:08Z

Cargo.toml


 [dependencies]
 dtoa = "1.0"
 itoa = "1.0"
 parking_lot = "0.12"
+prometheus-client-derive-proto-encode = { version = "0.1.0", path = "derive-proto-encode", optional = true }


Exploring the thought on whether we could use the same derive macro for both text and protobuf. I guess we can not feature flag the generated protobuf code via #[cfg(feature = "protobuf")] as we would not have access to the flag in the downstream user crate?

That is cool if we use the same derive macro, I'll do some experiments. 🙂

I investigated but yeah, I couldn't find a way to do that.

mxinden · 2022-08-26T05:37:07Z

Cargo.toml

@@ -10,14 +10,21 @@ repository = "https://github.com/prometheus/client_rust"
 homepage = "https://github.com/prometheus/client_rust"
 documentation = "https://docs.rs/prometheus-client"

+[features]
+protobuf = ["dep:prost", "dep:prost-types", "dep:prost-build", "dep:prometheus-client-derive-proto-encode", "dep:void"]


I think CI is currently not running with --all-features. Can you add it with this pull request to make sure your feature is being tested?

Added 33c940a

ackintosh · 2022-08-27T02:21:03Z

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.

* build(deps): update dtoa requirement from 0.4 to 1.0 Updates the requirements on [dtoa](https://github.com/dtolnay/dtoa) to permit the latest version. - [Release notes](https://github.com/dtolnay/dtoa/releases) - [Commits](dtolnay/dtoa@0.4.0...1.0.1) --- updated-dependencies: - dependency-name: dtoa dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> * src/encoding/text: Adjust to breaking changes in dtoa Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

) Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

…s#79) Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

…eus#78) Before `proemtheus-client` would use the `owning_ref` crate to map the target of a `std::sync::RwLockReadGuard`. `owning_ref` has multiple unsoundness issues, see https://rustsec.org/advisories/RUSTSEC-2022-0040.html. Instead of replacing `owning_ref` with a similar crate, we switch to locking via `parking_lot` which supports the above mapping natively. Signed-off-by: Max Inden <mail@max-inden.de>

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

void is used in only encoding/proto. Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

# Conflicts: # .github/workflows/rust.yml # CHANGELOG.md # Cargo.toml # src/metrics/gauge.rs

ackintosh · 2022-08-27T02:53:06Z

Bump the crate version and add a changelog entry 250db46
Rebased this branch according to the DCO check Protobuf support #47 (comment)

ackintosh · 2022-08-29T23:23:47Z

Rebased this branch according to the DCO check #47 (comment)

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

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

ackintosh · 2022-08-30T00:43:59Z

I have created #83

This comment was marked as resolved.

Sign in to view

ackintosh mentioned this pull request Mar 20, 2022

Replace prometheus client with the official one sigp/lighthouse#2956

Open

mxinden reviewed Apr 16, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/encoding/proto.rs Outdated Show resolved Hide resolved

src/encoding/proto.rs Outdated Show resolved Hide resolved

src/encoding/proto.rs Outdated Show resolved Hide resolved

ackintosh mentioned this pull request Jul 27, 2022

Add emulation test plan sigp/gossipsub-testground#4

Merged

ackintosh mentioned this pull request Aug 3, 2022

Make encode return iterator instead of vec ackintosh/client_rust#2

Merged

ackintosh force-pushed the protobuf-support branch from 98ffcc2 to 658afbc Compare August 12, 2022 23:38

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

ackintosh marked this pull request as ready for review August 23, 2022 07:18

mxinden reviewed Aug 26, 2022

View reviewed changes

dependabot bot and others added 3 commits August 27, 2022 11:21

src/metrics/gauge: Implement Gauge::dec and Gauge::dec_by (prometheus#30

638d84e

) Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

CHANGELOG: Prepare v0.14.0

132b7f4

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

mxinden and others added 23 commits August 27, 2022 11:24

src/encoding/proto: Refactor EncodeLabel and EncodeMetric

b3d49ce

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Fix assertions because of changes of the order of result

d55296d

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Remove unnecessary lifetime parameter

ff7ada0

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Rename EncodeLabel -> EncodeLabels

42f00e3

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Using Void to indicate the impossible case

6d4f6f6

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Changed proc macro accordingly since encode returns iterator

1c965d4

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Rename the proc macro since the name was a bit redundant

9e83678

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

examples/: Add actix-web binary (prometheus#76)

cbd97ab

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

.github/workflows/: Add wasm32 as a cross-compile target (prometheu…

de0f38f

…s#79) Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Add a test for Family (counter and histogram)

3cff4d6

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Add benchmark code for proto encoding

3548aa4

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Add a test for Family, Counter and Histogram

5af3aff

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Remove Box to reduce allocation count

9649407

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Use std::iter::Once on enums as enum returns single element

680483e

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

encoding/proto: Pass metric and label vec

15ef4e0

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

derive-proto-encode: Update according to the changes in encoding/proto

182d7b0

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

CHANGELOG.md: Prepare v0.18.0 (prometheus#80)

c68c4ba

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Cargo.toml: Make void optional

59624c5

void is used in only encoding/proto. Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

benches: Update according to the changes in encoding/proto

9c41272

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

encoding/proto: same implementation to slice

2d28710

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Bump the crate version and add a changelog entry

250db46

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

Add --all-features

33c940a

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>

ackintosh force-pushed the protobuf-support branch from bcf5567 to 33c940a Compare August 27, 2022 02:28

Merge branch 'master' into protobuf-support

2d98197

# Conflicts: # .github/workflows/rust.yml # CHANGELOG.md # Cargo.toml # src/metrics/gauge.rs

ackintosh mentioned this pull request Aug 30, 2022

Protobuf support #83

Merged

ackintosh closed this Aug 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protobuf support #47

Protobuf support #47

ackintosh commented Feb 24, 2022 •

edited

Loading

This comment was marked as resolved.

mxinden commented Feb 28, 2022

ackintosh commented Mar 7, 2022 •

edited

Loading

mxinden commented Mar 14, 2022

ackintosh commented Apr 7, 2022

mxinden commented Apr 8, 2022

mxinden left a comment

mxinden commented Apr 16, 2022

mxinden commented Apr 16, 2022

mxinden commented Apr 16, 2022

ackintosh commented Aug 6, 2022

ackintosh commented Aug 6, 2022

ackintosh commented Aug 6, 2022

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

mxinden commented Aug 20, 2022

ackintosh commented Aug 23, 2022

ackintosh commented Aug 23, 2022

mxinden left a comment

mxinden Aug 26, 2022

ackintosh Aug 27, 2022

ackintosh Aug 27, 2022

mxinden Aug 26, 2022

ackintosh Aug 27, 2022 •

edited

Loading

ackintosh commented Aug 27, 2022

ackintosh commented Aug 27, 2022

ackintosh commented Aug 29, 2022

ackintosh commented Aug 30, 2022

Protobuf support #47

Protobuf support #47

Conversation

ackintosh commented Feb 24, 2022 • edited Loading

📝 Todos

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

This comment was marked as resolved.

mxinden commented Feb 28, 2022

ackintosh commented Mar 7, 2022 • edited Loading

mxinden commented Mar 14, 2022

ackintosh commented Apr 7, 2022

mxinden commented Apr 8, 2022

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Apr 16, 2022

mxinden commented Apr 16, 2022

mxinden commented Apr 16, 2022

ackintosh commented Aug 6, 2022

ackintosh commented Aug 6, 2022

ackintosh commented Aug 6, 2022

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

mxinden commented Aug 20, 2022

ackintosh commented Aug 23, 2022

Benchmark

Allocation count

ackintosh commented Aug 23, 2022

mxinden left a comment

Choose a reason for hiding this comment

mxinden Aug 26, 2022

Choose a reason for hiding this comment

ackintosh Aug 27, 2022

Choose a reason for hiding this comment

ackintosh Aug 27, 2022

Choose a reason for hiding this comment

mxinden Aug 26, 2022

Choose a reason for hiding this comment

ackintosh Aug 27, 2022 • edited Loading

Choose a reason for hiding this comment

ackintosh commented Aug 27, 2022

ackintosh commented Aug 27, 2022

ackintosh commented Aug 29, 2022

ackintosh commented Aug 30, 2022

ackintosh commented Feb 24, 2022 •

edited

Loading

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

ackintosh commented Mar 7, 2022 •

edited

Loading

ackintosh Aug 27, 2022 •

edited

Loading