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

Store: fix error handling on limits #6171

Merged
merged 3 commits into from
Feb 28, 2023

Conversation

douglascamata
Copy link
Contributor

@douglascamata douglascamata commented Feb 28, 2023

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

I noticed from running Thanos Store in production that limits were being hit quite often and this triggered an alert based on percentage of failed requests. A deep investigation allowed me to identify that the grpc codes for requests hitting limits made no sense: they were mostly "Unknown" and "Aborted". This motivated me to fix such logic.

  • Removed a test implementation of the series and samples limiter factories used in a test to test its own behavior.
  • Now such test uses the real implementation of such limiters.
  • Fixed the error handling from the limiters, allowing a proper gRPC status code to be returned when limits are hit: ResourceExhausted.

Verification

E2E tests changed and passing.

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
squat
squat previously approved these changes Feb 28, 2023
Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

Nice find

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
@douglascamata
Copy link
Contributor Author

douglascamata commented Feb 28, 2023

As always, I forget to goimports my files 😭

Fixed in 60cf9a0.

Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Thanks, do we need to fix this in other stores?

@douglascamata
Copy link
Contributor Author

@fpetkovski from what I checked just now, yes. I can follow up with a PR fixing things around the limitedStoreServer. 😃

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.

4 participants