Fix tenant federation performance hotspot #4502
Merged
Conversation
This file contains hidden or 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
edma2
force-pushed
the
fix-merge-queryable-hotspot
branch
from
c3e4c2c to
5bc990c
Compare
added 3 commits
September 29, 2021 16:48
Signed-off-by: Eugene Ma <eugene.ma@airbnb.com>
Signed-off-by: Eugene Ma <eugene.ma@airbnb.com>
Signed-off-by: Eugene Ma <eugene.ma@airbnb.com>
edma2
force-pushed
the
fix-merge-queryable-hotspot
branch
from
5bc990c to
be84c7b
Compare
|
cc @simonswine |
simonswine
approved these changes
Oct 6, 2021
2 tasks
|
bump - can someone with merge privileges help merge this? |
|
Yes. I need a 2nd review. @bboreham or @pstibrany could you review this, please? |
srijan55
pushed a commit
to srijan55/cortex
that referenced
this pull request
Nov 26, 2021
* merge queryable: cache curr series Signed-off-by: Eugene Ma <eugene.ma@airbnb.com> * add test to check label set Signed-off-by: Eugene Ma <eugene.ma@airbnb.com> * update CHANGELOG Signed-off-by: Eugene Ma <eugene.ma@airbnb.com> Co-authored-by: Eugene Ma <eugene.ma@airbnb.com> Signed-off-by: Manish Kumar Gupta <manishkg@microsoft.com>
alvinlin123
pushed a commit
to ac1214/cortex
that referenced
this pull request
Jan 14, 2022
* merge queryable: cache curr series Signed-off-by: Eugene Ma <eugene.ma@airbnb.com> * add test to check label set Signed-off-by: Eugene Ma <eugene.ma@airbnb.com> * update CHANGELOG Signed-off-by: Eugene Ma <eugene.ma@airbnb.com> Co-authored-by: Eugene Ma <eugene.ma@airbnb.com> Signed-off-by: Alvin Lin <alvinlin@amazon.com>
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What this PR does:
Profiling federated queries shows a hotspot in
MergeQueryablethat makes federated queries over many tenants very slow.Performance measurements using synthetic read/write traffic with benchtool:
{__name__=~".+"}federated over all 100 tenants, last 15 minutesThe test was repeated with even more tenants (1000) and the performance gap was even larger (10s vs. 50s P99). With this patch applied, a federated query over 100 tenants patch performs similarly to a non-federated query (against the same number of queried time series and time range), eliminating almost all latency overhead of federated querying.
Profile graph taken on Querier while executing query without the patch (attached pprof file):
The root cause lies in the implementation of
addLabelsSeries.Labelswhich rebuilds the entire label set for a series, followed by a sort. It does this in order to insert an extra__tenant_id__label and rewrite any existing label with that name that would conflict.This isn't a big deal if called once per time series, but it's actually called many times for merging series in a sorted fashion. The merge algorithm uses a heap which performs a lot of comparisons (which are based on labels) when push/popping nodes. Therefore, we need to make comparisons cheap, which requires
.Labels()calls to be cheap.This PR attempts to fix this hot spot by memoizing the label set once per call to the iterator's
At()function, which makes theLabelscall essentially free.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]