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

fix: properly return http status codes from ingester to querier for RPC function calls #13134

Merged
merged 5 commits into from
Jun 7, 2024

Conversation

cstyan
Copy link
Contributor

@cstyan cstyan commented Jun 5, 2024

We noticed this earlier with incorrect query syntax on a recording rule that was then sent to an ingester. The error returned to the end user had an HTTP 500 status code without much information because the grpc server was not seeing a valid status code in the result from the ingester, and was wrapping the error as a generic 500 status code.

@slim-bean and I discussed on slack and we think all functions the Querier can call on the Ingester via RPC should use the same error wrapping as we have in our server utils code. Looking at the generated GRPC code, this should be:

  • Query
  • QuerySample
  • Label
  • Series
  • Tail
  • TailersCount
  • GetChunkIDs
  • GetStats
  • GetVolume
  • GetDetectedLabels
  • GetDetectedFields (this one can never return an error afaict)

Note that I have just done the easy thing for now, and I'll simplify the duplicated code in the morning

Signed-off-by: Callum Styan <callumstyan@gmail.com>
pkg/ingester/ingester.go Outdated Show resolved Hide resolved
Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Callum Styan <callumstyan@gmail.com>
@cstyan cstyan marked this pull request as ready for review June 5, 2024 20:40
@cstyan cstyan requested a review from a team as a code owner June 5, 2024 20:40
@cyriltovena
Copy link
Contributor

While this is a good idea to have. I think we should investigate how such queries end up on ingester, we should fail fast if this is not good syntax a querier or query-frontend should have caught it.

This was brought up during incident review meeting.

Signed-off-by: Callum Styan <callumstyan@gmail.com>
@cstyan
Copy link
Contributor Author

cstyan commented Jun 6, 2024

we should fail fast if this is not good syntax a querier or query-frontend should have caught it.

Agreed, and I will look into this more. Do you have any objections to merging this as is while we continue to investigate why these queries are making it to the ingester in the first place?

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@cyriltovena cyriltovena merged commit 691b174 into main Jun 7, 2024
59 checks passed
@cyriltovena cyriltovena deleted the callum-ingester-httperr branch June 7, 2024 11:02
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