promql(histograms): Change sample total calculation for histograms #12609

marctc · 2023-07-28T09:02:59Z

This PR changes the way of number samples are calculated when doing query operations with
histograms.
Added method for FloatHistograms that calculates the size in bytes of the whole struct
and its fields.
Added method that calculates number of samples (4 bytes per sample) of histogram in
HPoint struct.

beorn7

Thank you very much.

Very cool calculation of the actual size of a histogram. However, as explained in the comment, we have to do something less expensive and safer (which ideally still yields the same result).

beorn7 · 2023-08-09T17:20:14Z

promql/value.go

@@ -168,6 +168,21 @@ func (p HPoint) MarshalJSON() ([]byte, error) {
 	return json.Marshal([...]interface{}{float64(p.T) / 1000, h})
 }

+// histogramSamples returns the number of samples representing the histogram.
+// each sample is 4 bytes.
+func (p HPoint) histogramSamples() int {


I would call it more like histogramSize and explain it as "measured in relation to a conventional FPoint i.e. 16 bytes.

beorn7 · 2023-08-09T17:27:42Z

promql/value.go

+// histogramSamples returns the number of samples representing the histogram.
+// each sample is 4 bytes.
+func (p HPoint) histogramSamples() int {
+	return p.H.Size() / 4


I think it should be (p.H.Size() + 8) / 16.

In TotalSamples below, we count each FPoint as "one sample". One FPoint is 16 bytes, a 64bit int timestamp and a 64bit float sample value. So "one sample" so far has been 16 bytes.

For an HPoint, we have the same timestamp (8 bytes), and then a number of bytes in the histogram, the sum of which we need to divide by the 16 bytes we considered "one sample" so far.

beorn7 · 2023-08-09T17:29:05Z

promql/value.go

+}
+
+// totalHistogramSamples returns the total number of samples in the given slice of histograms.
+func totalHistogramSamples(histograms []HPoint) int {


Same naming thoughts as above.

model/histogram/float_histogram.go

beorn7 · 2023-08-15T12:12:43Z

I believe

prometheus/promql/engine.go

Line 1238 in d6e1b1a

ev.currentSamples += len(result)

and

prometheus/promql/engine.go

Line 1241 in d6e1b1a

tempNumSamples += len(result)

need an update, too. Which means we have to dive into the Vector and check if it is a float sample or a histogram sample.

beorn7 · 2023-08-15T12:15:38Z

promql/value.go

@@ -267,7 +282,7 @@ func (m Matrix) String() string {
 func (m Matrix) TotalSamples() int {


The doc comment here should be updated to reflect that histograms are taken into account in a weighted fashion.

beorn7 · 2023-08-15T12:18:22Z

Another code path that needs to change: https://github.com/prometheus/prometheus/blob/d6e1b1acdb1231f5bb694bc482b568ccea2b40af/promql/engine.go#L1339C6-L1339C6

totalSamples += len(s.Floats) + len(s.Histograms)

beorn7 · 2023-08-15T12:24:17Z

And another one:

prometheus/promql/engine.go

Lines 1813 to 1824 in d6e1b1a

    
           if len(mat[i].Floats) > 0 { 
        
           	mat[i].Floats = append(mat[i].Floats, FPoint{ 
        
           		T: ts, 
        
           		F: mat[i].Floats[0].F, 
        
           	}) 
        
           } else { 
        
           	mat[i].Histograms = append(mat[i].Histograms, HPoint{ 
        
           		T: ts, 
        
           		H: mat[i].Histograms[0].H, 
        
           	}) 
        
           } 
        
           ev.currentSamples++

beorn7 · 2023-08-15T12:26:02Z

And another one:

prometheus/promql/engine.go

Lines 1862 to 1869 in d6e1b1a

    
           vec = append(vec, Sample{ 
        
           	Metric: s.Labels(), 
        
           	T:      t, 
        
           	F:      f, 
        
           	H:      h, 
        
           }) 
        
           ev.currentSamples++

beorn7 · 2023-08-15T12:28:55Z

Another one:

prometheus/promql/engine.go

Lines 2034 to 2036 in d6e1b1a

    
           for drop = 0; histograms[drop].T < mint; drop++ { // nolint:revive 
        
           } 
        
           ev.currentSamples -= drop

beorn7 · 2023-08-15T12:39:39Z

Sorry, this whole sample tracking now appears way more complicated than I have anticipated.
There is also the QuerySamples type, which we need to double-check if it is always updated with the weighted sample count. It should also get updates in its doc comments to reflect how histograms are weighted in the sample count.

beorn7 · 2023-09-07T16:18:25Z

Apologies for long delays. Still fighting my way through the review backlog…

beorn7

Very good. I only have a few naming and comment nits, and I think the size calculation needs a fix. See my comments. Maybe I'm missing something.
Otherwise, this looks fine.

Apologies again for the review delay.

beorn7 · 2023-09-20T12:35:41Z

model/histogram/float_histogram.go

+// NOTE: this is only valid for 64 bit architectures.
+func (fh *FloatHistogram) Size() int {
+	// Size of each slice separately
+	posSpanSize := len(fh.PositiveSpans) * 8     // 8 bytes - int64 * 2


int64 * 2 isn't really a Span. It's int32 + uint32. Which still results in 8 bytes, so the number is correct.

beorn7 · 2023-09-20T12:35:50Z

model/histogram/float_histogram.go

+func (fh *FloatHistogram) Size() int {
+	// Size of each slice separately
+	posSpanSize := len(fh.PositiveSpans) * 8     // 8 bytes - int64 * 2
+	negSpanSize := len(fh.NegativeSpans) * 8     // 8 bytes - int64 * 2


beorn7 · 2023-09-20T12:40:34Z

model/histogram/float_histogram.go

+
+	// Total size of the struct
+
+	// fh is 32 bytes


Where are these coming from?

beorn7 · 2023-09-20T12:43:51Z

model/histogram/float_histogram.go

+	// fh.NegativeSpans is 24 bytes
+	// fh.PositiveBuckets is 24 bytes
+	// fh.NegativeBuckets is 24 bytes
+	structSize := 165


If I run the following on my (amd64) machine:

fh := histogram.FloatHistogram{} fmt.Println(unsafe.Sizeof(fh))

I get 136 bytes.

Which makes sense to me. All your numbers above seem fine, except the fh is 32 bytes, which I don't understand. If I add them all up, I get 133 bytes. However, the 1 byte CounterResetHint takes effectively 4 bytes because of alignment in the struct, so 133 + 3 = 136.

So the question is where you got the 32 bytes from at the top?

I honestly can't remember....perhaps when I had the function calculating everything automatically? You are right, I'm getting the same result, changing now.

promql/value.go

beorn7 · 2023-09-20T13:31:16Z

promql/value.go

+	return (p.H.Size() + 8) / 16
+}
+
+// totalHistogramSize returns the total number of samples in the given slice of histograms.


histograms → HPoints

promql/value.go

beorn7 · 2023-09-20T13:33:47Z

promql/value.go

@@ -168,6 +168,23 @@ func (p HPoint) MarshalJSON() ([]byte, error) {
 	return json.Marshal([...]interface{}{float64(p.T) / 1000, h})
 }

+// histogramSize returns the number of samples representing the histogram.


How about: "histogramSize returns the size of the HPoint compared to the size of an FPoint. Or in other words: This HPoint is n times larger than an FPoint, where n is the returned number."

beorn7 · 2023-09-20T13:37:15Z

promql/value.go

+}
+
+// totalHistogramSize returns the total number of samples in the given slice of histograms.
+func totalHistogramSize(histograms []HPoint) int {


I would call this totalHPointSize as it includes the timestamp and the pointer to the actual histogram.

beorn7 · 2023-10-14T18:16:42Z

@marctc is this still on your radar? I think it's very little left to do, and it would be nice to get this in soon.

marctc · 2023-10-16T08:36:36Z

@marctc is this still on your radar? I think it's very little left to do, and it would be nice to get this in soon.

Yep, looking into it today 👍

beorn7

One remaining substantial change (structSize is still 3 bytes off), and otherwise just comment and naming nits. Almost there…

beorn7 · 2023-10-17T16:18:13Z

model/histogram/float_histogram.go

@@ -334,6 +334,33 @@ func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool {
 	return true
 }

+// Size returns the size of the whole fields histogram in bytes.


This should make clear that it includes the pointer to the histogram.

How about "Size returns the total size of the FloatHistogram, which includes the size of the pointer to FloatHistogram, all its fields, and all elements contained in slices."

beorn7 · 2023-10-17T16:18:44Z

model/histogram/float_histogram.go

+	// Total size of the struct
+
+	// fh is 8 bytes
+	// fh.CounterResetHint is 1 byte


But it takes 4 bytes because af alignment.

beorn7 · 2023-10-17T16:20:09Z

model/histogram/float_histogram.go

+	// fh.NegativeSpans is 24 bytes
+	// fh.PositiveBuckets is 24 bytes
+	// fh.NegativeBuckets is 24 bytes
+	structSize := 141


So this should be 144. (I suggested 136 in the last round of review, but I was not aware that your intention is to include the size of the pointer to FloatHistogram).

beorn7 · 2023-10-17T16:22:26Z

model/histogram/float_histogram.go

+
+	// Total size of the struct
+
+	// fh is 8 bytes


Nit: This is a sentence and should end with a period. (Same everywhere else. Generally, I would even finish longer comments that aren't formally sentences with a period, like the "Total size of the struct" above. Comments are plain English and should have punctuation as if you are writing an English text.)

beorn7 · 2023-10-17T16:30:13Z

promql/value.go

@@ -264,10 +296,11 @@ func (m Matrix) String() string {
 }

 // TotalSamples returns the total number of samples in the series within a matrix.
+// It takes into account the number of samples in the histograms using the histogramSize method.


I would just repeat the same sentence as you have done above for func (vec Vector) TotalSamples(), i.e.

// Float samples have a weight of 1 in this number, while histogram samples have a higher // weight according to their size compared with the size of a float sample. // See HPoint.histogramSize for details.

beorn7 · 2023-10-17T16:36:54Z

promql/value.go

+// The total size is calculated considering the histogram timestamp (p.T - 8 bytes),
+// and then a number of bytes in the histogram.
+// This sum is divided by 16, as samples are 16 bytes.
+func (p HPoint) histogramSize() int {


Sorry for more naming nits. But why not call this just size()? It returns the size of the whole HPoint, not just of the histogram that is part of the HPoint.

Signed-off-by: Marc Tuduri <marctc@protonmail.com>

beorn7

Thank you very much. And sorry for this being way more involved that I initially thought.

marctc requested a review from roidelapluie as a code owner July 28, 2023 09:02

marctc force-pushed the query_max_samples_float_histogram branch 2 times, most recently from f60d015 to fd4895f Compare July 31, 2023 11:07

beorn7 requested changes Aug 9, 2023

View reviewed changes

beorn7 reviewed Aug 15, 2023

View reviewed changes

marctc requested review from juliusv and Nexucis as code owners August 24, 2023 14:23

marctc requested a review from beorn7 August 24, 2023 14:23

marctc force-pushed the query_max_samples_float_histogram branch from 380076c to ff69c62 Compare August 24, 2023 14:27

Nexucis removed request for juliusv and Nexucis August 25, 2023 09:24

marctc force-pushed the query_max_samples_float_histogram branch from b9189f0 to a2b2106 Compare August 25, 2023 15:05

beorn7 requested changes Sep 20, 2023

View reviewed changes

marctc force-pushed the query_max_samples_float_histogram branch from cbe95f9 to 27d9497 Compare October 16, 2023 13:05

beorn7 requested changes Oct 17, 2023

View reviewed changes

promql(histograms): Change sample total calculation for histograms

8fededf

Signed-off-by: Marc Tuduri <marctc@protonmail.com>

marctc force-pushed the query_max_samples_float_histogram branch from 27d9497 to 98bb3d0 Compare October 18, 2023 09:52

marctc requested a review from beorn7 October 18, 2023 09:52

marctc added 2 commits October 18, 2023 11:53

PR feedback

af7c31e

Signed-off-by: Marc Tuduri <marctc@protonmail.com>

More periods

1ce066e

Signed-off-by: Marc Tuduri <marctc@protonmail.com>

marctc force-pushed the query_max_samples_float_histogram branch from 98bb3d0 to 1ce066e Compare October 18, 2023 09:53

beorn7 approved these changes Oct 18, 2023

View reviewed changes

beorn7 merged commit 4d50e5d into prometheus:main Oct 18, 2023
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

promql(histograms): Change sample total calculation for histograms #12609

promql(histograms): Change sample total calculation for histograms #12609

marctc commented Jul 28, 2023

beorn7 left a comment

beorn7 Aug 9, 2023

beorn7 Aug 9, 2023

beorn7 Aug 9, 2023

beorn7 commented Aug 15, 2023

beorn7 Aug 15, 2023

beorn7 commented Aug 15, 2023 •

edited

beorn7 commented Aug 15, 2023

beorn7 commented Aug 15, 2023

beorn7 commented Aug 15, 2023

beorn7 commented Aug 15, 2023

beorn7 commented Sep 7, 2023

beorn7 left a comment

beorn7 Sep 20, 2023

beorn7 Sep 20, 2023

beorn7 Sep 20, 2023

beorn7 Sep 20, 2023

marctc Oct 16, 2023

beorn7 Sep 20, 2023

beorn7 Sep 20, 2023

beorn7 Sep 20, 2023

beorn7 commented Oct 14, 2023

marctc commented Oct 16, 2023

beorn7 left a comment

beorn7 Oct 17, 2023

beorn7 Oct 17, 2023

beorn7 Oct 17, 2023

beorn7 Oct 17, 2023

beorn7 Oct 17, 2023

beorn7 Oct 17, 2023

beorn7 left a comment

		@@ -267,7 +282,7 @@ func (m Matrix) String() string {
		func (m Matrix) TotalSamples() int {

promql(histograms): Change sample total calculation for histograms #12609

promql(histograms): Change sample total calculation for histograms #12609

Conversation

marctc commented Jul 28, 2023

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

beorn7 commented Aug 15, 2023

Choose a reason for hiding this comment

beorn7 commented Aug 15, 2023 • edited

beorn7 commented Aug 15, 2023

beorn7 commented Aug 15, 2023

beorn7 commented Aug 15, 2023

beorn7 commented Aug 15, 2023

beorn7 commented Sep 7, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented Oct 14, 2023

marctc commented Oct 16, 2023

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

beorn7 left a comment

Choose a reason for hiding this comment

beorn7 commented Aug 15, 2023 •

edited