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

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

[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.