seastar
seastar copied to clipboard
scylladb
[WIP] treewide: bring back prometheus protobuf support
the purpose of this change is to see if the recent test failures can be reproduced with the protobuf change.
This reverts commit 8ed9771ae98130d5adbccdaea4ecbac738170939. also adapts to the latest master HEAD of Seastar
quote from https://prometheus.io/docs/instrumenting/exposition_formats/#historical-versions
However, beginning with Prometheus v2.40, there is experimental support for native histograms, which – at least in its initial experimental state – utilizes the old Protobuf format (with some newer additions) again. Therefore, a very recent Prometheus server with the experimental native histogram support enabled, will again be able to ingest the Protobuf format. This support is experimental and might get removed again.
Fixes #1510 Signed-off-by: Kefu Chai [email protected]
we need to install libprotobuf-dev and protobuf-compiler into the docker image to prepare for this change. these two new dependencies are already added to install-dependencies.sh, so we just need to rerun the script, and update the seastar-toolchain docker image.
hi @avikivity could you please help reimage the docker image with the updated install-dependencies.sh? @amnonh just ping'ed me offline seeking for help on progress this change.
BTW, we could piggyback https://github.com/scylladb/seastar/pull/1630 with this update.
@amnonh hi Amnon, could you help review this change before the docker image is ready?
Since it's experimental, we should wait until it's generally available. Otherwise a change in the protocol could create incompatibility.
@avikivity there's no risk in adding the option to compile with protobuf, I'm doing that already when testing a Prometheus native histogram support. Even adding native histogram support has little risk if we keep it back-to-back experimental in our code base, e.g. add it but state that it's experimental. Prometheus needs to explicitly ask for protobuf to get it. So in a normal setup, the code will not be used.
It has a potential performance improvement in the Prometheus server with large clusters. Which by itself has a monetary impact.
It's not enough to state it's experimental.
- deploy a Seastar application with this code
- Prometheus changes the protocol and graduates the feature to GA
- User deploys new Prometheus
- Confusion
If something is experimental, it needs to be default-off on this side, not somewhere that's not controlled by the user.
It's not enough to state it's experimental.
If something is experimental, it needs to be default-off on this side, not somewhere that's not controlled by the user.
So, first the question, can we compile protobuf, I think it's safe adding the libraries to the toolchain and adding the potential support.
About keeping the protobuf behind an experimental command line flag, I can do that, it totally makes sense.
It's not enough to state it's experimental. If something is experimental, it needs to be default-off on this side, not somewhere that's not controlled by the user.
So, first the question, can we compile protobuf, I think it's safe adding the libraries to the toolchain and adding the potential support.
About keeping the protobuf behind an experimental command line flag, I can do that, it totally makes sense.
Yes that would be sufficient. If the user opts in, they are responsible to make sure everything is compatible.
LGTM, there is a followup work to bring the protobuf to the same level the text representation is, but that's on me, once this PR is merged.
@avikivity, if you will merge this PR, I will send the follow-up patches with the experimental flag support (either explicitly like you wrote in your comment, or following Prometheus' implicit awkward approach of adding a support native histogram flag that also enables binary protocol)
@avikivity, if you will merge this PR, I will send the follow-up patches with the experimental flag support (either explicitly like you wrote in your comment, or following Prometheus' implicit awkward approach of adding a support native histogram flag that also enables binary protocol)
Please adopt it and add the experimental flag at least, I don't want to merge a potentially-broken thing.
I've created PR #1818 with an additional commit that disable protbuf by default, note that prometheus gets its configuration from a config object and not directly from file