chore: move ErrorWithStatus to globalerror

[dimitarvdimitrov]

What this PR does

This PR moves ErrorWithStatus from the ingester package to a shared package. I need this change so that in #7777 the store-gateway and querier can start using the same error pattern that the distributor<>ingester requests use.

Context

ErrorWithStatus is internally used to wrap errors in the ingesters before returning them to distributors. It helps with 1) retaining the original error (in contrast with gogoproto and grpc packages which retain the error string) and with 2) attaching mimirpb error causes to responses returned over gRPC.

Necessary changes

• Move WrapGrpcContextError from pkg/util/grpc_error.go to pkg/util/globalerror/grpc.go. This was necessary because ErrorWithStatus uses pkg/mimirpb and keeping it in pkg/util creates an import cycle
• Rename pkg/util/globalerror/errors.go → pkg/util/globalerror/user.go. The package is named error anyways. The content of that file is user-visible errors and error codes (exceeding limits, validation, etc.). The new file contains grpc error handling.
  • there are other places that handle general errors: pkg/api/error/error.go (seems read-path related), pkg/mimirpb/errors.go (seems like a good fit too)

Note to reviewers

• the only new code is TestErrorWithStatus in pkg/util/globalerror/grpc_test.go
• the changes in pkg/distributor/errors.go aren't strictly necessary. I took the chance to refactor that as well to reduce "similar but different" code. It changes the semantics, but shouldn't change the user-visible behaviour. The returned error still implements GRPCStatus() and error. This means the ruler should be able to use the error in the same way (code) and the grpc library should as well (code)
  • if this is a contentious point I am happy to take it out
  • removed in 7713def

Checklist

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

[pracucci]

Good job! I agree with this preparatory refactoring. Thanks for the detailed PR description. I left a comment about distributor error (which I'm not sure it's the same thing you mentioned in the PR description) that I would like you to double check.

Also, can you rebase, please? ShouldLog() interface and semantic has changed.

[pracucci]

Look at the code changes in distributor. toGRPCError(). I've the feeling the Error() we'll return from this function will be different. Previously it was gRPC statusError.Error() but now it's the status message (statusError.Error() decorates the status message). I'm not sure whether this is a problem or not, but if my analysis is correct I think we're masquerading a change in a refactoring PR.

[dimitarvdimitrov]

I've the feeling the Error() we'll return from this function will be different

you're right. We're now returning globalerror.ErrorWithStatus instead of status.*statusError.

But also the actual error strings will be something like connection refused - because globalerror.ErrorWithStatus returns purely the message

mimir/pkg/util/globalerror/grpc.go

Lines 105 to 107 in 069ef53

	func (e ErrorWithStatus) Error() string {
	return e.Status.Message()
	}

. Whereas previously they were rpc error: code = Unavailable desc = connection refused because that's what status.*statusError constructs

i still think it's only a cosmetic change, but I can't be certain, so I'll revert that change and leave a comment if anyone feels adventurous in the future

[dimitarvdimitrov]

restored the previous code and added a comment in 7713def

[duricanikolic]

i still think it's only a cosmetic change, but I can't be certain, so I'll revert that change and leave a comment if anyone feels adventurous in the future

Hi @dimitarvdimitrov,
Since I was not involved in reviewing PR, I will reply you now. It is not a cosmetic change.

TL;DR

Errors of type globalerror.ErrorWithStatus are gRPC errors that could be recognized by both grpc and gogo libraries, that preserve the semantics of their underlying error. Regular gRPC errors created by both grpc and gogo libraries do not have that features. Errors produced by ingesters should preserve the underlying semantics, while errors produced by distributors don't have to preserve it (at least not in this moment).

Details

Taken from the current distributor/errors.go:

// errorWithStatus is slightly different from globalerror.NewErrorWithGRPCStatus. It returns status.Err() instead of an error with the status message.
// The actual difference is between "rpc error: code = XYZ desc = message" and just "message".
// At the time of writing this should be purely a cosmetic difference, but it's hard to verify.
// Because of this difference, the function is used instead of globalerror.NewErrorWithGRPCStatus.

This is the change you did in distributor.toGRPCError(). You basically cut one piece of the code and created a function out of it. This function simply creates a gRPC error with details by using gogo's status package: error message corresponds to the original errors's Error(), and the detail is the error cause of type mimirpb.ErrorCause. This way no information about original error's semantics is preserved. For example, if a grpc logging middleware receives this error, it cannot understand whether it was a non-loggable error, or an error that should be sampled etc. Just its error message and its details (used for a communication between different Mimir components) would be sent.

I recommend that we either revert this change or that we remove the comment which introduces confusion (it looks like we are not so sure why we do it this way).

On the other hand, we have the current globalerror.ErrorWithStatus:

// ErrorWithStatus is used for wrapping errors returned by ingester.
// Errors returned by ingester should be gRPC errors, but the errors
// produced by both gogo/status and grpc/status packages do not keep
// the semantics of the underlying error, which is sometimes needed.
// For example, the logging middleware needs to know whether an error
// should be logged, sampled or ignored. Errors of type ErrorWithStatus
// are valid gRPC errors that could be parsed by both gogo/status
// and grpc/status packages, but which preserve the original error
// semantics.

I put this comment when I implemented ErrorWithStatus within the ingester package in order to explain its main purpose, and why it was different from regular gRPC errors created by the two libraries. I think that the explanation (already present in the code) is clear. But if it is not we can rewrite it better.

[pracucci]

Why do they need to be exposed?

[dimitarvdimitrov]

there is an ingester function which uses them. The idea was to have this mostly as a data transfer object which doesn't need a constructor. The existing NewErrorWithGRPCStatus is more of a convenience function to reduce duplication

[dimitarvdimitrov]

there was a faillint rule which prevents globalerror from importing any other package. That changed in this PR because we now import pkg/mimirpb. However, failling isn't flexible enough to add an exception. I also couldn't get golangci-lint with depguard to work as it just seemed to arbitrarily choose which files from the repo to scan.

so I removed the linter rule in 60ee703