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

Get rid of httpgrpc on distributor's write path #6377

Merged
merged 6 commits into from
Oct 17, 2023

Conversation

duricanikolic
Copy link
Contributor

@duricanikolic duricanikolic commented Oct 14, 2023

What this PR does

This PR modifies the existing implementation of Distributor.Push() by returning only errors with gRPC codes coming from the distributor. The only exceptions are the errors returned by the ingester, which can still contain HTTP status codes. This will be fixed in one of the following PRs.

Proto messages WriteErrorDetails and ErrorCause field have been added to the mimirpb package.

enum ErrorCause {
  INVALID = 0;
  REPLICAS_DID_NOT_MATCH = 1;
  TOO_MANY_CLUSTERS = 2;
  BAD_DATA = 3;
  INGESTION_RATE_LIMITED = 4;
  REQUEST_RATE_LIMITED = 5;
}

message WriteErrorDetails {
  ErrorCause Cause = 1;
}

The purpose of WriteErrorDetails is to enrich distributor's gRPC errors with additional details, which closely explain the nature of the error itself. Possible values of ErrorCauses are analog to the distributor error types.

The following table shows the mapping between the error types encountered during Distributor.Push() and the corresponding gRPC codes:

error type gRPC code ErrorCause
validationError codes.FailedPrecondition mimirpb.VALIDATION
ingestionRateLimitedError codes.ResourceExhausted mimirpb.INGESTION_RATE_LIMITED
requestRateLimitedError code.Unavailable or codes.ResourceExhausted mimirpb.REQUEST_RATE_LIMITED
replicasDidNotMatchError codes.AlreadyExists mimirpb.REPLICAS_DID_NOT_MATCH
tooManyClustersError codes.FailedPrecondition mimirpb.TOO_MANY_CLUSTERS
other errors codes.Internal none

The table above does NOT include the errors returned by the ingester which contain either HTTP status codes 400 and 503 or gRPC codes: these errors are propagated in the original form. This will be improved in one of the following PRs.

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]

@duricanikolic duricanikolic force-pushed the yuri/handling-errors-new branch 3 times, most recently from f3b329b to 30140f7 Compare October 16, 2023 08:39
@duricanikolic duricanikolic marked this pull request as ready for review October 16, 2023 09:34
@duricanikolic duricanikolic requested a review from a team as a code owner October 16, 2023 09:34
Comment on lines 146 to 150
// requestRateLimitedError implements the distributorError interface.
func (e requestRateLimitedError) errorCause() mimirpb.ErrorCause {
Copy link
Contributor

Choose a reason for hiding this comment

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

Opinionated nit, but this says the same, plus it's also checked by the compiler and can't get outdated.

Suggested change
// requestRateLimitedError implements the distributorError interface.
func (e requestRateLimitedError) errorCause() mimirpb.ErrorCause {
var _ distributorError = requestRateLimitedError{}
func (e requestRateLimitedError) errorCause() mimirpb.ErrorCause {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have implemented the required change.

@@ -34,6 +34,18 @@ message WriteRequest {

message WriteResponse {}

enum ErrorCause {
REPLICAS_DID_NOT_MATCH = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an INVALID = 0 const here? That would prevent the code from silently being interpreted as REPLICAS_DID_NOT_MATCH if there's a bug somewhere and cause isn't set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do it.

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
return mimirpb.REPLICAS_DID_NOT_MATCH
}

var _ distributorError = replicasDidNotMatchError{}
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these statements for? is that to verify that the type satisfies the interface?
It would be useful (for me) if there was a comment what this does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @replay,
Exactly, these statements are here for verifying that the type satisfies the interface.
I was asked by @colega (here) to add them because this way the compiler complains if we by mistake forget to implement some methods of an interface.
I agree that adding a comment explaining that would be beneficial and will add it.

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 okay with having a comment, although this is a standard Golang statement.

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

@replay replay left a comment

Choose a reason for hiding this comment

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

This looks good to me, thx

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.

3 participants