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

Query-frontend: Add experimental query blocker #5609

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

wilfriedroset
Copy link
Collaborator

What this PR does

Add experimental query blocker

Which issue(s) this PR fixes or relates to

This commit implements the following proposal: #5029

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@wilfriedroset wilfriedroset force-pushed the queries-blocker branch 3 times, most recently from e0321fb to a086fbd Compare July 28, 2023 12:22
@wilfriedroset wilfriedroset marked this pull request as ready for review July 28, 2023 12:36
@wilfriedroset wilfriedroset requested review from a team as code owners July 28, 2023 12:36
@wilfriedroset wilfriedroset force-pushed the queries-blocker branch 3 times, most recently from 2f81022 to cde3fe8 Compare August 8, 2023 21:06
@osg-grafana
Copy link
Contributor

osg-grafana commented Aug 9, 2023

  • Please rename file:
    docs/sources/mimir/configure/configure-blocking-queries.md to
    docs/sources/mimir/configure/configure-blocked-queries.md

@osg-grafana osg-grafana closed this Aug 9, 2023
@osg-grafana osg-grafana reopened this Aug 9, 2023
@osg-grafana
Copy link
Contributor

Sorry about closing the PR; I have no idea what keystroke I mistakenly just used. Eek.

@osg-grafana osg-grafana added the type/docs Improvements or additions to documentation label Aug 9, 2023
@wilfriedroset
Copy link
Collaborator Author

thank you @osg-grafana, I've taken into account your remarks.

# Configure queries to block

In certain situations, you might want to control what queries are being sent to your Mimir installation. These queries
might be intentionally or unintentionally expensive to run, and they might affect the overall stability or cost of running
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @wilfriedroset, perhaps for my personal learning, but why would a query ever be intentionally expensive to run? It sounds like there is something underneath this idea and the result is that a given query is expensive to run.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From a service provider, we might be exposed to denial of service, this is where such features are useful.
For example, a single user looking to cause harm to your service could storm the Mimir cluster with queries that would not be stopped by the current limits.
In that event, either the cluster is scaled or the quality of service decrease.

Most of this work is inspired and/or copied from Loki.
Loki's documentation contains the same phrase: https://grafana.com/docs/loki/latest/operations/blocking-queries/

Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for your patience with this PR.

I left a few comments, can you take a look?

docs/sources/mimir/manage/mimir-runbooks/_index.md Outdated Show resolved Hide resolved
pkg/util/validation/errors.go Outdated Show resolved Hide resolved

for _, block := range blocks {
if strings.TrimSpace(block.Pattern) == strings.TrimSpace(query) {
level.Warn(logger).Log("msg", "query blocker matched with exact match policy", "query", query)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd make this an Info log line. Everything is working as intended.

Copy link
Contributor

Choose a reason for hiding this comment

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

On another note: it might be useful to identify which rule is blocking the query. Maybe something as simple as the index of the rule will be enough to debug block configurations.

pkg/frontend/querymiddleware/blocker.go Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/blocker_test.go Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/blocker.go Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/blocker_test.go Show resolved Hide resolved
pkg/frontend/querymiddleware/blocker.go Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/blocker_test.go Outdated Show resolved Hide resolved
@colega
Copy link
Contributor

colega commented Aug 29, 2023

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

@wilfriedroset
Copy link
Collaborator Author

Sorry about the delay @dimitarvdimitrov I was on PTO.
I've taken into account everything that I can except the part of about the mock (see above). Let me know what you think about this amended PR.

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/blocker_test.go Outdated Show resolved Hide resolved
@wilfriedroset wilfriedroset force-pushed the queries-blocker branch 3 times, most recently from 7f8d3ba to d64df6b Compare September 13, 2023 12:40
This commit implements the following proposal: grafana#5029

Signed-off-by: Wilfried Roset <wilfriedroset@users.noreply.github.com>
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

LGTM, great work! And thanks for addressing my feedback.

@osg-grafana do you have any other remarks on the docs or are we good to merge this?

@dimitarvdimitrov dimitarvdimitrov merged commit 1fda8ea into grafana:main Sep 15, 2023
28 checks passed
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.

Thanks @wilfriedroset ! Do you mind adding the feature to the list of experimental features listed at docs/sources/mimir/configure/about-versioning.md?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/query-frontend enhancement New feature or request release/notified-changelog-cut type/docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants