promql: ensure series in matrix values returned for instant queries are sorted

[charleskorn]

The PromQL engine always sorts matrix values returned for range queries (source), but does not do the same thing for matrix values returned for instant queries.

This PR makes the behaviour for instant queries consistent with range queries: matrix values are now always sorted by series.

I've added the TestInstantQueryWithRangeVectorSelector/matches series with points in range test case to demonstrate the issue and that it has been fixed. (Note that without the fix, the series order is unpredictable, so you may need to run the test multiple times to see the failure.)

I've also added some other test cases for instant queries that return matrices given there weren't any test cases for this scenario.

[krajorama]

/prombench main

[prombot]

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-14083 and main

After successful deployment, the benchmarking results can be viewed at:

• Prometheus Meta
• Prombench Dashboard
• Grafana Explorer, Loki logs
• Parca profiles (e.g. in-use memory)

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart main

[krajorama]

Looks good, one suggestion and I started prombench.

[krajorama]

/prombench cancel

[prombot]

Benchmark cancel is in progress.

[krajorama]

LGTM

[machine424]

Thanks.

(Note that without the fix, the series order is unpredictable, so you may need to run the test multiple times to see the failure.)
If the operation is cheap, we could run it multiple times in a loop to increase the likelihood of a change in order.

Taking a look at the benchmark dashboards, I can see some overhead in the CPU/query latency and Avg query engine timings/s dashboards as expected. However, I am unsure if this overhead is negligible.

[darshanime]

I can see some overhead in the CPU/query latency and Avg query engine timings/s dashboards as expected. However, I am unsure if this overhead is negligible.

Running the benchmarks would help quantify the overhead?

[bboreham]

Do you think this behaviour is documented anywhere?
I think it should be. Anyway we can go ahead with the change.

[bboreham]

Running the benchmarks

We did run prombench, although it doesn't have this kind of query.

I don't think there are any benchmarks which currently test the case of an instant query returning a matrix. Would you like to write some?

[charleskorn]

Do you think this behaviour is documented anywhere? I think it should be.

I couldn't find it documented anywhere. Perhaps here makes sense?

Should we also require instant vectors to be sorted by series as well? Seems like this would be good for the sake of consistency.

[bboreham]

I couldn't find it documented anywhere. Perhaps here makes sense?

👍

Should we also require instant vectors to be sorted by series as well?

The code takes care not to do that, since there are PromQL functions like sort which give a specific order.
Arguably it could do it where the expression has not supplied an order, but that seems complicated.

[bboreham]

Related: the code silently ignores any sort in the PromQL when returning a matrix.
In my mind I imagine some heartless Prometheus maintainer gloating "they'll figure it out eventually", but we could add an Annotation to be nice.

[when multiple points are returned, which set do you sort on?]

[bboreham]

Also related: sort_by_label could work for matrixes.

[charleskorn]

Related: the code silently ignores any sort in the PromQL when returning a matrix. In my mind I imagine some heartless Prometheus maintainer gloating "they'll figure it out eventually", but we could add an Annotation to be nice.

[when multiple points are returned, which set do you sort on?]

I've created #14115 so we don't lose this.

[charleskorn]

I've resolved the merge conflicts. The CI failure seems to be an unrelated flake.

[bboreham]

LGTM, thanks!