Re: RFC: error codes on exit

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 20, 2021, 17:47 UTC
Message-ID: <YKaguiSjewjpvOj5@google.com>
In-Reply-To: <YKZj/s/9dp4Oo7aB@coredump.intra.peff.net>

Hi,

Jeff King wrote:

> On Wed, May 19, 2021 at 04:34:24PM -0700, Jonathan Nieder wrote:

>> One kind of signal we haven't been able to make good use of is error
>> rates.  The problem is that a die() call can be an indication of

[... list of some categories snipped ...]

Show 28 quoted lines

> I've run into this problem, too. If you run a website that runs Git
> commands on behalf of users and try to get metrics on failing exit
> codes, it's hard to tell the difference between "the repo is broken",
> "Git has a bug", "the user (or other caller) asked for something
> stupid", and "some transient error occurred".
>
> But I'm not sure that even Git can always tell the difference between
> those things. Some real-world examples I've run into:
>
>   - "rev-list $oid" can't find object $oid. Is the repo corrupt? Or is
>     the caller unreasonable to ask for that object? Or was there a race
>     or other transient error which made the object invisible?
>
>   - upload-pack is writing out a packfile, but gets EPIPE. Did the
>     network drop out? Or is a Git bug causing one side to break
>     protocol?
>
> Some rough categorization may help, but a lot of those need to propagate
> the specific errors back to the caller. For instance, the rev-list
> example could be FAILED_PRECONDITION in your terminology. But really, we
> want to tell the caller "the object you asked for doesn't exist". And
> then it can decide if that was user error (somebody hitting a URL for an
> object that we have no reason to think exists), or a sign of problems
> elsewhere in the system (if we just got $oid from Git, we expect it to
> be there).
>
> So it seems like the most useful thing is specific error codes for
> specific cases.

As a bit of precedent: in the server we have both of these: we have application-specific error codes like AUTHENTICATOR_EXPIRED, and we have generic codes that those map to like UNAUTHENTICATED. The application-specific codes tend to be useful for ad hoc queries as part of incident response, versus the generic codes that have been more useful for defining an SLO (because they are about "how do I want to respond to this error" instead of about the cause).

More specifically for the "missing object" case: it's common enough for a user to ask for an object that doesn't exist that we indeed call it FAILED_PRECONDITION, which has worked well. We have other monitoring in place for checking that all repositories pass fsck and that fetching an object after pushing it succeeds. (In general, this kind of case is very common in monitoring any service that has state.)

>                 And that gets very daunting to think about annotating
> and communicating about each such case (we don't even pass that level of
> detailed information inside the program in a machine-readable way;
> scraping stderr is the best way to figure this stuff out now).

This feels like good news to me: it sounds like if we add application-specific codes like MISSING_OBJECT to Git, then it would be useful to both of us.

The mapping to HTTP-status-style generic codes could then wait for later, to be submitted if and when others have interest. (I.e., that part is easy to keep maintained internally.)

So I'm feeling encouraged. :)

> I dunno. Maybe a rougher categorization would help your case, but not
> mine. But I'm a bit skeptical that we'll have enough coverage of various
> conditions to be useful, and that it won't turn into a headache trying
> to categorize everything.

Two more points I want to emphasize:

 1. We don't have to be exhaustive: as Felipe suggested, it's fine for
    some errors (even most error paths!) to use a code such as
    UNKNOWN.  I care more about coverage of commonly occuring errors
    than categorizing everything, especially because this sets up a
    feedback loop that can lead to improved coverage over time.

 2. By focusing on the practical and ignoring everything else, I think
    we can avoid this becoming an unbounded taxonomy exercise.  That's
    part of the appeal of code.proto /
    https://github.com/abseil/abseil-cpp/blob/HEAD/absl/status/status.h
    for me: by using a preexisting list of codes based on "here is
    what a user would be expected to do in response to this error",
    they make the error classification decision relatively simple.  I
    think we can maintain that kind of simplicity with a Git-specific
    enum, too, so I think this is doable (and I'd make sure to be
    available over time to help answer questions about the
    classification as the project gets used to it).

Thanks, Jonathan