Repository navigation
fix sorting in sort_by_label and sort_by_label_desc - #7886
SungJin1212 wants to merge 2 commits into
Conversation
b6167fd to
9af3335
Compare
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
9af3335 to
11b7295
Compare
sandy2008
left a comment
sandy2008
left a comment
There was a problem hiding this comment.
LGTM. All six new TestMergeResponse_SortByLabel cases fail without the merge.go change and pass with it. One optional suggestion inline.
| } | ||
|
|
||
| vec := make(promql.Vector, 0, len(output)) | ||
| for _, s := range output { |
There was a problem hiding this comment.
Optional: vec is filled in map iteration order, so funcSortByLabel sees a different input order on every merge. Upstream's comparator isn't a strict weak order when a series lacks a sort label (natsort.Compare("", "x") and Compare("x", "") are both false) or when two values are naturally equal (1 vs 01), so in those cases the merged order varies between runs. Merging {job="a"}, {job="b",foo="x"}, {job="c"}, {job="d",foo="y"} from two shards for sort_by_label(sum by (job, foo) (up), "foo") gave 4 different orders in 200 runs, some with foo="y" ahead of foo="x". Iterating over slices.Sorted(maps.Keys(output)) makes it deterministic, and the new tests still pass. (prometheus/prometheus#19466 is open upstream for the natural-equal tie-break.)
There was a problem hiding this comment.
Thanks, applied it to the latest commit.
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
What this PR does:
When sharded instant query has
sort_by_labelorsort_by_label_desc, the query frontend re-sorts the merged result by sample value instead of by the given labels. For example, the result ofsort_by_label(sum by (job) (up), "job")comes back in value order rather thanjoborder.This PR adds a separate sort plan for them. It orders the merged series with Prometheus' own
sort_by_label/sort_by_label_descimplementation.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]docs/configuration/v1-guarantees.mdupdated if this PR introduces experimental flags