-
Notifications
You must be signed in to change notification settings - Fork 697
[MQE] Configure delayed name removal per tenant #13926
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
base: main
Are you sure you want to change the base?
Conversation
|
One alternative approach I considered and discarded for this is to create two engines, one with the feature enabled and the other with it disabled, and for each query we can direct it to the appropriate engine based on the tenants, which is a similar mechanism to the fallback engine. I decided against this for now as if we were to do this for future per-tenant toggles as well, the number of engines we create would increase exponentially.
Fixed based on Charles' suggestions. |
|
💻 Deploy preview available ([MQE] Configure delayed name removal per tenant): |
tacole02
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs look good! I left one minor suggestion. Thank you!
This reverts commit 302aa78.
Co-authored-by: Taylor C <41653732+tacole02@users.noreply.github.com>
|
Bugbot Autofix prepared a fix for the bug found in the latest run.
|
3be20a2 to
1c36681
Compare
ac1ace3 to
cba2b94
Compare
cba2b94 to
850529d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON. A Cloud Agent has been kicked off to fix the reported issue.
|
Bugbot Autofix prepared a fix for the bug found in the latest run.
|
9c4dbc9 to
847fd12
Compare
| limitsProvider := querier.NewTenantQueryLimitsProvider(t.Overrides) | ||
| analysisHandler := analysis.Handler(t.QuerierQueryPlanner, limitsProvider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#14086 has been merged since you opened this - we should use the shared limits provider instance rather than creating a new one:
t.QueryLimitsProvider
| limitsProvider := querier.NewTenantQueryLimitsProvider(t.Overrides) | |
| analysisHandler := analysis.Handler(t.QuerierQueryPlanner, limitsProvider) | |
| analysisHandler := analysis.Handler(t.QuerierQueryPlanner, t.QueryLimitsProvider) |
| limitsProvider := querier.NewTenantQueryLimitsProvider(t.Overrides) | ||
| analysisHandler := analysis.Handler(t.QueryFrontendQueryPlanner, limitsProvider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above:
| limitsProvider := querier.NewTenantQueryLimitsProvider(t.Overrides) | |
| analysisHandler := analysis.Handler(t.QueryFrontendQueryPlanner, limitsProvider) | |
| analysisHandler := analysis.Handler(t.QueryFrontendQueryPlanner, t.QueryLimitsProvider) |
| // If the first call succeeded but the second failed, use the second error, otherwise keep the first. | ||
| if actualErr == nil && actualErr2 != nil { | ||
| actualErr = actualErr2 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be clearer to split this test in two: one test for GetMaxEstimatedMemoryConsumptionPerQuery, and one for GetEnableDelayedNameRemoval.
What this PR does
Make the delayed name removal feature configurable per tenant. Note that this applies only to MQE. The feature in Prometheus engine is disabled and we are not adding a flag to control that in this PR (please let me know if we should add it) as it is only used as a fallback engine.
Tested locally in docker-compose.
Which issue(s) this PR fixes or relates to
Follow up to #12509
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]. If changelog entry is not needed, please add thechangelog-not-neededlabel to the PR.about-versioning.mdupdated with experimental features.Note
Moves delayed name removal control from a global querier flag to a per-tenant limit (MQE-only) and threads it through planning, execution, and analysis.
enable_delayed_name_removal(CLI-querier.enable-delayed-name-removal); removesquerier.enable_delayed_name_removalfrom querier config and disables Prometheus engine’s versionQueryLimitsProvider(newGetEnableDelayedNameRemoval); returns an error on mixed-tenant conflictsQueryPlanner.NewQueryPlannow acceptsenableDelayedNameRemoval; analysis handler takes limits provider and passes the setting throughNewStaticQueryLimitsProvider(limit, enable)signature and added boolean arg to planner callsWritten by Cursor Bugbot for commit 847fd12. This will update automatically on new commits. Configure here.