Improve GetRules performance #5805
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
rajagopalanand
force-pushed
the
lock-fix
branch
2 times, most recently
from
b77ed02 to
28bb394
Compare
yeya24
reviewed
Mar 30, 2024
pkg/ruler/manager.go
Outdated
| @@ -184,6 +198,29 @@ func (r *DefaultMultiTenantManager) syncRulesToManager(ctx context.Context, user | |||
| } | |||
| } | |||
|
|
|||
| func (r *DefaultMultiTenantManager) getRulesManager(user string, ctx context.Context) (RulesManager, bool) { | |||
There was a problem hiding this comment.
Can we only do one thing in this method to get manager from the map rather than creating a new one?
Create manager and make it up and running should be a separate function.
|
Can you please update the PR description and mention what this pr is trying to do? |
yeya24
reviewed
Mar 30, 2024
rajagopalanand
force-pushed
the
lock-fix
branch
from
28bb394 to
d66adf6
Compare
rajagopalanand
changed the title
rajagopalanand
force-pushed
the
lock-fix
branch
from
d66adf6 to
28e46cc
Compare
…entirety of sync rules Signed-off-by: Anand Rajagopal <anrajag@amazon.com>
rajagopalanand
force-pushed
the
lock-fix
branch
from
28e46cc to
1161d31
Compare
yeya24
reviewed
Apr 1, 2024
Signed-off-by: Anand Rajagopal <anrajag@amazon.com>
alanprot
pushed a commit
to alanprot/cortex
that referenced
this pull request
Apr 2, 2024
…entirety of sync rules (cortexproject#5805) Signed-off-by: Anand Rajagopal <anrajag@amazon.com>
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:
Currently the manager's SyncRuleGroups and GetRules methods share the same lock. This means that if SyncRuleGroups becomes slow then GetRules will have to wait a long time to acquire the lock.
SyncRuleGroups can become slow when rule groups are updated with slow running rules because the rule group will wait for currently evaluating rule to finish before it stops
This PR fixes this problem by:
userManagerMtxfor it's intended purpose which is to protect theuserManagersmapuseManagerMtxto RWMutex so that exclusive lock is acquired only when a new manager is created or clean up occurs at the end ofSyncRuleGroups. By doing this, the duration of exclusive lock is reduced and the scope is reducedsyncRuleMtxto ensureSyncRuleGroupsis executed sequentiallyrules/manager.Updateis called. This is because each Prometheus rule manager can still be responsible for large number of rule groups and if many of those rule groups have slow running rules, then ListRules will be slow. The cached rule groups will be removed as soon as update is completeWhich issue(s) this PR fixes:
Fixes #5745
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]