Improving globalerror package

[duricanikolic]

What this PR does

This PR refactors the globalerror package in such a way that it exports the following functions:

• ToGRPCStatusError(), that returns an error corresponding to a gRPC status built out of the given parameters: the gRPC status' code and details are passed as parameters, while its message corresponds to the original error. The resulting error is of type error, and it can be parsed to the corresponding gRPC status by both gogo/status and grpc/status packages.
• WrapErrorWithGRPCStatus(), that wraps a given error with a gRPC status, which is built out of the given parameters: the gRPC status' code and details are passed as parameters, while its message corresponds to the original error. The resulting error is of type globalerror.ErrorWithStatus. This function replaces the previously available globalerror.NewErrorWithGRPCStatus() function.
• WrapGRPCErrorWithContextError(), that checks if the given error is a gRPC error corresponding to a standard golang context error, and if it is, wraps the former with the latter. If the given error isn't a gRPC error, or it doesn't correspond to a standard golang context error, the original error is returned. This function replaces the previously available globalerror.WrapGrpcContextError() function.

This PR also replaces usages of globalerror.NewErrorWithGRPCStatus() in storegateway and querier packages with the new globalerror.ToGRPCStatusError(), because the latter is more appropriate.

Which issue(s) this PR fixes or relates to

Fixes #

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.

[dimitarvdimitrov]

generally I didn't see any blockers with the PR. But I'm wondering if we really need 2 different ways to wrap GRPC errors - ToGRPCStatusError and WrapErrorWithGRPCStatus. Wouldn't it be simpler if there was less choice around wrapping GRPC errors?

[dimitarvdimitrov]

pointer receivers on a method sometimes indicate that the type can be mutated by the method. Can't we get away with value receivers - (e ErrorWithStatus)

[duricanikolic]

Initially I implemented it as you suggested. The problem is that with a value receiver ErrorWithStatus.Causes was not correctly passed form one method to another. That's why I decided to implement it this way.

But if you have other suggestions, I am happy to hear them.

This was the problem:

_, err := i.Push() // the result of i.Push()  was ingester.errPushGrpcDisabled
require.ErrorIs(t, err, errPushGrpcDisabled) // this was failing because err had the Causes field different from the actual one

[dimitarvdimitrov]

When the values are pointers, then I think we hit this condition and the actual pointer addresses are compared instead of the errors https://cs.opensource.google/go/go/+/refs/tags/go1.22.3:src/errors/wrap.go;l=55-57

This became a problem as Causes became a slice and slices make structs not comparable. Which makes me have second thoughts about having a slice in the struct. Maybe we can use fmt.Errorf("%w: %w", err, context.Canceled) in this place and avoid needing to store a slice ourselves. Or simply store two booleans in ErrorWithStatus indicating whether the error is a context cancelation or a deadline exceeded or neither.

[duricanikolic]

Hi @dimitarvdimitrov ,
I have removed pointer receivers, and returned ErrorWithStatus from every function that used to return *ErrorWithStatus. I have also removed the Causes []error slice, and put back the previous UnderlyingErr error, but added another field ctxErr err that gets assigned a value only after WrapGRPCErrorWithContextError () is called.

Similarly, Unwrap() []error returns []error{e.UnderlyingErr} or []error{UnderlyingErr, ctxErr} depending on whether ctxErr is set or not.

[dimitarvdimitrov]

not very actionable, but decided to bring this up anyways

this changes the contract of this type. From what I could find implementing Unwrap() []error will still work with errors.Is() (code), but not with errors.Unwrap() (code). I think that should be fine because we don't use errors.Unwrap() in the Mimir repo.

[duricanikolic]

Hi @dimitarvdimitrov,
This is what the errors package documentation states:

An error e wraps another error if e's type has one of the methods

Unwrap() error
Unwrap() []error

So it is enough to implement one of these two in order to have correct error wrapping. Moreover, by doing it this way we can remove the Is() and As() functions that the grpcContextError type implemented.

[duricanikolic]

generally I didn't see any blockers with the PR. But I'm wondering if we really need 2 different ways to wrap GRPC errors - ToGRPCStatusError and WrapErrorWithGRPCStatus. Wouldn't it be simpler if there was less choice around wrapping GRPC errors?

If this is the way we decide to go I can do it.
I opted for 2 separate methods because it is not always required to preserve the original error semantics. The status package itself doesn't preserve it, so I wanted to have both options, since both of them are actually needed in Mimir.

[dimitarvdimitrov]

generally I didn't see any blockers with the PR. But I'm wondering if we really need 2 different ways to wrap GRPC errors - ToGRPCStatusError and WrapErrorWithGRPCStatus. Wouldn't it be simpler if there was less choice around wrapping GRPC errors?

If this is the way we decide to go I can do it. I opted for 2 separate methods because it is not always required to preserve the original error semantics. The status package itself doesn't preserve it, so I wanted to have both options, since both of them are actually needed in Mimir.

not feeling strongly about this. My consideration was for please new to the codebase or just everyone else who yet hasn't familiarized with error handling in the system. Are there drawbacks to always keeping the original error?

[duricanikolic]

generally I didn't see any blockers with the PR. But I'm wondering if we really need 2 different ways to wrap GRPC errors - ToGRPCStatusError and WrapErrorWithGRPCStatus. Wouldn't it be simpler if there was less choice around wrapping GRPC errors?

If this is the way we decide to go I can do it. I opted for 2 separate methods because it is not always required to preserve the original error semantics. The status package itself doesn't preserve it, so I wanted to have both options, since both of them are actually needed in Mimir.

not feeling strongly about this. My consideration was for please new to the codebase or just everyone else who yet hasn't familiarized with error handling in the system. Are there drawbacks to always keeping the original error?

Hi @dimitarvdimitrov,
I have pushed another commit where I removed ToGRPCStatusError(), but I added the following function on ErrorWithStatus:

// Err returns an immutable error representing this ErrorWithStatus.
// Returns nil if UnderlyingError is nil or Status.Code() is OK.
// The resulting error is of type error, and it can be parsed to
// the corresponding gRPC status by both gogo/status and grpc/status
// packages.
func (e ErrorWithStatus) Err() error {
	if e.UnderlyingErr == nil {
		return nil
	}
	if e.Status.Code() == codes.OK {
		return nil
	}
	return e.Status.Err()
}

A similar function exists in both gogo and grpc status packages, and it is used to return an error corresponding to the status itself.

[duricanikolic]

.Err() is not mandatory

[duricanikolic]

.Err() is not mandatory, it is just used to remove semantic connection with stGwErr

[dimitarvdimitrov]

LGTM