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

Protobuf support #83

Merged
merged 75 commits into from
Sep 11, 2022
Merged

Protobuf support #83

merged 75 commits into from
Sep 11, 2022

Conversation

ackintosh
Copy link
Contributor

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 {
                // ...
            }
        }

ackintosh and others added 30 commits August 30, 2022 08:29
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

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Build error below occurs:

error[E0207]: the lifetime parameter `'a` is not constrained by the impl trait, self type, or predicates
   --> src/encoding/proto.rs:199:6
    |
199 | impl<'a, N, A> EncodeMetric for Counter<N, A>
    |      ^^ unconstrained lifetime parameter

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

error[E0207]: the lifetime parameter `'a` is not constrained by the impl trait, self type, or predicates
   --> src/encoding/proto.rs:199:6
    |
199 | impl<'a, N, A> EncodeMetric for Counter<N, A>
    |      ^^ unconstrained lifetime parameter

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

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
`not found in this scope` error occurs when no feature specified

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
…theus_client::encoding`

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
@ackintosh
Copy link
Contributor Author

  • Merged the two derive crates
  • Fixed missing docs
  • Fixed clippy warnings
  • Moved Encode trait
  • Added a changelog entry and bumped up the minor version

@mxinden Please have another look. 🙏

@mxinden
Copy link
Member

mxinden commented Sep 7, 2022

Thanks for the follow-ups. I kicked off CI, which is complaining. Can you take a look?

@divagant-martian
Copy link
Contributor

most if not all the problems are tackled in ackintosh#7

quote! {
let mut label = prometheus_client::encoding::proto::Label::default();
label.name = #ident_string.to_string();
label.value = format!("{}", self.#ident);
Copy link
Contributor

Choose a reason for hiding this comment

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

This imposes a Display requirement on the fields of a struct. Is this actually needed? if so it would be good to document it somewhere because it's unexpected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing it out. I'm working on that in ackintosh#9.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ackintosh#9 has been merged into this PR.

@ackintosh
Copy link
Contributor Author

Ahh, Signed-off-by is required...

Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
- `assert!(false, ..)` should probably be replaced
- redundant clone
- field assignment outside of initializer for an instance created with Default::default()
Remove a `Display` requirement and fix CI errors
@ackintosh
Copy link
Contributor Author

@mxinden @divagant-martian

Removed a Display requirement from the derive macro (8baf256) and fixed CI errors. I checked that all checks have passed on my repo. ackintosh#9

Copy link
Member

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

Wonderful work. I will cut a release soonish.

Thank you @ackintosh for tackling this large project!


### Changed

- Move`Encode` trait from `prometheus_client::encoding::text` to `prometheus_client::encoding`. See [PR 83].
Copy link
Member

Choose a reason for hiding this comment

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

🙏

@mxinden mxinden merged commit ffc2ab6 into prometheus:master Sep 11, 2022
@ackintosh ackintosh deleted the protobuf branch September 11, 2022 21:27
@ackintosh
Copy link
Contributor Author

Thank you so much for your help! ✨ @mxinden @divagant-martian

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.

Custom encoder?
3 participants