Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

query frontend: sorted queries do not produce sorted results for shardable queries #6059

Closed
sdufel opened this issue Jan 19, 2023 · 3 comments · Fixed by #6125 or cortexproject/cortex#5148

Comments

@sdufel
Copy link
Contributor

sdufel commented Jan 19, 2023

Thanos, Prometheus and Golang version used:
thanos, version (branch: , revision: 2d6b0d4)
build user:
build date:
go version: go1.19
platform: darwin/arm64
(Latest main as of 2023-01-19)

Object Storage Provider: N/A

What happened:
When running a query-frontend with vertical sharding enabled, wrapping shardable instant queries with sort() or sort_desc() does not
produce sorted results.

What you expected to happen:
Instant queries wrapped in a sort() should return results ordered by value

How to reproduce it (as minimally and precisely as possible):
Run a query-frontend with --query-frontend.vertical-shards=2
Issue a shardable query wrapped with sort, e,g,:

sort(sum(rate(container_cpu_usage_seconds_total{}[2m])) by (pod))

Full logs to relevant components:
N/A

@yeya24
Copy link
Contributor

yeya24 commented Jan 23, 2023

This seems a bug indeed. I can see if sort or sort_desc is the root expression, then we will hit the bug because we always sort by series labels when merging, not by sample values.
We can:

  1. disable sharding if sort or sort_desc is in the root expression
  2. Sort based on values if sort or sort_desc is in the root expression

@fpetkovski
Copy link
Contributor

We should be able to implement 2. fairly easily. We did something similar in the engine for topk/bottomk: https://github.com/thanos-community/promql-engine/pull/156/files

@alanprot
Copy link
Contributor

alanprot commented Jan 23, 2023

I think in the topk/bottomk is ok to sort at the and as when using those functions probably we will not be returning lots of results. I'm a bit more worried on the general case though: I can have a sort that returns lots of series and we will now have to sort it again on the QF. I think we should at least do a k-way merge there as the partitioned results will be already sorted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants