Native histogram support in proto spec #256
Conversation
The name after the `oneof` doesn't show up in the explicit getter for a particular field, e.g. for the sum of a `HistogramValue` as a double, the getter is called `GetDoubleValue` in Go. Nothing in this getter tells you that you are getting the sum. For one, this is confusing. But you will also get a name collision if any of the other fields becomes a `oneof` (e.g. the count for a float histogram). With this commit, the getter will be called `GetDoubleSum` etc. Signed-off-by: beorn7 <[email protected]>
There are rare use cases of "weighted" or "scaled" histogram, where the total count and the count in buckets are actually floating point values. The need for this "float histogram" becomes more obvious with the introduction of Native Histograms. With Histograms being first class citizens also within Prometheus, a recording rule that outputs Histograms will generally output Histograms where the counts are floating point numbers again (because PromQL operates on floats only, and because the result of a `rate` operation is interpolated and therefore not integral anymore, even if the input is). If such a result of a recording rule is federated, we need to represent "float histograms" in the exposition format. This commit introduces the "float histogram" capability to the conventional Histogram, which is also needed to add Native Histograms later. Signed-off-by: beorn7 <[email protected]>
|
From a very quick glance this looks okay for a future OM 2.0, other than the We may wish to do the renamings of the existing fields now in 1.x, to avoid future hassle when things are harder to change. I'm okay with that in principle as it should be fairly low impact now, however it may count as a breaking change. If the current Go accessors aren't right, we should probably fix that in 1.x though. |
|
Right, |
This is still experimental. It is not clear yet how to represent native histograms in the text format, but with this commit, implementers of OpenMetrics using the protobuf format can already gather experience with native histograms. Signed-off-by: beorn7 <[email protected]>
beorn7
force-pushed
the
beorn7/native-histogram
branch
from
bc81641
to
f8b5441
Compare
|
What's the latest on this? It would be awesome to have native histogram support in proto 💪🏽 |
|
I believe this has everything native histograms need. This seems also be generally correct from the OM side. At least, after the first correction, no other concerns have been voiced. I have marked it as "ready for review" now. However However, this needs some shepherding from the OM side:
The 3rd item refers to the efforts to get native histograms into the OM text format. Paging @csmarchbanks and @SuperQ . |
This commit mirrors prometheus/client_model#80 . Signed-off-by: beorn7 <[email protected]>
The proto file was missing the `go_package` option. Furthermore, the generated Go file was put into the `bin` directory, which is weird. Signed-off-by: beorn7 <[email protected]>
|
@SuperQ @RichiH could you advise how to move this forward? The v1.0.0 protobuf spec is part of https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md , so we could even merge this without changing v1.0.0. But maybe you would like a v1.1 or v2.0 branch or something? Note, however, that the current proto file has naming issues that you might want to fix within v1.0.0 or as v1.0.1 (it's only a change in the generated code, not in the wire format of the protobuf, see above: "… the previous naming of the oneof fields seems like an oversight to me (the author might not have been aware of the resulting naming of the getters, where the top-level name of the oneof block doesn't show up at all, e.g. in Go, the getter for the histogram sum field was called GetDoubleValue previously)." |
|
Proto looks fine to me; I'd prefer to have the same changes in the textual description as well though. Is there a short-form prose which I could adopt into normative language? |
|
The best right now is probably the comments in the proto file (which are also contained in this PR). This is some kind of histogram spec, that I'm currently working on. You could pick from the data model (which is mostly complete) but the part specifically about the exposition format is still work in progress. |
This is marked as WIP because it is not meant as a final proposal yet, merely to demonstrate how it could look like and enable early adopters to play with it and for the reviewers to give feedback.
I am not sure what the stability guarantees are for the proto spec. I believe that these changes should not be a breaking change WRT the wire format, but they do change the API, mostly because of the different naming of
oneoffields and turning the count field of a histogram into aoneof(which would have created a naming collision if I didn't change the naming).In different news, the previous naming of the
oneoffields seems like an oversight to me (the author might not have been aware of the resulting naming of the getters, where the top-level name of theoneofblock doesn't show up at all, e.g. in Go, the getter for the histogramsumfield was calledGetDoubleValuepreviously).