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

Make httpgrpc.Server return 4xx errors #455

Closed
wants to merge 1 commit into from
Closed

Conversation

duricanikolic
Copy link
Contributor

@duricanikolic duricanikolic commented Dec 20, 2023

What this PR does:
Currently, httpgrpc's Server.Handle() returns an error only if a response received from the HTTP server contains a 5xx HTTP status code. Responses with any other HTTP status code (e.g., 4xx) are converted to non-erroneous HTTPResponse objects. As a consequence, the correlated status_code labels in the request duration metrics is success.

This PR allows Server.Handle() to return errors even in case of a 4xx HTTP status code. Namely, the presence of a header with key httpgrpc.Return4XXErrorsKey and any value in a response received from HTTP server indicates to Server.Handle() to convert that response to an error. This way the correlated status_code labels in the request duration metrics will be 4xx.

Which issue(s) this PR fixes:

Part of grafana/mimir#1830

Checklist

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

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
@pracucci
Copy link
Contributor

I discourage using an header to enable the behaviour. The feature flag approach is better. I would close this PR in favour of #457.

@duricanikolic
Copy link
Contributor Author

I am closing this PR in favour of #457

@duricanikolic duricanikolic deleted the yuri/4xx-errors branch December 21, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants