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?

I think this will create conflicts regardless. Lets see where we are at when either this or the Prometheus 3 PR's are close to merging.

pkg/streamingpromql/engine_test.go

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"}.

I'm still not sure what you mean by the buckets changing over time sorry? Do you just mean where the output series has labels more than just its __name__?

I'm imagining a test case like this:

load 6m metric{env="test", le="1"} 1 2 metric{env="test", le="2"} 5 _ metric{env="test", le="3"} _ 9 metric{env="test", le="5"} 8 _ metric{env="test", le="7"} _ 20 eval range from 0 to 6m step 6m histogram_quantile(0.5, metric) {env="test"} xxx yyy

charleskorn

Nice work 🙂

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

pkg/streamingpromql/operators/functions/histograms.go

pkg/streamingpromql/operators/functions/quantile.go

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.

It's a good idea, but I think there are some complicated edge cases as you point out. So I agree better for a followup PR.

pkg/streamingpromql/operators/functions/quantile.go

charleskorn

This is looking good.

I'd like to see some benchmark results, and I'd also like for
histogram_quantile to go behind a feature flag so we can turn it off if need be - there's a lot of complexity here, and while I can't see any obvious issues, it'd be good to have that available if we need it.

charleskorn · 2024-11-22T03:14:32Z

pkg/streamingpromql/operators/functions/histograms.go

@@ -219,7 +218,13 @@ func (h *HistogramFunctionOverInstantVector) accumulateUntilGroupComplete(ctx co
 		// It is also possible that both series groups are the same.
 		// The conflict in points is then detected in computeOutputSeriesForGroup.
 		h.saveNativeHistogramsToGroup(s.Histograms, thisSeriesGroups.nativeHistogramGroup)
-		h.saveFloatsToGroup(s.Floats, thisSeriesGroups.bucketValue, thisSeriesGroups.classicHistogramGroup)
+		if thisSeriesGroups.classicHistogramGroup != nil {


If there is no le label, but there are float values for the series, do we need to emit an annotation or do something like that here?

charleskorn · 2024-11-22T03:16:03Z

pkg/streamingpromql/operators/functions/histograms.go

@@ -136,7 +140,7 @@ func (h *HistogramFunctionOverInstantVector) SeriesMetadata(ctx context.Context)
 		if !groupExists {
 			g.labels = series.Labels
 			g.group = bucketGroupPool.Get()
-			g.group.firstInputSeriesIdx = innerIdx
+			g.group.lastInputSeriesIdx = innerIdx


This isn't quite right - we need to update lastInputSeriesIdx both when the group already exists and when it doesn't, but it's currently only set when the group is created for the first time.

Same comment applies for classic histogram case below.

Might be good to add some tests for the sorting logic to catch stuff like this.

pkg/streamingpromql/operators/functions/quantile.go

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

jhesketh added 6 commits November 20, 2024 22:16

Address review feedback (part 1)

c2c54ac

Add upstream quantile tests

2084871

Address review feedback (part 2)

853312b

Use seriesGroupPair struct

c3ef405

Only set classicHistogramGroup if le exists

0ecdfd6

Sort the output groups

91734fb

charleskorn reviewed Nov 22, 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

jhesketh Nov 20, 2024

charleskorn Nov 18, 2024

jhesketh Nov 18, 2024

charleskorn Nov 18, 2024

jhesketh Nov 20, 2024

charleskorn Nov 22, 2024

charleskorn left a comment

charleskorn Nov 18, 2024

jhesketh Nov 20, 2024

charleskorn left a comment

charleskorn Nov 22, 2024

charleskorn Nov 22, 2024

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

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

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

jhesketh Nov 18, 2024 •

edited

Loading