protobuf: move from OpenMetrics to Prometheus proto#298
protobuf: move from OpenMetrics to Prometheus proto#298howardjohn wants to merge 4 commits intoprometheus:masterfrom
Conversation
Signed-off-by: John Howard <john.howard@solo.io>
Our goal is to have our builds compile without external dependencies. `protox` helps achieve this. Signed-off-by: John Howard <john.howard@solo.io>
Signed-off-by: John Howard <john.howard@solo.io>
Signed-off-by: John Howard <john.howard@solo.io>
howardjohn
force-pushed
the
proto/native-prometheus
branch
from
8531d9d to
63ccfdb
Compare
krisztianfekete
left a comment
krisztianfekete
left a comment
There was a problem hiding this comment.
I think all here is fine as the foundational work to enable Native Histograms implementation in follow-up PRs, and would love to see this move forward. Added one comment about the changelog.
| - Updated `prost`, `prost-build`, and `prost-types` dependencies to `v0.14`. | ||
| - The `protobuf` feature now generates and encodes Prometheus | ||
| `io.prometheus.client` protobuf messages from `metrics.proto` rather than the | ||
| OpenMetrics protobuf data model. This is a breaking change for users of the `protobuf` feature. |
There was a problem hiding this comment.
I think we should call out the most important change, namely the fact that metrics names will change.
brancz
left a comment
brancz
left a comment
There was a problem hiding this comment.
lgtm
Looking forward to unlocking native histograms!
|
(while I can merge, I'd feel more comfortable if @mxinden is the one who hits the merge button) |
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Thanks for the context through prometheus/docs#2836.
This library is explicitly build in a way to support many encoding formats. Instead of replacing the Open Metrics Protobuf format with the Prometheus Protobuf format, this pull request could as well add the Prometheus Protobuf format as a third encoding format, and mark the Open Metrics Protobuf format as #[deprecated]. In consecutive releases we can then remove the Open Metrics Protobuf format entirely. I assume this to be a much better user experience. @howardjohn what do you think? Would this be a large effort? Would this be worth the additional complexity.
As per protox, that looks like a good quality of life improvement.
I am sorry for the late review. I am no longer using the library myself and mostly focus on Firefox these days. Thus I think it would make sense for me to step down as a maintainer, handing it over to one or more of all of you fine folks. I will follow-up with a mail to the Prometheus mailing list.
|
I'm good either way on adding or replacing. Adding seems to make sense, I just went with replacing based on #296 (comment) (might have misinterpreted). I can switch it over to adding next week |
4 participants
Fixes #296
Replaces #294
Replaces #295
(I can split 294 and 295 out if desired just thought it was easier to handle all in one go)