tikv / rust-prometheus

Prometheus instrumentation library for Rust applications
Apache License 2.0
1.07k stars 182 forks source link

Remove or make protobuf dependency optional #210

Closed OtaK closed 5 years ago

OtaK commented 5 years ago

Is your feature request related to a problem? Please describe. As documented here: https://prometheus.io/docs/instrumenting/exposition_formats/ Prometheus > 2.0 does not support Protobuf at all anymore. The protobuf crate is huge when compiled (a bit less than 400KB) and it bloats the binaries that are using rust-prometheus without the protobuf format.

Describe the solution you'd like Publish a breaking version removing the dependency and the Protobuf support altogether

Describe alternatives you've considered It would make sense to not include the protobuf dependency/feature or to make it optional behind a non-default feature for legacy clients.

Additional context 390.6KiB protobuf

siddontang commented 5 years ago

Hi @OtaK

Seem it only removes Protobuf from the exposed metric, but the client still uses it? if yes, we can't remove the Protobuf.

breezewish commented 5 years ago

Thanks for your suggestion. I agree that we can make Protobuf optional (which should be only only needed when "push" feature is on).

OtaK commented 5 years ago

@siddontang Hi! See here: https://prometheus.io/docs/instrumenting/writing_clientlibs/#exposition

Exposition
Clients MUST implement the text-based exposition format outlined in the exposition formats documentation.

Reproducible order of the exposed metrics is ENCOURAGED (especially for human readable formats) if it can be implemented without a significant resource cost.

So yeah, the whole support of protobuf has been removed from Prometheus 2.0 and up.

@breeswish Well, even for pushing metrics Prometheus Pushgateway requires the text format: https://prometheus.io/docs/instrumenting/pushing/

Occasionally you will need to monitor components which cannot be scraped. The Prometheus Pushgateway allows you to push time series from short-lived service-level batch jobs to an intermediary job which Prometheus can scrape. Combined with Prometheus's simple text-based exposition format, this makes it easy to instrument even shell scripts without a client library.
siddontang commented 5 years ago

hi @OtaK

It is very appreciated that you can help us remove it. you can ask @breeswish for help.

OtaK commented 5 years ago

I tried looking around and it seems the whole codebase revolves around the (now deprecated) data structures for the Protobuf protocol. When I tried to make the dep optional the whole crate was erroring in almost all files so I put that work on hold. :/

nagisa commented 5 years ago

Now that protobuf dependency is optional all that is necessary to resolve this issue is to remove protobuf from the list of features enabled by default.

breezewish commented 5 years ago

I think we might keep making protobuf feature enabled by default since it could still be very useful when treating with legacy prometheus. Close this issue since we can optionally disable it.

thomaseizinger commented 2 years ago

I think we might keep making protobuf feature enabled by default since it could still be very useful when treating with legacy prometheus. Close this issue since we can optionally disable it.

Is there a chance to revise this decision? protobuf is an extremely heavy dependency:

Completed protobuf v2.27.1 in 23.2s

Features that are once enabled can never be disabled again and it is likely that a library author that adds metrics to their library forgets to say default-features = false which results in every application having to pay for that even if they are not using the protobuf encoding.