MQE: add support for `group_left` and `group_right` (aka many-to-one and one-to-many matching) #10119

charleskorn · 2024-12-04T02:17:54Z

What this PR does

This PR adds support for group_left and group_right (aka many-to-one and one-to-many matching) in binary operations to MQE.

In our benchmarks, latency is up to 80% lower and peak memory utilisation is up to 30% lower with MQE compared to Prometheus' engine:

                                                                         │  Prometheus  │               Mimir                │
                                                                         │    sec/op    │   sec/op     vs base               │
Query/h_1_*_on(l)_group_left()_a_1,_instant_query-10                        321.8µ ± 2%   340.3µ ± 9%        ~ (p=0.065 n=6)
Query/h_1_*_on(l)_group_left()_a_1,_range_query_with_100_steps-10           452.3µ ± 1%   358.5µ ± 2%  -20.74% (p=0.002 n=6)
Query/h_1_*_on(l)_group_left()_a_1,_range_query_with_1000_steps-10         1651.5µ ± 0%   756.0µ ± 3%  -54.23% (p=0.002 n=6)
Query/h_100_*_on(l)_group_left()_a_100,_instant_query-10                    4.694m ± 1%   4.659m ± 2%        ~ (p=0.310 n=6)
Query/h_100_*_on(l)_group_left()_a_100,_range_query_with_100_steps-10      22.581m ± 1%   8.320m ± 1%  -63.16% (p=0.002 n=6)
Query/h_100_*_on(l)_group_left()_a_100,_range_query_with_1000_steps-10     185.37m ± 3%   39.77m ± 0%  -78.55% (p=0.002 n=6)
Query/h_2000_*_on(l)_group_left()_a_2000,_instant_query-10                  84.71m ± 1%   85.24m ± 2%        ~ (p=0.132 n=6)
Query/h_2000_*_on(l)_group_left()_a_2000,_range_query_with_100_steps-10     468.3m ± 1%   155.0m ± 4%  -66.90% (p=0.002 n=6)
Query/h_2000_*_on(l)_group_left()_a_2000,_range_query_with_1000_steps-10   4006.0m ± 2%   761.7m ± 1%  -80.99% (p=0.002 n=6)
geomean                                                                     20.87m        10.41m       -50.14%

                                                                         │   Prometheus   │                Mimir                │
                                                                         │       B        │      B        vs base               │
Query/h_1_*_on(l)_group_left()_a_1,_instant_query-10                        73.48Mi ±  1%   73.76Mi ± 1%        ~ (p=0.699 n=6)
Query/h_1_*_on(l)_group_left()_a_1,_range_query_with_100_steps-10           72.05Mi ±  1%   73.21Mi ± 1%   +1.62% (p=0.002 n=6)
Query/h_1_*_on(l)_group_left()_a_1,_range_query_with_1000_steps-10          70.13Mi ±  1%   70.78Mi ± 1%        ~ (p=0.087 n=6)
Query/h_100_*_on(l)_group_left()_a_100,_instant_query-10                    69.96Mi ±  1%   70.16Mi ± 1%   +0.29% (p=0.015 n=6)
Query/h_100_*_on(l)_group_left()_a_100,_range_query_with_100_steps-10       73.55Mi ±  1%   73.12Mi ± 1%        ~ (p=0.262 n=6)
Query/h_100_*_on(l)_group_left()_a_100,_range_query_with_1000_steps-10     107.05Mi ±  5%   90.28Mi ± 3%  -15.66% (p=0.002 n=6)
Query/h_2000_*_on(l)_group_left()_a_2000,_instant_query-10                  79.68Mi ±  3%   79.24Mi ± 4%        ~ (p=0.485 n=6)
Query/h_2000_*_on(l)_group_left()_a_2000,_range_query_with_100_steps-10     172.9Mi ±  6%   131.9Mi ± 2%  -23.73% (p=0.002 n=6)
Query/h_2000_*_on(l)_group_left()_a_2000,_range_query_with_1000_steps-10    651.3Mi ± 13%   438.1Mi ± 0%  -32.74% (p=0.002 n=6)
geomean                                                                     107.0Mi         97.69Mi        -8.68%

I am not adding a changelog entry given it is covered by the generic MQE changelog entry that links to #10067.

Which issue(s) this PR fixes or relates to

#10067

Checklist

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

charleskorn · 2024-12-04T03:29:28Z

pkg/streamingpromql/operators/binops/grouped_vector_vector_binary_operation.go

+	"github.com/grafana/mimir/pkg/streamingpromql/types"
+)
+
+var errMultipleMatchesOnManySide = errors.New("multiple matches for labels: grouping labels must ensure unique matches")


Note to reviewers: this error message isn't the best, but it's what Prometheus uses, and including more specific details is difficult.

jhesketh

looks good to me. I admit for some of the review I'm trusting the general approach, unit tests, and benchmarks, but it overall seems good sans a few comments.

pkg/streamingpromql/operators/binops/binary_operation.go

pkg/streamingpromql/operators/binops/grouped_vector_vector_binary_operation.go

jhesketh · 2024-12-05T09:57:01Z

Forgot to add that I think we should see group left/right in the mixed metrics test gauntlet

charleskorn · 2024-12-10T09:42:50Z

Forgot to add that I think we should see group left/right in the mixed metrics test gauntlet

Added in 7214a8e.

