ruler: don't retry on non-retriable error

[narqo]

What this PR does

These changes improve the internals of the retries in ruler.RemoteQuerier. That is, the calls inside Query shouldn't retry, if it stumbles upon a well-known non-retriable errors.

Which issue(s) this PR fixes or relates to

Fixes #6609

Checklist

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

[narqo]

Note, these tests duplicate what is already covered in TestRemoteQuerier_QueryRetryOnFailure. I don't think, testing the actual number of retries worth this number of code. The backoff is an internal implementation detail (_that's IMHO, and I don't feel strong about it).

[narqo]

Note: these are outside the scope of changes. It was just confusing, while I was reading through the code, that we use non-gRPC codes in gRPC statuses, while their docs explicitly ask not doing that (I'm now aware that httpgrpc has different semantics for the codes, while keeping the same implementation) ;)

[dimitarvdimitrov]

are there other errors with ResourceExhausted code which we might want to retry?

I'm thinking about cases where the ingesters are overwhelmed and return a ResourceExhausted. In this case I think it makes sense to retry in the ruler. Giving up means data loss (failure to evaluate a query and not writing the result). Retrying has some chance of succeeding eventually

Also is there a way to only not retry the errors when the response is too large?

[narqo]

Are there other errors with ResourceExhausted code which we might want to retry?
···
Also is there a way to only not retry the errors when the response is too large?

I looked around and can't say for sure. There are modules where, historically, the ResourceExhausted is used as a signal, that the request was rate-limited. We expect the client to retry their request with a backoff in such a case.

After looking around other projects, e.g. OTel or Google API clients, I feel there is not one universal rule on which statuses are retriable :/ I decided to update the PR to skip retrying only on exceeding the max-msg-size. The only way to do that is not too pretty, but it might be ok, as an optimisation for a failure path.

[duricanikolic]

As discussed via Slack, the initial idea was to make all errors with codes.ResourceExhausted status code fail. Some of these errors come from gRPC itself (e.g., https://github.com/grpc/grpc-go/blob/84b85babc00b4b8460e53b6ee110bfb49e9311cf/rpc_util.go#L610-L615). On the other hand, there are some errors with status code codes.ResourceExhausted returned by distributors and ingesters that should be retried. These cases are:

1. ingester too busy: the returned error has codes.ResourceExhausted status code, but also contains a mimirpb.ErrorDetail with cause mimirpb.TOO_BUSY and should always be retried
2. ingestion rate limit hit: the returned error has codes.ResourceExhausted status code, but also contains a mimirpb.ErrorDetail with cause mimirpb.INGESTION_RATE_LIMITED and should be retried if distributor's -distributor.service-overload-status-code-on-rate-limit-enabled is set to true
3. request rate limit hit: the returned error has codes.ResourceExhausted status code, but also contains a mimirpb.ErrorDetail with cause mimirpb.REQUEST_RATE_LIMITED and should be retried if distributor's -distributor.service-overload-status-code-on-rate-limit-enabled is set to true

So I recommend to check whether the error contains a detail of type mimirpb.ErrorDetails with one of the causes mentioned above and to retry when needed.

An example:

stat, ok := grpcutil.ErrorToStatus(err)
if ok {
	switch code := stat.Code() {
		case codes.ResourceExhausted:
			details := stat.Details()
			if len(details) != 1 {
				return nil, err
			}
			errDetails, ok := details[0].(*mimirpb.ErrorDetails)
			if !ok {
				return nil, err
			}
			if errDetails.GetCause() == mimirpb.INGESTION_RATE_LIMITED 
				|| errDetails.GetCause() == mimirpb.REQUEST_RATE_LIMITED
				|| errDetails.GetCause() == mimirpb.TOO_BUSY {
				continue
			}
			return nil, err
		default:
			// In case the error was a wrapped HTTPResponse, its code represents HTTP status;
			// 4xx errors shouldn't be retried because it is expected that
			// running the same query gives rise to the same 4xx error.
			if code/100 == 4 {
				return nil, err
			}
		}
}

IMPORTANT: this code is just an example, and it doesn't take into account the -distributor.service-overload-status-code-on-rate-limit-enabled flag.

[duricanikolic]

After another chat with @narqo I actually realized that mimirpb.INGESTION_RATE_LIMITED and mimirpb.REQUEST_RATE_LIMITED could be thrown on the write path only, while the present PR concerns the read path. Therefore, the -distributor.service-overload-status-code-on-rate-limit-enabled CLI flag is not important, and the implementation can be simplified.

stat, ok := grpcutil.ErrorToStatus(err)
if ok {
	switch code := stat.Code() {
		case codes.ResourceExhausted:
			details := stat.Details()
			if len(details) != 1 {
				return nil, err
			}
			errDetails, ok := details[0].(*mimirpb.ErrorDetails)
			if !ok {
				return nil, err
			}
			if errDetails.GetCause() == mimirpb.TOO_BUSY {
				continue
			}
			return nil, err
		default:
			// In case the error was a wrapped HTTPResponse, its code represents HTTP status;
			// 4xx errors shouldn't be retried because it is expected that
			// running the same query gives rise to the same 4xx error.
			if code/100 == 4 {
				return nil, err
			}
		}
}

[dimitarvdimitrov]

On a second thought I'm not sure I agree with this PR. The original issue was to not retry queries which are known to be rejected on the next retry (e.g. because the response is too big). With this PR rules will become less resilient to temporary overloads on the read path. Given that we don't support rules backfilling the missing evaluation means data loss or less reliable alerting. IMO increased load on the read path is more tolerable than data loss.

Can't we detect only the specific error which #6609 mentions and not retry only then?

[dimitarvdimitrov]

Can't we detect only the specific error which #6609 mentions and not retry only then?

I was only looking at the comments Yuri left without actually looking at the code. Code seems to be doing exactly this.

[dimitarvdimitrov]

I left a minor comment about the changelog, but otherwise LGTM. Thanks for working on this.

[dimitarvdimitrov]

spoke with @duricanikolic privately and we agreed that since the error string is emitted as a string from the gRPC code there's little we can do have more reliable parsing than substring matching. If we get more similar string matching in the future around gRPC maybe we can consider making a change upstream to surface those errors in some way.