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

Fail through to next middleware when querySizeLimit cannot be applied #9050

Conversation

trevorwhitney
Copy link
Collaborator

What this PR does / why we need it:

When the query size limiter can't limit the query, fail through to the next middleware instead of erroring. This can happen, for example, when a query spans schemas, which is still a valid query case, so we want to make sure to fall back to existing behavior.

* When the query size limiter can't limit the query, fail through to the
  next middleware instead of erroring. This can happen, for example,
  when a query spans schemas.
@grafanabot
Copy link
Collaborator

Hello @trevorwhitney!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@trevorwhitney trevorwhitney added the type/bug Somehing is not working as expected label Apr 5, 2023
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

nit, lgtm

pkg/querier/queryrange/limits.go Outdated Show resolved Hide resolved
Co-authored-by: Owen Diehl <ow.diehl@gmail.com>
@trevorwhitney trevorwhitney changed the title Fail through to next middleware when cant limit Fail through to next middleware when querySizeLimit cannot be applied Apr 5, 2023
@trevorwhitney trevorwhitney merged commit c587b53 into grafana:main Apr 5, 2023
grafanabot pushed a commit that referenced this pull request Apr 5, 2023
…#9050)

**What this PR does / why we need it**:

When the query size limiter can't limit the query, fail through to the
next middleware instead of erroring. This can happen, for example, when
a query spans schemas, which is still a valid query case, so we want to
make sure to fall back to existing behavior.

---------

Co-authored-by: Owen Diehl <ow.diehl@gmail.com>
(cherry picked from commit c587b53)
trevorwhitney added a commit that referenced this pull request Apr 6, 2023
…applied (#9051)

Backport c587b53 from #9050

Co-authored-by: Trevor Whitney <trevorjwhitney@gmail.com>
@@ -353,7 +353,8 @@ func (q *querySizeLimiter) Do(ctx context.Context, r queryrangebase.Request) (qu
// Only support TSDB
schemaCfg, err := q.getSchemaCfg(r)
if err != nil {
return nil, httpgrpc.Errorf(http.StatusInternalServerError, "Failed to get schema config: %s", err.Error())
level.Error(log).Log("msg", "failed to get schema config, not applying querySizeLimit", "err", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a warning level.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed: #13029

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS type/bug Somehing is not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants