-
Notifications
You must be signed in to change notification settings - Fork 480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
api: support native histograms in api/v1/metrics #611
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Signed-off-by: Jan Fajerski <[email protected]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you.
Another piece for #515.
An example return: