resources: for native histograms show String() output in the UI #596

jan--f · 2023-11-22T14:19:17Z

If a histogram has no classic buckets, assume its a native histogram and render the stringyfied version of it.

Fixes: #515

beorn7 · 2023-11-23T17:45:12Z

resources/template.html

-				<tr>
-					<th scope="row">Sample Sum</th>
-					<td>{{value .GetSampleSum}}</td>
+					<td>{{.String}}</td>


The problem here is that this is just the string representation of the protobuf, which is not very human-readable.

There should be code somewhere already to convert a protobuf histogram to model.histogram.Histogram or model.histogram.FloatHistogram. Maybe we can utilize it to get a display here that looks similar to what we currently get in the Prometheus UI itself?

I'm in a hurry right now, but I can help you to find this conversion code if you cannot find it easily.

No worries, I'll go dig. Thanks for the pointer.

So the code that does this during scraping is unfortunately too tightly coupled with the parser:
https://github.com/prometheus/prometheus/blob/980e2895a2eebb9664af104e6d4e19932cc74432/model/textparse/protobufparse.go#L173-L214

Maybe you could extract a separate ProtoToHistogram(*dto.Histogram) (*histogram.Histogram, *histogram.FloatHistogram) from that code (to be used there and then in the PGW, too).

Or maybe not. In prometheus/prometheus, we use Gogo protobuf, in prometheus/pushgateway, we use the official Go protobuf library. So the dto.Histogram types are actually different.
I would just write the conversion code proto -> histogram.Histogram here in PGW for now. Depending on how long it is and where it is actually used, we can then move it somewhere else if needed.

beorn7 · 2024-04-11T11:50:07Z

Hi @jan--f , with #611 merged, it should just be one more small step to modify this PR to have a (more or less) human-readable histogram representation in the UI. Do you think you can work on this any time soon?

jan--f · 2024-04-11T13:27:30Z

yes indeed, hoping to have an update next week.

jan--f · 2024-04-19T09:42:04Z

Ok better late then never I suppose.

Wdyt @beorn7

beorn7 · 2024-04-23T11:45:56Z

Looks great at first glance. I'm on vacation this week, but this is high on my list once I'm back in action.

beorn7

Maybe I'm missing something, but I think this can be simplified, see comments.

beorn7 · 2024-05-02T15:58:34Z

go.mod

@@ -41,3 +41,5 @@ require (
 	google.golang.org/appengine v1.6.8 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
 )
+
+replace github.com/prometheus/prometheus v0.48.1 => ../prometheus


That's probably a leftover from your local dev environment?

Indeed. Apologies... 🤦

beorn7 · 2024-05-02T16:02:50Z

handler/status.go

+						return fmt.Sprintf("{ count:%v sum:%v %s}",
+							m.GetSampleCountFloat(),
+							m.GetSampleSum(),
+							histogram.BucketsAsString[float64](histogram.GetAPIFloatBuckets(fh)))


Wouldn't return h.String() do the same?

It sure does 👍

beorn7 · 2024-05-02T16:03:52Z

handler/status.go

+						return fmt.Sprintf("{ count:%v sum:%v %s}",
+							m.GetSampleCount(),
+							m.GetSampleSum(),
+							histogram.BucketsAsString[uint64](histogram.GetAPIBuckets(h)))


And here fh.String()?

beorn7 · 2024-05-02T16:04:35Z

handler/status.go

+							m.GetSampleCountFloat(),
+							m.GetSampleSum(),
+							histogram.BucketsAsString[float64](histogram.GetAPIFloatBuckets(fh)))
+					} else {


This else isn't really needed, is it? (You return in the if block above.)

That's the else branch if we have a histogram and not a float histogram.

But since you return in line 81, the current else branch is anyway only taken if h!=nil.

I mean, don't remove the whole block, just remove the } else { and the closing } below and outdent the content of the block.

beorn7 · 2024-05-02T16:05:12Z

histogram/prometheus_model.go

@@ -79,6 +80,32 @@ func BucketsAsJson[BC model.BucketCount](buckets []APIBucket[BC]) [][]interface{
 	return ret
 }

+func BucketsAsString[BC model.BucketCount](buckets []APIBucket[BC]) string {


As said above, I assume you can use the String method of the model histograms, and then you don't need this helper.

beorn7 · 2024-05-02T16:08:42Z

resources/template.html

 			{{- with .Histogram}}
 			<table class="table table-striped table-bordered">
-				{{- range .Bucket}}
+                                {{- if le (len .Bucket) 0}}


So there is the case of a "dual histogram", which has both classic buckets and the sparse buckets of a native histogram, so that the scraper can decide what kind of histogram it wants to ingest (possibly even both). In that case, we should probably display both the classic and the native histogram, maybe with a <th scope="row">Native Histogram representation</th>.

I see, so would then make sense to format classic histograms if there are classic buckets and native histograms if Schema != nil?

Schema is just an int32, so it never can be nil.

I would render a classic histogram if there are classic buckets, and I would render a native histogram if there are native buckets. The only edge case is if there are no buckets at all, but then the rendering of either isn't much of a difference (just sum and count), so you could pick any one rendering for that.

beorn7 · 2024-05-15T16:26:27Z

resources/template.html

 			{{- with .Histogram}}
 			<table class="table table-striped table-bordered">
-				{{- range .Bucket}}
+                                {{- if .Schema }}


Hmm, I guess this essentially tests if .Schema is 0 or not, which is not what we want. (Schema 0 is a perfectly valid schema for native histograms.) As said in another comment, let's simply test if there are native buckets.

beorn7 · 2024-05-15T16:27:12Z

resources/template.html

 				</tr>
+                                {{- end}}
+                                {{- if gt (len .Bucket) 0}}


Or if there are neither native nor classic buckets (to handle the case of a bucket-less histogram).

beorn7 · 2024-05-15T16:27:44Z

Sorry for sprinkling the comments instead of doing them in one review. But I think I have addressed all questions now.

If a histogram has no classic buckets, assume its a native histogram and render a string version of it like Prometheus currently does. Signed-off-by: Jan Fajerski <[email protected]>

jan--f · 2024-05-17T09:31:30Z

Sorry for sprinkling the comments instead of doing them in one review. But I think I have addressed all questions now.

Not at all, thanks a lot for your patience. I think all comments are addressed now.

beorn7

Wonderful. Thanks a lot.

Note that I'll update the commit description to the current state of affairs when merging.

jan--f requested a review from beorn7 November 22, 2023 14:19

jan--f mentioned this pull request Nov 22, 2023

Add support for native histograms #515

Closed

beorn7 reviewed Nov 23, 2023

View reviewed changes

beorn7 mentioned this pull request Jan 18, 2024

api: support native histograms in api/v1/metrics #611

Merged

jan--f force-pushed the ui-histogram-as-string branch from e2ad8a6 to 1a206b5 Compare April 19, 2024 09:40

jan--f force-pushed the ui-histogram-as-string branch from 1a206b5 to 898ee2c Compare April 19, 2024 10:13

beorn7 requested changes May 2, 2024

View reviewed changes

jan--f force-pushed the ui-histogram-as-string branch from 898ee2c to a944780 Compare May 3, 2024 07:17

beorn7 reviewed May 15, 2024

View reviewed changes

resources: naive rendering for native histograms

92ab250

If a histogram has no classic buckets, assume its a native histogram and render a string version of it like Prometheus currently does. Signed-off-by: Jan Fajerski <[email protected]>

jan--f force-pushed the ui-histogram-as-string branch from a944780 to 92ab250 Compare May 17, 2024 09:30

beorn7 approved these changes May 22, 2024

View reviewed changes

beorn7 merged commit c83c5cc into prometheus:master May 22, 2024
4 checks passed

SuperQ mentioned this pull request Jun 7, 2024

Cut v1.9.0 #654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resources: for native histograms show String() output in the UI #596

resources: for native histograms show String() output in the UI #596

jan--f commented Nov 22, 2023

beorn7 Nov 23, 2023

jan--f Nov 24, 2023

beorn7 Nov 28, 2023

beorn7 Feb 2, 2024

beorn7 commented Apr 11, 2024

jan--f commented Apr 11, 2024

jan--f commented Apr 19, 2024

beorn7 commented Apr 23, 2024

beorn7 left a comment

beorn7 May 2, 2024

jan--f May 3, 2024

beorn7 May 2, 2024

jan--f May 3, 2024

beorn7 May 2, 2024

beorn7 May 2, 2024

jan--f May 3, 2024 •

edited

Loading

beorn7 May 15, 2024

beorn7 May 15, 2024

beorn7 May 2, 2024

beorn7 May 2, 2024

jan--f May 3, 2024

beorn7 May 15, 2024

beorn7 May 15, 2024

beorn7 May 15, 2024

beorn7 commented May 15, 2024

jan--f commented May 17, 2024

beorn7 left a comment

resources: for native histograms show String() output in the UI #596

resources: for native histograms show String() output in the UI #596

Conversation

jan--f commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented Apr 11, 2024

jan--f commented Apr 11, 2024

jan--f commented Apr 19, 2024

beorn7 commented Apr 23, 2024

beorn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan--f May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented May 15, 2024

jan--f commented May 17, 2024

beorn7 left a comment

Choose a reason for hiding this comment

jan--f May 3, 2024 •

edited

Loading