Fix regression that caused client errors to be tracked in cortex_ruler_write_requests_failed_total #7472
Conversation
…r_write_requests_failed_total Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
| @@ -293,7 +293,7 @@ func wrapPartitionPushError(err error, partitionID int32) error { | |||
| return errors.Wrap(err, fmt.Sprintf("%s %d", failedPushingToPartitionMessage, partitionID)) | |||
| } | |||
|
|
|||
| func isClientError(err error) bool { | |||
| func isIngesterClientError(err error) bool { | |||
There was a problem hiding this comment.
"client" here refers to client pushing data to distributor, right? I don't see how this rename makes sense in that context.
There was a problem hiding this comment.
To my understanding, not in this context. This function only checks ingester errors and not distributor errors, that's why I named it specifically isIngesterClientError().
@duricanikolic Is my understanding correct or not?
There was a problem hiding this comment.
Among other arguments of dskit's ring.DoBatchWithOptions() there are:
- a callback, which is the actual function we want to invoke on a set of instances, and that might return an error
- an optional
ring.DoBatchOptionscontaining aIsClientErrorfunction, used to distinguish between client and server errors returned by the callback function.
In this particular context, distributor.push() calls ring.DoBatchWithOptions() and passes ingester.Push() as a callback and isIngesterClientError (ex isClientError) as ring.DoBatchOptions.IsClientError function. This function should, therefore, distinguish between ingester's client and server errors.
So, @pracucci has a correct understanding.
What this PR does
Today I've been paged by
MimirRulerTooManyFailedPushes. The Mimir cluster health was good and there was no critical error on the write path. However, some recording rules were failing to append samples to ingesters because of:In Mimir we consider such error a client error, because we have no control over it (e.g. recording rules generating clashing series). For this reason, in the ruler we have a logic to not count client errors in
cortex_ruler_write_requests_failed_total. However, the recent work on gRPC errors introduced a regression, because the ruler code looks for a HTTPgRPC 4xx status code, but now theDistributor.Push()doesn't return a HTTPgRPC anymore.In this PR I've fixed it introducing
mimirpb.IsClientError()utility. The reason why I've placed it inmimirpbis because the error codes are defined there and I needed to use the same utility both inpkg/storage/ingest(used by ingest storage) andpkg/ruler.I've also keep backward compatibility because it's something we also have in other places where we handle such errors, but I'm open to feedback.
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.