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

Replace validation error with error in query limiter #4240

Merged
merged 2 commits into from
Jun 2, 2021

Conversation

treid314
Copy link
Contributor

@treid314 treid314 commented Jun 1, 2021

Signed-off-by: Tyler Reid tyler.reid@grafana.com

What this PR does:
This is an internal refactor to return an Error from the query limiter and wrap to be returned as a validation error in the ingester and blocks storage queryables.

Which issue(s) this PR fixes:
Fixes comments from PR #4216

Checklist

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

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Copy link
Contributor

@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.

Looking at code changes, LGTM! I've seen CI has failed. Going to restart it cause I think it's due to a couple of flaky tests and not related to this change.

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you!

…ntained string

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
_, err = c.QueryRange("{__name__=~\"series_.+\"}", series1Timestamp, series4Timestamp.Add(1*time.Hour), blockRangePeriod)
require.Error(t, err)
assert.Contains(t, err.Error(), "max number of series limit")
assert.Contains(t, err.Error(), "500")
Copy link
Contributor Author

@treid314 treid314 Jun 1, 2021

Choose a reason for hiding this comment

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

For some reason after this change, err.Error() is returning encoded strings rather than plain strings, so the assert contains no longer works. Instead of checking the response string, I checked for 500 like we do in some other tests. Is there a better way to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct. We do not expect a 500 error if the limit is reached, but a 4xx error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh :( You're right. My bad for overlooking that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm investigating it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR fixes it:
#4251

@pstibrany pstibrany merged commit 4c56545 into cortexproject:master Jun 2, 2021
pracucci added a commit to pracucci/cortex that referenced this pull request Jun 3, 2021
Signed-off-by: Marco Pracucci <marco@pracucci.com>
pracucci added a commit that referenced this pull request Jun 3, 2021
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants