docs: Tango Errors RFC

[justinwon777]

Adds a Tango Errors RFC covering the core/errors package and its APIs

[sbalabanov]

Few things are missing in the RFC design:

• Clear error domain definition. What types of errors could ever be in the system? (see comments on data structures).
• Guidelines on using classifiers; where exactly should classification be done and what data will it use. In the provided example, bazel query error may be both infra (i.e. no bazel binary on the machine) or user (i.e. change is incompatible with the trunk). Infra might be retryable (i.e. bazilisk failed with http error) or not (i.e. incorrect argument list to this specific bazel version).
• How internal classification translates to the API layer (GRPC codes and rich error object). GRPC defines its own codes which cannot be changed or extended, typically observable by common infrastructure (i.e. muttley).
• Missing dependency flag. We would like to compute inclusive and exclusive reliability metric for the service, i.e. it should differentiate between its own errors (tango process + subprocesses it runs) and external systems it calls (i.e. CI or storage).

[sbalabanov]

we should be able to differentiate between missing arg (which is a programming error on a caller side) vs build failure (which is a user error not related to the ecosystem correctness)

[justinwon777]

I agree we should eventually have different error codes for different types of user failures like canceled and build failure, but I think we should take advantage of that once our error classification is more mature. For instance, right now there is no way for Tango to know why a buildkite build failed, so there is no use of having a build failure error code.

[justinwon777]

I added a cancelled error code since we see that one a lot and think it'd be helpful to distinguish cancelled from other user failures.

[justinwon777]

@sbalabanov partial response, still looking into your GRPC codes comment

• Guidelines on using classifiers; where exactly should classification be done and what data will it use. In the provided example, bazel query error may be both infra (i.e. no bazel binary on the machine) or user (i.e. change is incompatible with the trunk). Infra might be retryable (i.e. bazilisk failed with http error) or not (i.e. incorrect argument list to this specific bazel version).

I added a more detailed example of the usage. This package's purpose is to provide the framework for callers to classify an error, and the caller is responsible for determining how to classify an error. At the controller layer, the mapper will handle all errors and convert them to the proto type (unclassified will be unknown error code). Some packages like bazel will need more work done to differentiate user and infra failures.

• Missing dependency flag. We would like to compute inclusive and exclusive reliability metric for the service, i.e. it should differentiate between its own errors (tango process + subprocesses it runs) and external systems it calls (i.e. CI or storage).

Yes we could potentially add two more error codes ErrorInternalDependency and ErrorInternalDependencyRetryable. In a future task we can identify external systems that can use these and then add the error codes.

[justinwon777]

• How internal classification translates to the API layer (GRPC codes and rich error object). GRPC defines its own codes which cannot be changed or extended, typically observable by common infrastructure (i.e. muttley).

Added the gRPC code mapping to the error code definitions

[sywhang]

Do you want to implement fmt.Formatter for this type? See https://pkg.go.dev/github.com/pkg/errors#hdr-Formatted_printing_of_errors.

If you impl this, the errors will get a Verbose field populated using the formatter when you log it thru Zap.

[justinwon777]

Considering we're going to log errors with the zap fields, the error, failure source, and error code will be in the log, so I don't think having another verbose field would add anything not already in the log.

[sbalabanov]

potential counter case
let's say two very independent functions use storage function Get()
let's say storage implementation is mysql, and it returns mysql-specific "connection failure" error code as mysql error object, which is infra_retryable error
will both call sites run an independent classification of such an event? How do they know they have to classify mysql exit codes and cast to mysql error type?

[justinwon777]

I would expect Get() to classify the error, so the two independent functions receive the classified error. I don't think it should be the caller's responsibility to know how to handle storage errors. Same with when SQ calls Tango it'd be better if Tango returned the classified error than SQ classifying it right? Which is why we're making this errors package.

[justinwon777]

Following our discussion I updated it so component packages will return sentinels and top level layers that call the component will check for the sentinel and classify it.

[justinwon777]

A little concerned if different layers call the same component then each layer needs to implement classifying the same sentinels. I haven't seen a case yet where Tango would classify the same sentinel differently, so the classifying logic could get redundant. Although I think there aren't that many user/infra-retryable errors in Tango, so it's not like we're checking sentinels on every single error.

[sbalabanov]

not just validation

[justinwon777]

Yeah I only put validation here cause currently in OSS we only have validation errors, but in internal service we will have other possible user failures like build failures. Updated so it doesn't seem like it's only validation failures.

[sbalabanov]

I do not think the sentinel is a good example as it does not show the reason of the error.
HTTP download can fail because connection to a server fails (likely recoverable) or if the required version does not exist on a server (404) - likely not recoverable.
Here both of them are combined.

[justinwon777]

The doc for Do says it can return a network error and non 2xx error codes are not returned in err, but in the response body with nil err. I added a net.Error check to the example.