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

Distributor: improve wrapping of gRPC errors returned by the ingester #6426

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

duricanikolic
Copy link
Contributor

What this PR does

This PR fixes a regression introduced by #6416, where we replaced usages of grpc/status with gogo/status for ingester's gRPC error creation.
Namely, if an error created by gogo's status.Error() is wrapped (for example by using errors.Wrap() or fmt.Errorf("...%w..."), the resulting error is not recognized by status.FromError(). In the case of grpc's status.Error() this was not the case, i.e., even wrapped errors were recognized by grpc's status.FromError().
Therefore, the only place where ingester errors are wrapped (Distributor.handleIngesterPushError()) must be updated in such a way that even the wrapped gRPC errors coming from the ingester preserve their type and semantics.

Which issue(s) this PR fixes or relates to

Part of #6008

Checklist

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

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
@duricanikolic duricanikolic requested a review from a team as a code owner October 18, 2023 14:52
Copy link
Collaborator

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

I see, LGTM

@duricanikolic duricanikolic merged commit ef9b288 into main Oct 18, 2023
28 checks passed
@duricanikolic duricanikolic deleted the yuri/handling-errors-new branch October 18, 2023 15:29
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.

2 participants