From 1e638c1c85eeffef7606ffec35fc77c9c75ab2b5 Mon Sep 17 00:00:00 2001 From: Charles Korn Date: Tue, 1 Oct 2024 15:03:49 +1000 Subject: [PATCH] MQE: fix incorrect query results or "found duplicate series for the match group" errors when binary operation has unsorted labels in `on` (#9482) * MQE: fix incorrect query results or "found duplicate series for the match group" errors when binary operation has unsorted labels in `on` * Add changelog entry * Address PR feedback: explain purpose of tests (cherry picked from commit e8e1e13dd9d186954b422406a79eeabbf2cfccaa) --- CHANGELOG.md | 2 +- .../operators/vector_vector_binary_operation.go | 2 ++ .../testdata/ours/binary_operators.test | 16 ++++++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d58d19c2dcb..b892878679b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ * `cortex_alertmanager_alerts` * `cortex_alertmanager_silences` * [CHANGE] Cache: Deprecate experimental support for Redis as a cache backend. #9453 -* [FEATURE] Querier: add experimental streaming PromQL engine, enabled with `-querier.query-engine=mimir`. #9367 #9368 #9398 #9399 #9403 #9417 #9418 #9419 #9420 +* [FEATURE] Querier: add experimental streaming PromQL engine, enabled with `-querier.query-engine=mimir`. #9367 #9368 #9398 #9399 #9403 #9417 #9418 #9419 #9420 #9482 * [FEATURE] Query-frontend: added experimental configuration options `query-frontend.cache-errors` and `query-frontend.results-cache-ttl-for-errors` to allow non-transient responses to be cached. When set to `true` error responses from hitting limits or bad data are cached for a short TTL. #9028 * [FEATURE] gRPC: Support S2 compression. #9322 * `-alertmanager.alertmanager-client.grpc-compression=s2` diff --git a/pkg/streamingpromql/operators/vector_vector_binary_operation.go b/pkg/streamingpromql/operators/vector_vector_binary_operation.go index b0b86e14fe3..dbf98af096f 100644 --- a/pkg/streamingpromql/operators/vector_vector_binary_operation.go +++ b/pkg/streamingpromql/operators/vector_vector_binary_operation.go @@ -402,6 +402,8 @@ func (b *VectorVectorBinaryOperation) groupKeyFunc() func(labels.Labels) []byte buf := make([]byte, 0, 1024) if b.VectorMatching.On { + slices.Sort(b.VectorMatching.MatchingLabels) + return func(l labels.Labels) []byte { return l.BytesWithLabels(buf, b.VectorMatching.MatchingLabels...) } diff --git a/pkg/streamingpromql/testdata/ours/binary_operators.test b/pkg/streamingpromql/testdata/ours/binary_operators.test index a0612c6e1bc..944901b117a 100644 --- a/pkg/streamingpromql/testdata/ours/binary_operators.test +++ b/pkg/streamingpromql/testdata/ours/binary_operators.test @@ -98,11 +98,27 @@ eval range from 0 to 24m step 6m left_side - on(env, pod) right_side {env="test", pod="a"} -9 -18 -27 {env="test", pod="b"} -36 -45 -54 +# Test the same thing again with the grouping labels in a different order. +# (The implementation of binary operations relies on grouping labels being sorted in some places, +# so this test exists to ensure this is done correctly.) +eval range from 0 to 24m step 6m left_side - on(pod, env) right_side + {env="prod", pod="a"} -63 -72 -81 + {env="test", pod="a"} -9 -18 -27 + {env="test", pod="b"} -36 -45 -54 + eval range from 0 to 24m step 6m left_side - ignoring(env, pod) right_side {group="baz"} -33 -42 -51 {group="bar"} -6 -15 -24 {group="foo"} -69 -78 -87 +# Test the same thing again with the grouping labels in a different order. +# (The implementation of binary operations relies on grouping labels being sorted in some places, +# so this test exists to ensure this is done correctly.) +eval range from 0 to 24m step 6m left_side - ignoring(pod, env) right_side + {group="baz"} -33 -42 -51 + {group="bar"} -6 -15 -24 + {group="foo"} -69 -78 -87 + clear # One-to-one matching, but different series match at different time steps, or not at all