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

Streaming PromQL engine: sort series in matrix at end #8058

Merged
merged 3 commits into from
May 8, 2024

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented May 6, 2024

What this PR does

This PR modifies the behaviour of the streaming PromQL engine to not sort series in the aggregation operator, and instead sort the final matrix value.

Matrix values are expected to be sorted by series labels, so we can't avoid sorting series altogether - but we don't need to sort intermediate results.

Sorting series in the aggregation operator is unnecessary:

  • in the specific case of the aggregation operator, it's better to return series in whatever order minimises the number of incomplete groups held in memory, as this reduces peak memory utilisation in the case where the input series are sorted according to the output groups (and in the worst case, peak memory utilisation is no worse than what we have today)
  • there may be another operator consuming the result of the aggregation, and so sorting the output may be a waste of time, as the consuming operator might not need its input to be sorted

Some operators (eg. selectors) will always return series in the correct order, but slices.SortFunc handles the case where the input is already sorted relatively efficiently, so this extra sorting pass does not add much overhead (<1% latency in our benchmarks).

The final sorting pass does not require more memory utilisation or allocations.

Which issue(s) this PR fixes or relates to

(none)

Checklist

  • Tests updated.
  • [n/a] Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • [n/a] about-versioning.md updated with experimental features.

@charleskorn charleskorn marked this pull request as ready for review May 6, 2024 07:14
@charleskorn charleskorn requested a review from a team as a code owner May 6, 2024 07:14
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

It took me a while to understand why this change lower the peak memory utilization in some cases, but now I do understand it. Good job! 👏

@charleskorn charleskorn merged commit bdc5b16 into main May 8, 2024
29 checks passed
@charleskorn charleskorn deleted the charleskorn/dont-sort-series-early branch May 8, 2024 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants