Metric bucket should not return error when error is expected

[yeya24]

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

Now, when metric bucket got an error which is isOpFailureExpected, it doesn't increment the metric but still returns the error back to the caller.

For example, it is quite common to log the error returned by bucket operations like below. However, even if I already specify IsXXXError as expected, the error still returns and will be logged. To filter it out, I need to add another IsXXXError(err) check to not log the error, which is very redundant to me.

err := userBucket.ReaderWithExpectedErrs(IsXXXError).Iter(...)
level.Log("msg", "got error", "err", err)

We also have another usecase which doesn't support checking the error in the caller. https://github.com/cortexproject/cortex/blob/master/pkg/storage/bucket/s3/bucket_client.go#L135

We implemented our own storage provider which has retries. Since the error is returned, there is no way for the retryer to know if the error is expected or not and it will keep retrying and finally log the error. There is no way to inject the expected error to the bucket client itself in this case.

Verification

[JoaoBraveCoding]

Just a small comment apart from that lgtm

[JoaoBraveCoding]

We can omit this one no?

Suggested change

return err

[yeya24]

Thanks. Good catch!
Addressed in the latest commit

[JoaoBraveCoding]

lgtm

[aknuds1]

To filter it out, I need to add another IsXXXError(err) check to not log the error, which is very redundant to me.

Isn't it better to be explicit about this though, than to assume that the caller doesn't want to know that an error occurred?

This change breaks Mimir in many places, and I see the following problems with it:

1. The doc comment is no longer correct, as it only says that "it will not increment objstore_bucket_operation_failures_total metric". It doesn't mention that expected errors are swallowed.
2. The caller has to e.g. check whether BucketReader.Get returns a nil io.ReadCloser in case the error is nil, for the case when an expected error is encountered.
3. InstrumentedBucket.WithExpectedErrs is intended to concern itself with metrics, but whether to return an error or not is completely beyond that scope and violates the principle of least surprise.

In sum, I think this PR should be reverted. It would be better to either add another method, as an alternative to WithExpectedErrs that swallows expected errors, or leave to the caller to ignore expected errors (as is the case now).