MQE: Add support for histogram_quantile #9929

jhesketh · 2024-11-18T00:36:17Z

Also preps support for more classic histogram functions to come. (Will require some re-work, but the basics are there).

Tidies up annotation tests and checks their results between engines. (Since sometimes we emit annotations with results, and sometimes the results are omitted when there is an annotation).

What this PR does

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

Also preps support for more classic histogram functions to come. (Will require some re-work, but the basics are there). Tidies up annotation tests and checks their results between engines. (Since sometimes we emit annotations with results, and sometimes the results are omitted when there is an annotation).

charleskorn

Still working my way through the implementation and will keep going tomorrow - I have some suggestions for the tests in the meantime

pkg/streamingpromql/engine_test.go

charleskorn · 2024-11-18T06:02:20Z

pkg/streamingpromql/engine_test.go

@@ -1836,11 +1836,82 @@ func (t *timeoutTestingQueryTracker) Close() error {
 	return nil
 }

-func TestAnnotations(t *testing.T) {


Is there a reason why you've split this test in half?

I like shorter tests and it felt like a good place to split it up since I wanted a function to just test the histogram annotations etc. Happy to rejoin though if you disagree.

I'm not entirely against it, but it is going to cause a bunch of merge conflicts with the upcoming Prometheus 3 changes (which introduce a bunch of new annotations) unless I rebase those.

Perhaps we can keep things as they were in this PR and then split the test in a later PR?

charleskorn · 2024-11-18T06:04:35Z

pkg/streamingpromql/engine_test.go

+					// If both results are available, compare them (sometimes we skip prometheus)
+					if len(results) == 2 {
+						// We do this extra comparison to ensure that we don't skip a series that may be outputted during a warning
+						// or vice-versa where no result may be expected etc.
+						testutils.RequireEqualResults(t, testCase.expr, results[0], results[1])
+					}


Do we need to compare the results when we're testing annotations? I'd expect the results to be exercised elsewhere.

We already have the results, so we may as well compare them.
The main reason though is that some annotations stop the output, and some don't. We want to make sure we are doing that consistently.
Yes, this is likely checked elsewhere, but I'm always in favour of extra checks and balances personally. We also have some annotation tests that are difficult to check elsewhere so it may be easy to miss something.

charleskorn · 2024-11-18T06:06:49Z

pkg/streamingpromql/testdata/ours/classic_histograms.test

Could you please add a test for the case where the buckets for an output series change over time (eg. at T=1, buckets are 1, 2 and 5, but at T=2, buckets are 1, 3 and 7).

I'm not sure what you mean by output series here sorry?

Ah sorry: let's say the input series are:

metric{env="test", le="1"} metric{env="test", le="2"} metric{env="test", le="3"} metric{env="test", le="5"} metric{env="test", le="7"}

Then these all map to the one output series, {env="test"}.

charleskorn

Nice work 🙂

I'd like to see some benchmark results for this.

charleskorn · 2024-11-18T23:02:38Z

pkg/streamingpromql/operators/functions/histograms.go

+	memoryConsumptionTracker *limiting.MemoryConsumptionTracker
+	timeRange                types.QueryTimeRange
+
+	Annotations            *annotations.Annotations


Is there a reason why this is exported?

charleskorn · 2024-11-18T23:04:21Z

pkg/streamingpromql/operators/functions/histograms.go

+type bucketGroup struct {
+	groupedMetricName    string           // The metric name of the group. May be duplicate between groups.
+	pointBuckets         []buckets        // Buckets for the grouped series at each step
+	nativeHistograms     *[]promql.HPoint // Histograms should only ever exist once per group


Why is this a pointer to a slice?

charleskorn · 2024-11-18T23:09:29Z

pkg/streamingpromql/operators/functions/histograms.go

+		// Each series belongs to two groups
+		seriesGroupPair := make([]*bucketGroup, 2)
+
+		// Store the le label. If it doesn't exist, it'll be an empty string


Does it matter if le is present but empty? (vs. not present)

charleskorn · 2024-11-18T23:10:40Z

pkg/streamingpromql/operators/functions/histograms.go

+			lb := labels.NewBuilder(series.Labels)
+			g.labels = lb.Labels()


Why not just set g.labels = series.Labels?

charleskorn · 2024-11-18T23:12:26Z

pkg/streamingpromql/operators/functions/histograms.go

+}
+
+type bucketGroup struct {
+	groupedMetricName    string           // The metric name of the group. May be duplicate between groups.


What if we store the input series index, rather than the metric name? Then we can retrieve the metric name from innerSeriesMetricNames when we need it.

charleskorn · 2024-11-18T23:45:04Z

pkg/streamingpromql/operators/functions/quantile.go

+		if a.upperBound < b.upperBound {
+			return -1
+		}
+		if a.upperBound > b.upperBound {
+			return +1
+		}
+		return 0


[nit] This could be simplified to something like a.upperBound - b.upperBound, couldn't it?

charleskorn · 2024-11-18T23:54:03Z

pkg/streamingpromql/operators/functions/histograms.go

+		} else {
+			for _, f := range fPoints {
+				pointIdx := h.timeRange.PointIndex(f.T)
+				g.pointBuckets[pointIdx] = append(


Something to consider, might be something for a follow-up PR: what if we allocated g.pointBuckets[pointIdx] once based on the expected number of buckets? As it stands, we'll keep appending to g.pointBuckets[pointIdx], which may require many expansions of the slice, with all the allocations and copying that entails.

We could assume that if there are any floats present at a point, then all buckets will be present at that point (which should hold true unless the bucket layout changes).

We could also then pre-sort the list of buckets by upperBound, and then directly write to the correct bucket, reducing / eliminating any shuffling required when sorting in bucketQuantile.

The only thing I'm not sure about is how we'd handle the case where some buckets aren't present (eg. because the bucket layout changed mid-query) - we'd need to keep track of which buckets are present somehow.

charleskorn · 2024-11-18T23:56:47Z

pkg/streamingpromql/operators/functions/quantile.go

+		return math.NaN(), false, false
+	}
+	rank := q * observations
+	b := sort.Search(len(buckets)-1, func(i int) bool { return buckets[i].count >= rank })


We should be able to use slices.BinarySearch here.

charleskorn · 2024-11-18T23:57:06Z

pkg/streamingpromql/operators/functions/quantile.go

Are there any tests for the functions in this file that we can copy across from Prometheus as well?

charleskorn · 2024-11-18T23:58:38Z

pkg/streamingpromql/operators/functions/quantile.go

+	return bucket.Lower + (bucket.Upper-bucket.Lower)*(rank/bucket.Count)
+}
+
+// coalesceBuckets merges buckets with the same upper bound.


When does this happen in practice?

jhesketh requested review from stevesg, grafanabot and a team as code owners November 18, 2024 00:36

jhesketh force-pushed the jhesketh/mqe-histogram-quantile branch from 38d1a77 to d293f3a Compare November 18, 2024 00:40

Update comments

b471cea

jhesketh force-pushed the jhesketh/mqe-histogram-quantile branch from d89bbab to 78fc811 Compare November 18, 2024 05:45

Copy in quantile functions instead of exporting them from Prometheus

5abeee0

jhesketh force-pushed the jhesketh/mqe-histogram-quantile branch from 78fc811 to 5abeee0 Compare November 18, 2024 05:46

charleskorn reviewed Nov 18, 2024

View reviewed changes

jhesketh added 3 commits November 18, 2024 19:54

Fix lint

c2ac252

Address review feedback

bb84e60

Fix tests

016d546

jhesketh force-pushed the jhesketh/mqe-histogram-quantile branch from c20de2b to 016d546 Compare November 18, 2024 10:01

charleskorn reviewed Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MQE: Add support for histogram_quantile #9929

MQE: Add support for histogram_quantile #9929

jhesketh commented Nov 18, 2024

charleskorn left a comment

charleskorn Nov 18, 2024

jhesketh Nov 18, 2024 •

edited

Loading

charleskorn Nov 18, 2024

charleskorn Nov 18, 2024

jhesketh Nov 18, 2024

charleskorn Nov 18, 2024

jhesketh Nov 18, 2024

charleskorn Nov 18, 2024

charleskorn left a comment

charleskorn Nov 18, 2024

charleskorn Nov 18, 2024

charleskorn Nov 18, 2024

charleskorn Nov 18, 2024

charleskorn Nov 18, 2024

charleskorn Nov 18, 2024

charleskorn Nov 18, 2024

charleskorn Nov 18, 2024

charleskorn Nov 18, 2024

charleskorn Nov 18, 2024

		lb := labels.NewBuilder(series.Labels)
		g.labels = lb.Labels()

MQE: Add support for histogram_quantile #9929

Are you sure you want to change the base?

MQE: Add support for histogram_quantile #9929

Conversation

jhesketh commented Nov 18, 2024

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

charleskorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhesketh Nov 18, 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

charleskorn 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhesketh Nov 18, 2024 •

edited

Loading