This repository has been archived by the owner on Jul 19, 2023. It is now read-only.
Split query by interval #713
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Definitively like this approach. |
kolesnikovae
changed the title
kolesnikovae
force-pushed
the
feat/query_split_by_interval
branch
2 times, most recently
from
2c5afd8 to
f6efd32
Compare
kolesnikovae
force-pushed
the
feat/query_split_by_interval
branch
from
f6efd32 to
73b6469
Compare
kolesnikovae
commented
May 29, 2023
cyriltovena
reviewed
May 30, 2023
kolesnikovae
force-pushed
the
feat/query_split_by_interval
branch
from
f2bc3a9 to
5b0a109
Compare
|
I'm waiting for this one to be merged to start working on query limits in the frontend. |
# Conflicts: # pkg/api/api.go
simonswine
pushed a commit
to simonswine/pyroscope
that referenced
this pull request
Jun 30, 2023
* Separate handlers in querier fronted * Clarify http responce decompression implementation details * Draft query time split * Split SelectSeries by time * Align interval to step duration * Remove unused code * Add querier.max-concurrent option * Fix connect headers
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #696
The PR contains the basic implementation of query parallelization by time sub-ranges. I decided to not over-copmlicate the solution for now, because the primary goal of the work is to find bottle necks in the read path. Over time, however, we should have a composable query plan (logical and physical).
The implementation only covers the following APIs:
SelectMergeStacktracesSelectSeriesThe following configuration options were added:
querier.split-queries-by-interval. Defaults to 0 – the new mechanism is disabled.querier.max-query-parallelism. Defaults to 0 – the limit is disabled. Specifies how many sub-queries can be executed simultaneously per a single query in the frontend. In practice, I don't think we need to limit this, or have a very big value (thousands), because it is up to the scheduler (query-scheduler.max-outstanding-requests-per-tenantdefaults to 100) and the querier to decide when to execute a sub-query. The parameter is present for consistence with the existing frontend implementations.querier.max-concurrent. Defaults to 4 – The default value was previously set statically. Indicates how many requests (queries or sub-queries) a single querier can handle concurrently.(I propose to set the values separately, after a sane default is found).
There's an issue that's worths mentioning: a flame graph aggregated from many
SelectMergeStacktracescalls differs from the one fetched without parallelization because of how the truncation works. The difference is small but might be noticeable is some cases:Comparison
The discrepancy comes from the fact that the set of truncated nodes for each "intermediate" flame graph (read: tree) is different and their weights vary from one to another. I'm not sure if this is something that requires an immediate fix, but we should keep an eye on this.
Before is on the left. Notice that the share of other node has decreased. However, it may have opposite results in some cases, where the nodes are truncated early (when we don't know about other trees) before a critical mass has built up, contributing to other more than we want. This is a problem because our assumption that we preserve the top N most significant nodes is not true anymore. In case if it becomes a real issue, I propose to simply increase the number of nodes for trees that are inputs of the aggregation (e.g. by 20-100%), first.