promql: add sort_by_label and sort_by_label_desc functions #11299

galexrt · 2022-09-12T13:35:13Z

This adds functions to sort a vector by its label value.

Based on #1533

Signed-off-by: Alexander Trost galexrt@googlemail.com

There was some discussion around adding this functionality to PromQL/ Prometheus in the original PR #1533, though based on the various comments from users and now myself stumbling upon this while trying to visualize certain metrics sorted by label value, I would gladly see this topic revisited.

roidelapluie · 2022-09-12T14:07:50Z

How difficult would it be to make this a vararg function to be able to sort by multiple labels?

sort_by_label(up, "job","instance")

galexrt · 2022-09-12T14:13:40Z

@roidelapluie I can look into making it into a vararg function to be able to sort by multiple labels. Is there an existing function that already accepts a list of strings as an argument that I can look at?

roidelapluie · 2022-09-12T14:16:30Z

label_join()

galexrt · 2022-09-12T14:59:25Z

@roidelapluie The sort_by_label and sort_by_label_desc functions now accept a list of labels and is sorting the vector accordingly (see functions.tests).

juliusv · 2022-09-12T18:52:36Z

docs/querying/functions.md

@@ -401,6 +401,14 @@ in ascending order.

 Same as `sort`, but sorts in descending order.

+## `sort_by_label()`
+
+`sort_by_label(v instant-vector, label string, ...)` returns vector elements sorted by their label values and sample value in case of label values being equal, in ascending order.


Hmm, I wonder if we shouldn't keep this completely independent from the sample value and instead use other label values to break the sorting tie? That way, sort results would be stable over time, given the same set of time series.

I agree with Julius. We could also decide not to sort at all in that case, so you can do sort_by_label(sort(up),"job")

Yeah, that sounds even better.

So I should remove the "fallback" to comparing the value in the sort_by_label functions? This might make the test unstable, though I have to re-check if that happens without this "fallback" compare.

The tests should be stable even without this

we should make it clear that labels are sorted lexicographically

@roidelapluie Without the value comparison the test fails no matter in what order the result in the test(s) are placed.

That is strange. The order should be predictible.

It seems that it is not predictable as every X run the test fails and complains about the order of the result.

How should we approach this that we can hopefully get this functionality integrated?

From my perspective, the fix here is to sort the values using the sort/sort_desc functions and then do the sort by label(s).

promql/functions.go

roidelapluie · 2022-09-13T07:36:18Z

promql/functions.go

+			continue
+		case 1:
+			return false
+		}


we need a return at the end

bboreham · 2022-09-13T09:39:05Z

Note that range queries are always sorted by label sets, right at the end:

prometheus/promql/engine.go

Line 722 in 8b863c4

sort.Sort(mat)

Perhaps you could mention in the documentation this is intended for instant queries.

juliusv · 2022-09-13T09:54:20Z

Note that range queries are always sorted by label sets, right at the end:

prometheus/promql/engine.go

Line 722 in 8b863c4

sort.Sort(mat)

Perhaps you could mention in the documentation this is intended for instant queries.

That same comment would apply to sort() and sort_desc() as well. In general, sorting is a no-op for range queries, as output points are always sorted by the X and Y axis there (unless someone cares about a specific legend item sorting, which I haven't heard yet).

I think we can either omit that comment on the new functions as well, or add a comment to the existing sort functions too. Something like:

Please note that sorting functions only affect the results of instant queries, as range query results always have a fixed output ordering.

bboreham · 2022-09-13T11:41:47Z

I think it would be easy to work out that sort (by value) is not a good idea over a range, where different series can be the highest value at different points in time.

Whereas sort_by_label is something you might reasonably want for a range query, e.g. to change the order in which lines are arranged in a dashboard. Hence it should be documented that it won't do what you asked.

juliusv · 2022-09-13T13:15:34Z

Ok yeah, that makes sense. Or we could actually make it work for range queries as well, but I doubt that's worth it. So then let's just add a comment for the new functions.

bboreham

Hi, we discussed this PR at the Prometheus dev summit today; we would like to move forward, but would like to ask you to put these functions behind a feature flag.

I had a couple of comments about the implementation too.

bboreham · 2023-02-23T16:44:03Z

promql/functions.go

@@ -1218,6 +1238,78 @@ func (s *vectorByReverseValueHeap) Pop() interface{} {
 	return el
 }

+type vectorByLabelHeap struct {


What does the word "Heap" denote here?

bboreham · 2023-02-23T16:45:40Z

promql/functions.go

+	// Incase the labels are the same, NaN should sort to the bottom, so take
+	// ascending sort with NaN first and reverse it.
+	byLabelSorter := vectorByLabelHeap{vector: vals[0].(Vector), labels: stringSliceFromArgs(args[1:])}
+	sort.Sort(sort.Reverse(byLabelSorter))


slices.SortFunc is probably neater now.

roidelapluie · 2023-07-18T11:19:57Z

cc @galexrt are you still willing to work on this, rebase this pull request and put the functions behind a feature flag? Thanks!

galexrt · 2023-07-18T11:32:29Z

cc @galexrt are you still willing to work on this, rebase this pull request and put the functions behind a feature flag? Thanks!

Oh that totally fell of my radar, yes I am willing to work on that. Do you have a link to a feature flag example somewhere that I can use as an example?

roidelapluie · 2023-07-18T12:06:32Z

You can look at the engine ops EnableNegativeOffset which used to be a feature flag

galexrt · 2023-07-18T12:18:06Z

You can look at the engine ops EnableNegativeOffset which used to be a feature flag

So should I dynamically add the sort_by_label and sort_by_label_desc funcs to the FunctionsList map on start when the engine is created in promql/engine.go?

roidelapluie · 2023-08-08T11:27:41Z

I think a proper error saying that the feature needs to be enabled if the feature flag is not set would be better. However, I don't know if we can properly error from promql functions so we might need to be creative.

galexrt · 2023-08-14T19:20:35Z

@roidelapluie I think I have found a way to check the feature flag in the evaluator. Please take another look at the latest code changes, thanks!

galexrt · 2023-10-18T16:09:11Z

@roidelapluie I have updated the PR to resolve the conflicts. Any news on this?

juliusv

Thank you, looks pretty good to me now!

It seems like the main outstanding question is around sort stability and whether we should resort to:

Sorting by sample value
Sorting by the remaining label set
Do nothing

I would be for sorting by the remaining label set if that is easily doable (hopefully just a small extension of the SortFunc for the 0 string comparison case?). Sorting by sample value seems too unrelated to the label sorting and is also not necessarily always stable, as two series can and often do have the same sample value.

docs/feature_flags.md

promql/engine.go

juliusv · 2023-10-26T14:44:11Z

documentation/examples/prometheus.yml


    # metrics_path defaults to '/metrics'
    # scheme defaults to 'http'.

    static_configs:
-      - targets: ["localhost:9090"]
+      - targets: ["localhost:9138"]


Looks like some other unrelated changes crept into the PR here now?

I have removed this unrelated change, PTAL

juliusv · 2023-11-22T14:08:25Z

Thanks! I see the tests failing with this error currently:

--- FAIL: TestEvaluations (5.87s)
    --- FAIL: TestEvaluations/testdata/functions.test (1.20s)
        test.go:105: 
            	Error Trace:	/__w/prometheus/prometheus/promql/test.go:105
            	            				/__w/prometheus/prometheus/promql/test.go:83
            	Error:      	Received unexpected error:
            	            	error in eval sort_by_label(http_requests, "instance", "group") (line 498): expected metric {__name__="http_requests", group="production", instance="0", job="api-server"} with [100.000000] at position 3 but was at 1
            	Test:       	TestEvaluations/testdata/functions.test

juliusv · 2023-11-22T14:09:09Z

documentation/examples/prometheus.yml

@@ -20,7 +20,7 @@ rule_files:
 # Here it's Prometheus itself.
 scrape_configs:
  # The job name is added as a label `job=<job_name>` to any timeseries scraped from this config.
-  - job_name: "prometheus"
+  - job_name: "extended-ceph-exporter"


I still see this unrelated change in here?

Fixed, all unrelated changes should be gone now

galexrt · 2023-11-23T21:11:50Z

Thanks! I see the tests failing with this error currently:

--- FAIL: TestEvaluations (5.87s)
    --- FAIL: TestEvaluations/testdata/functions.test (1.20s)
        test.go:105: 
            	Error Trace:	/__w/prometheus/prometheus/promql/test.go:105
            	            				/__w/prometheus/prometheus/promql/test.go:83
            	Error:      	Received unexpected error:
            	            	error in eval sort_by_label(http_requests, "instance", "group") (line 498): expected metric {__name__="http_requests", group="production", instance="0", job="api-server"} with [100.000000] at position 3 but was at 1
            	Test:       	TestEvaluations/testdata/functions.test

I have changed the sorting logic slightly, and it should now work better when more than 2 labels are involved.
To note sorting with 3 or more labels is "weird" because if the first label doesn't it doesn't compare the next label(s), added some test cases for that as well.

PTAL

This adds functions to sort a vector by its label value. Based on prometheus#1533 Signed-off-by: Alexander Trost <galexrt@googlemail.com>

Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>

roidelapluie · 2023-11-28T14:31:53Z

Thanks!

pstibrany · 2023-11-28T15:07:27Z

It's amazing to see this merged, ~~9 ehm~~ 7 years after first #1533 :) Thanks @galexrt and all the reviewers! 🎉 (I can't count)

pixelrebel · 2023-11-28T18:08:18Z

Thank you @galexrt ! This is a great achievement!

bobrik · 2024-01-16T00:34:15Z

Straightforward string sorting yield very questionable results for humans:

I urge you to consider using natural sorting instead.

Computers can sort by label however they want even without using sort_by_label, but humans need a little help.

See these PRs that added natural sorting to Grafana:

juliusv · 2024-01-16T10:51:20Z

@bobrik Agreed, good point! It's not yet too late to change things, since the new sorting functions are added under an experimental feature flag. Do you want to send a PR?

bobrik · 2024-01-16T18:13:54Z

Here's my attempt: #13411.

galexrt requested review from codesome, roidelapluie, juliusv and Nexucis as code owners September 12, 2022 13:35

galexrt mentioned this pull request Sep 12, 2022

Added sort_by_label function for sorting by label values. #1533

Closed

galexrt force-pushed the sort_by_labels branch from 704437c to 298497e Compare September 12, 2022 13:39

galexrt force-pushed the sort_by_labels branch 2 times, most recently from 1a1be49 to 84d2e6d Compare September 12, 2022 14:57

juliusv reviewed Sep 12, 2022

View reviewed changes

promql/functions.go Outdated Show resolved Hide resolved

juliusv reviewed Sep 12, 2022

View reviewed changes

promql/functions.go Outdated Show resolved Hide resolved

roidelapluie reviewed Sep 13, 2022

View reviewed changes

promql/functions.go Outdated

continue

case 1:

return false

}

Copy link

Member

roidelapluie Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need a return at the end

bboreham reviewed Feb 23, 2023

View reviewed changes

galexrt force-pushed the sort_by_labels branch 2 times, most recently from 03b3ecb to b14c014 Compare July 18, 2023 11:37

galexrt force-pushed the sort_by_labels branch from b14c014 to 6af884a Compare July 18, 2023 12:13

roidelapluie self-assigned this Aug 8, 2023

galexrt force-pushed the sort_by_labels branch 2 times, most recently from a054cc4 to 66ff46b Compare August 14, 2023 19:19

roidelapluie mentioned this pull request Sep 5, 2023

PoC for query post-processing #8434

Closed

galexrt force-pushed the sort_by_labels branch 2 times, most recently from bff4451 to ae874fd Compare October 18, 2023 16:05

juliusv reviewed Oct 23, 2023

View reviewed changes

docs/feature_flags.md Outdated Show resolved Hide resolved

promql/engine.go Outdated Show resolved Hide resolved

galexrt force-pushed the sort_by_labels branch from ae874fd to c102153 Compare October 24, 2023 10:25

juliusv reviewed Oct 26, 2023

View reviewed changes

galexrt force-pushed the sort_by_labels branch 2 times, most recently from a9160ce to 224e642 Compare November 22, 2023 11:22

juliusv reviewed Nov 22, 2023

View reviewed changes

galexrt force-pushed the sort_by_labels branch from 224e642 to dbeef3d Compare November 23, 2023 21:08

galexrt and others added 2 commits November 28, 2023 14:40

promql: add sort_by_label and sort_by_label_desc functions

5051a99

This adds functions to sort a vector by its label value. Based on prometheus#1533 Signed-off-by: Alexander Trost <galexrt@googlemail.com>

sort_by_label: Switch to feature flag

c1ec6ae

Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>

roidelapluie force-pushed the sort_by_labels branch from dbeef3d to c1ec6ae Compare November 28, 2023 14:11

roidelapluie approved these changes Nov 28, 2023

View reviewed changes

roidelapluie merged commit 4293de9 into prometheus:main Nov 28, 2023
24 checks passed

pstibrany mentioned this pull request Jan 9, 2024

Update vendored mimir-prometheus grafana/mimir#7057

Merged

4 tasks

promql: add sort_by_label and sort_by_label_desc functions #11299

promql: add sort_by_label and sort_by_label_desc functions #11299

Conversation

galexrt commented Sep 12, 2022

roidelapluie commented Sep 12, 2022

galexrt commented Sep 12, 2022

roidelapluie commented Sep 12, 2022

galexrt commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboreham commented Sep 13, 2022

juliusv commented Sep 13, 2022

bboreham commented Sep 13, 2022

juliusv commented Sep 13, 2022

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roidelapluie commented Jul 18, 2023

galexrt commented Jul 18, 2023

roidelapluie commented Jul 18, 2023

galexrt commented Jul 18, 2023

roidelapluie commented Aug 8, 2023

galexrt commented Aug 14, 2023

galexrt commented Oct 18, 2023

juliusv left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliusv commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galexrt commented Nov 23, 2023

roidelapluie commented Nov 28, 2023

pstibrany commented Nov 28, 2023 • edited

pixelrebel commented Nov 28, 2023

bobrik commented Jan 16, 2024

juliusv commented Jan 16, 2024

bobrik commented Jan 16, 2024

juliusv left a comment •

edited

pstibrany commented Nov 28, 2023 •

edited