# Conflicts: # cmd/mimir/config-descriptor.json # cmd/mimir/help-all.txt.tmpl # docs/sources/mimir/configure/configuration-parameters/index.md # pkg/streamingpromql/config.go # pkg/streamingpromql/engine_test.go

…same timestamp on the same "one" side for the same group with different additional labels

…many" side

…sons

…on either side

… BytesWithoutLabels() are reused if resized

…n slice

…omputeResult` to reduce nesting

jhesketh

lgtm, thanks for addressing the comments!

charleskorn mentioned this pull request Dec 4, 2024

Mimir Query Engine #10067

Open

charleskorn commented Dec 4, 2024

View reviewed changes

charleskorn force-pushed the charleskorn/mqe-group-left-right branch from 4f7e673 to 74afbb3 Compare December 4, 2024 03:45

charleskorn marked this pull request as ready for review December 4, 2024 04:08

charleskorn requested review from tacole02 and a team as code owners December 4, 2024 04:08

jhesketh reviewed Dec 5, 2024

View reviewed changes

charleskorn added 15 commits December 10, 2024 20:51

Add feature toggle

dc874aa

# Conflicts: # cmd/mimir/config-descriptor.json # cmd/mimir/help-all.txt.tmpl # docs/sources/mimir/configure/configuration-parameters/index.md # pkg/streamingpromql/config.go # pkg/streamingpromql/engine_test.go

Enable upstream test cases

d9a467e

Remove comments about and/or/unless.

aad0179

Add some test cases

37434ad

Make condition clearer

31c886f

Add comparison operation edge cases

6ec6e0e

Add tests for comparison operators with group_left / group_right

fa12925

Update tests to reflect prometheus/prometheus#15471

a09c1ac

Expand test to confirm label handling behaviour

4965398

Rename existing operator

64114f7

Add structure for new operator

749789b

Initial implementation of SeriesMetadata

9ce7407

Initial implementation

882b2a5

Handle case where one set of many-side series many to many output series

0ea8687

Use correct labels when grouping one side series

b29c99e

charleskorn force-pushed the charleskorn/mqe-group-left-right branch from b72c6f1 to b58c518 Compare December 10, 2024 09:52

charleskorn added 5 commits December 10, 2024 20:56

Return a conflict error message if there are multiple samples at the …

11bf6b0

…same timestamp on the same "one" side for the same group with different additional labels

Fix handling of cases where additional labels appear on the "many" side

d48be6a

Return a non-misleading error message when a conflict occurs on the "…

4e1f6fa

…many" side

Update comments

1876813

Fix regression in comparison operation output labels

d35f355

charleskorn added 19 commits December 10, 2024 20:56

Disable one-to-one comparison operation cases that fail for known rea…

87bbd46

…sons

Fix linting warnings and simplify computeOutputSeries()

a53580a

Add tests for annotations

331cf70

Add tests for case where additional labels are not present on series …

606a96d

…on either side

Add series sorting test

8c56b2d

Add provenance comment

25be40f

Add benchmark

502245f

Expand comments

0b8fa34

Fix typo in test names

7e4113a

Add test cases with label names in different orders

c41b575

Add some test cases with native histograms

230adfe

Ensure buffers passed to labels.Labels.Bytes(), BytesWithLabels() and…

a6d8763

… BytesWithoutLabels() are reused if resized

Address PR feedback: use minimal number of points for binary operatio…

e467023

…n slice

Address PR feedback: rename latestSeries to latestSeriesIndex

aabd093

Address PR feedback: add docstring for updatePresence

8c8f782

Address PR feedback: try to reuse slices in more cases

7aa29ee

Run mixed metrics tests in parallel

153f66f

Add group_left to mixed metrics tests

3a262bf

Address PR feedback: refactor `vectorVectorBinaryOperationEvaluator.c…

d2e2a1c

…omputeResult` to reduce nesting

charleskorn force-pushed the charleskorn/mqe-group-left-right branch from b58c518 to d2e2a1c Compare December 10, 2024 09:56

charleskorn requested a review from jhesketh December 10, 2024 10:59

tacole02 approved these changes Dec 13, 2024

View reviewed changes

This was referenced Dec 13, 2024

MQE: don't create copies of native histogram during binary operations #10049

Open

MQE: fix issues with vector/vector binary comparsion operations #10235

Draft

jhesketh approved these changes Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MQE: add support for `group_left` and `group_right` (aka many-to-one and one-to-many matching) #10119

MQE: add support for `group_left` and `group_right` (aka many-to-one and one-to-many matching) #10119

charleskorn commented Dec 4, 2024

charleskorn Dec 4, 2024

jhesketh left a comment

jhesketh commented Dec 5, 2024

charleskorn commented Dec 10, 2024

jhesketh left a comment

MQE: add support for group_left and group_right (aka many-to-one and one-to-many matching) #10119

Are you sure you want to change the base?

MQE: add support for group_left and group_right (aka many-to-one and one-to-many matching) #10119

Conversation

charleskorn commented Dec 4, 2024

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

charleskorn Dec 4, 2024

Choose a reason for hiding this comment

jhesketh left a comment

Choose a reason for hiding this comment

jhesketh commented Dec 5, 2024

charleskorn commented Dec 10, 2024

jhesketh left a comment

Choose a reason for hiding this comment

MQE: add support for `group_left` and `group_right` (aka many-to-one and one-to-many matching) #10119

MQE: add support for `group_left` and `group_right` (aka many-to-one and one-to-many matching) #10119