api: support native histograms in api/v1/metrics#611
api: support native histograms in api/v1/metrics#611beorn7 merged 2 commits intoprometheus:masterfrom
Conversation
beorn7
left a comment
There was a problem hiding this comment.
This looks perfect.
Could you amend the test in api_test.go so that it exercises a native histogram (or maybe even two, an integer one and a float one).
|
A marginally related thing: The protobuf format supports It should be easy to add the support for both classic and native histograms, now that you are on it… |
|
And another thought: IIRC this API is only used for the UI. (There might be users that use it for something else, but I would rather expect them to use the /metrics output directly.) In the UI, I think we should aim for a representation like in the Prometheus UI, which is an explicit description of the individual buckets with absolute population, upper and lower bound. We could argue that we don't need to do this conversion in the UI code because we already have an API exclusively serving the UI, so that API could already render this representation, in the most extreme case just as a string, as created by the histogram's String() method. This would simplify the UI code. A less extreme approach would be to let the API render the buckets individually, but with upper/lower bound and absolute population (similar to the Prometheus query API). This would still allow a reasonably simple UI code, while the API output would still lend itself to some data processing. The approach so far in this PR is also fine, but it would mean that the UI would show the "internal" histogram representation (with delta population, schema, spans), or you would need to recreate a lot of histogram logic in the UI code. |
|
After more thinking: I keep talking about the UI above, because I have forgotten again that we still don't use the API to render the UI in the PGW (cf. #596). While we do have a vague plan to eventually use the API to render the UI, the current usage pattern of the API are more relevant for the decision how to format native histograms. However, I'm not sure what those usage patterns are. If users want a representation close to the exposition format or close to the internal representation in the TSDB, the current approach in this PR is a good fit. However, if users use the API to render some kind of UI themselves, a representation closer to what the Prometheus query API is doing would be much better. The latter would also help with that future plan of rendering the PGW UI based on the API response. My gut feeling is we should do the latter, but I could easily be convinced otherwise. For reference, this is the format in the Prometheus query API: https://prometheus.io/docs/prometheus/latest/querying/api/#native-histograms |
797c3b8 to
6630714
Compare
|
@beorn7 I added code for human readable histograms. While I fix the tests, I was wondering about a few things:
|
6630714 to
04864ea
Compare
|
Thank you very much. I think the naming or layout of the histogram package is not critical. It can be easily refactored if needed. I think what you did serves its purpose. However, the string formatting for a whole bucket in Here is a truncated example from a Prometheus query API response: "buckets": [
[
1,
"-0.0000152587890625",
"-0.00000762939453125",
"42"
],
[
1,
"-0.000003814697265625",
"-0.0000019073486328125",
"2"
],
[
0,
"9.5367431640625e-07",
"0.0000019073486328125",
"145"
],
[
0,
"0.000003814697265625",
"0.00000762939453125",
"11"
],
]I think this is more useful than your "full string" approach because it allows users that are interested in the data (rather than just rendering a UI) to easily access bucket boundaries and bucket counts. (For example, Grafana is using this format to render heatmaps based on Prometheus histograms.) At the same time, it will be very easy to render a UI from that. With a few line of JavaScript or whatever you are using in your UI, you can render what You can look at the code that lives over in prometheus/prometheus to see how this is rendered: (It's using jsoniter, so not directly re-usable, but it can still serve as inspiration.) |
|
Ah my apologies, I totally missed that aspect. Will update, thank you! |
04864ea to
df429a2
Compare
Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
df429a2 to
7396f67
Compare
|
Apologies for the long lag yet again. |
| return ret | ||
| } | ||
|
|
||
| func GetAPIBuckets(h *model.Histogram) []APIBucket[uint64] { |
There was a problem hiding this comment.
This and GetAPIFloatBuckets are exported function since I intend to use them for the web ui changes.
|
Looks great at first glance. I'll have a detailed look in the coming week. |

Another piece for #515.
An example return: