RFC: error codes on exit

Hi,

(Danger, jrn is wading into error handling again...)

At $DAYJOB we are setting up some alerting for some bot fleets and developer workstations, using trace2 as the data source. Having trace2 has been great --- combined with gradual weekly rollouts of "next", it helps us to understand quickly when a change is creating a regression for users, which hopefully improves the quality of Git for everyone.

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

 a. the user asked to do something that isn't sensible, and we kindly
    rebuked the user

 b. we contacted a server, and the server was not happy with our
    request

 c. the local Git repository is corrupt

 d. we ran out of resources (e.g., disk space)

 e. we encountered an internal error in handling the user's
    legitimate request

and these different cases do not all motivate the same response. (E.g., if (c) affects just a single bot but produces a high error rate from that bot, we shouldn't be alarmed; if (d) is happening on a bot, then we should look into giving it more disk; if (e) is increasing significantly during a rollout then we should roll back quickly.)

In order to do this, I would like to annotate "exit" events with a classification of the error. I'm not too opinionated about what that classification looks like (bikeshedding welcome!) --- e.g., something like the enumeration at https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto is likely to work fine.

(I'm particularly fond of how that maps to HTTP statuses. See also https://github.com/abseil/abseil-cpp/blob/HEAD/absl/status/status.h for an example of using that kind of enumeration within a single process.)

The API could look something like

	--- a/cache.h
	+++ b/cache.h
	@@ -590,6 +590,15 @@ int is_git_directory(const char *path);
	  */
	 int is_nonbare_repository_dir(struct strbuf *path);

	+enum git_error_code {
	+	/*
	+	 * Not an error (= HTTP 200)
	+	 */
	+	OK = 0,
	+};
	+NORETURN void fatal(enum git_error_code code, const char *err, ...)
	+	__attribute__((format (printf, 2, 3)));
	+
	 #define READ_GITFILE_ERR_STAT_FAILED 1
	 #define READ_GITFILE_ERR_NOT_A_FILE 2
	 #define READ_GITFILE_ERR_OPEN_FAILED 3

(with new error codes added when they first get used) and a typical caller could look like

	Subject: xsize_t: tag "cannot handle files this big" as a failed precondition

	Unlike retriable errors, failed preconditions indicate that some
	aspect of the state needs to be changed in order to recover.  Mark
	this error as such to make signals from monitoring in controlled
	environments (e.g., bot fleets or corporate installations of Git)
	easier to understand.

	Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
[...]
	+       /*
	+        * The system is not in a state required for the operation to succeed.
	+        * For example, a file on disk is larger than we can handle.
	+        * (= HTTP 400)
	+        */
	+       FAILED_PRECONDITION = 9,
[...]
	 static inline size_t xsize_t(off_t len)
	 {
		if (len < 0 || len > SIZE_MAX)
	-               die("Cannot handle files this big");
	+               fatal(FAILED_PRECONDITION, "Cannot handle files this big");

Further down the line I can imagine making use of git_error_code elsewhere for e.g. some limited retries of the corresponding transaction when we fail to lock a file.

Thoughts?  Good idea?  Bad idea?

Thanks, Jonathan

Jonathan Nieder wrote:

It's good to not include many initial codes, but I would start with at least three:

  OK = 0,
  UNKNOWN = 1,
  NORMAL = 2,

die() could be mapped to UNKNOWN.

fatal() for me sounds 1) very dramatic, 2), not a verb, 3) and not a complete thing (fatal what?)

I would prefer "fail", or "fault", or anything that is a verb.

Great idea.

On Wed, May 19, 2021 at 6:40 PM Felipe Contreras <felipe.contreras@gmail.com> wrote:

If you go that route, could you please pick a word other than "normal" to describe errors that are not entirely unexpected? I'm worried that someone will see "normal" and use it instead of "OK" to indicate success.

-Alex

On 5/21/21 9:53 AM, Alex Henrie wrote:

<sysexits.h>

	-hpa

On 22/05/21 06.20, H. Peter Anvin wrote:

Looking that header file you mention, I saw:

For EX_USAGE case, we may sometimes display correct usage syntax so that users can fix their typing.

We may use EX_CONFIG when we encounter any errors when parsing .gitconfig.

EX_OSFILE isn't necessary for Git, though.

"H. Peter Anvin" <hpa@zytor.com> writes:

Is the value assignment standardized across systems?

We want human-readable names in the source to help developers while we want platform neutral output in the log so that log collectors can do some "intelligent" things about the output. If EX_USAGE is always 64 everywhere, that is great---we can emit "64" in the log and log collectors can take it as if they saw "EX_USAGE". But if the value assignment is platform-dependent, it does not help all that much.

    Side note.  We had a similar discussion on <errno.h> and
    strerror(); the numbers do not help without knowing which
    platform the error came from, and strerror() output is localized
    and not suitable for machine consumption.

In a sense, it is worse than we keep a central mapping between names programmers use to give to the new fatal() helper function and the string the tracing machinery will emit for these names.

Thanks.

sysexits.h numbers are the same across all platforms which have it, I believe. I think it originated with Sendmail wanting to know a bit more about why any subprocesses exited.

On May 22, 2021 1:49:09 AM PDT, Junio C Hamano <gitster@pobox.com> wrote:

Junio C Hamano wrote:

I think his intention was to suggest to use that list as inspiration... As in have USAGE, NOINPUT, UNAVAILABE, etc.

I would prefer to start with something easy... UNKNOWN = 1, USUAL = 2.

No, please use the standardized numbers when they apply.

On May 22, 2021 2:22:42 PM PDT, Felipe Contreras <felipe.contreras@gmail.com> wrote:

H. Peter Anvin wrote:

I wasn't talking about the numbers, but the names.

Do you see something wrong with USAGE = EX__USAGE?

Why avoid the standard symbols?

On May 22, 2021 2:53:21 PM PDT, Felipe Contreras <felipe.contreras@gmail.com> wrote:

On 21/05/2021 17:53, Alex Henrie wrote:

Typical <== Normal

Though abnormal and atypical often have different implications ;-) P.

Philip Oakley wrote:

Or USUAL.

On Sat, May 22, 2021 at 3:19 PM Felipe Contreras <felipe.contreras@gmail.com> wrote:

The words "typical" and "usual" have the same problem of making it sound like there was no error. I would suggest terms like "user error", "network error", etc. instead.

-Alex

Alex Henrie wrote:

Not to me. "Usual" is typically an adjective, which makes me think: usual what? (something is missing). Although both can be used as nouns ("the usual", "the new normal") in this context "usual" is much less confusing (I don't recall hearing "the command returned usual status").

Jonathan Nieder <jrnieder@gmail.com> writes:

We already have BUG() for e. and die() for everything else, and "everything else" may be overly broad for your purpose.

I am sympathetic to the cause and I agree that introducing a finer-grained classification might be a solution. I however am not sure how we can enforce developers to apply such a manually assigned "error code" cosistently.

Just to throw in a totally different alternative to see if it works better, I wonder if you can teach die() to report to the trace2 stream where in the code it was called from and which vintage of Git it is running.

The stat collection side that cares about certain class of failures can have function that maps "die() at <filename>:<lineno>@<version>" to "what kind of die() it is".

E.g. blame.c:50@v2.32.0-rc0-184-gbbde7e6616" may be BUG(), while blame.c:2740@v2.32.0-rc0-184-gbbde7e6616 may be an user-error.

That way, our developers do not have to do anything special and cannot do anything to screw up the classification.

Junio C Hamano wrote:

BUG() and die() can call fatal().

You don't enforce developers to do this--just like you don't enforce developers to use advice() instead of fprintf(stderr, )... You nudge them in that direction, and eventually it becomes a habit.

Developers in other languages and stacks have no problem with this granularity. They do this in languages like JavaScript, C++, Ruby and Python regularlly. And developers dealing with HTTP have no trouble with error codes (like 200, 400, and 404).

Hi,

Junio C Hamano wrote:

I think two things you're hinting at are "what about maintainability?" and "what is the migration path?"

I suspect that a number of error paths will remain unclassified for a long time, possibly indefinitely. The way I've seen this treated in other tools is that it's okay for something to show up as an INTERNAL error if it doesn't happen frequently: sure, that can cause us a bit of unnecessary worry when it starts occuring more often, but at that point we're in a good place to replace it with something more appropriate.

That means that we would still want to keep die() or some equivalent. That in turn might suggest that the API I suggested is overly verbose; it might make sense to have a different die()-style helper for each type of error, matching what we do with die() and BUG().

Side note: you might wonder why keeping die() would even be a question. For example, there are all the outstanding patches that still use die(); changing such a fundamental API would seem to be a nonstarter. Fortunately, though, the tools in contrib/coccinelle/README allow changing an API in three steps:

 1. Introduce the new API.  Keep the old API around for backward
    compatibility.

 2. Add a "pending" coccinelle semantic patch to automatically
    update callers to the new API.  Update existing callers using
    'make coccicheck-pending'.

 3. Remove the old API and mark the semantic patch as no longer
    pending.  Patches using the old API can be fixed using 'make
    coccicheck'

So we can make this decision based on whether the resulting API is one we like more; in this example, I suspect that keeping die() is preferable _even though_ it would be possible to remove by staying in step 2 for a while without too much fuss.

For ad hoc queries, this is a rather nice tool. Traces already record filename, version, and line number, though I believe in the die() case it currently just points to the implementation of die(). ;-)

However, for analysis in aggregate (for example, to define an SLO[1]) that would require us to maintain a database that maps <filename>:<lineno> to error code. That database would be essentially the same as patches to record the error codes, so what it would really amount to is having these deployments using a permanent fork of Git. It would also get rid of the chance to discuss and improve common error paths on-list.

If we expect the error codes to not be useful to anyone else, then that is the right choice to make (or rather, we'd have to use other heuristics, such as having the traces record a collection of offsets in the binary and a build-id so we can key off of stack trace signatures). Part of the reason I started this thread is to get a sense of whether these can be useful to others.

Thanks, Jonathan

[1] https://sre.google/sre-book/service-level-objectives/

Jonathan Nieder <jrnieder@gmail.com> writes:

Not really. There is not much to migrate from, and mislabeling by developers, or more realistically disagreement between developers what label is appropriate for which condition that leads to die(), would be a usability problem and not maintainability that I would be worried about.

Yes, the suggestion was to start from there to gain experience, because ...

... others will find it useful if the classification matches _their_ needs, but I suspect the "bin" a single die() location wants to be classfied into would end up to be different depending on what the log collecting entity is after.

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

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

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.

-Peff

Hi,

Jeff King wrote:

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

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

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. :)

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

On Thu, May 20, 2021 at 10:47:38AM -0700, Jonathan Nieder wrote:

Perhaps. I think the context matters between "missing an object from the command line" and "missing an object I expected to find while traversing". And I'm not sure all spots which look up an object will know that context.

In some sense that's "just" a programming problem; surfacing the errors to the right spot that can decide how to exit. But I worry a bit that it's fighting uphill against the current code structure. There's probably going to be a period where MISSING_OBJECT versus UNKNOWN is wildly inaccurate, and a long tail of cases to fix.

Erring to say "UNKNOWN" is probably OK for most callers (they are happy to learn of a specific error and act on it appropriately, but if Git can't tell it to them, they have a generic path). But erring in the other direction might be bad (you fail to realize a repo is corrupt, and instead attribute it to caller error).

So again, I return "I dunno". Something of this magnitude probably has to be done incrementally and over time. But I'd be loathe to trust it and convert existing callers use it for a while. And that creates a chicken-and-egg problem for finding the places which need improvement.

-Peff

On 5/19/21 7:34 PM, Jonathan Nieder wrote:

...

For the error event that `error()` and `die()` and friends generate, I emit both the fully formatted error message and the format string.

The latter, if used as a dictionary key, would let you group like events from different processes without worrying about the filename or blob id or remote name or etc. in any one particular instance.

Would that be sufficient as an error classification and something that you can key off of in your post-processing ?

Granted the same format message might be used in multiple places in the source, but I also provide the source filename and line number.

If it turns out that all of the error events come from "usage.c" (i.e. error_builtin() or die_builtin()), then maybe we need to look at another way of wrapping those calls to pass the F/L of actual caller. I hesitated to do that because of the existing indirection tricks in usage.c WRT the `set_error_routine()` and friends. (And that assumes that the format string is a viable solution for you problem.)

Jeff

On 2021-05-20 at 15:09:44, Jeff Hostetler wrote:

I don't think that's going to be sufficient. Many calls to die() contain a translated string. I am a native Anglophone and work in a company where English is the sole language of communication, but I have my computer configured in French, and I have had it configured in Spanish as well.

It's totally possible that one of my colleagues who has a non-English native language might be using a different language as well, so it would be difficult to reliably map the format string into a fixed error case in a typical corporate setting, since many languages might be in use.

The source filename and line number would be more helpful, but inconveniently, people frequently change the code of Git, so the line numbers aren't always stable over time. So I think an error code would be helpful nevertheless.

On 2021-05-19 at 23:34:24, Jonathan Nieder wrote:

In general, I'm in favor of adding some sort of error code here. Even though I don't normally use trace2, I think there's a lot of benefit to having a standardized set of error codes, and this seems like as good a place as any to introduce them.

A future iteration of this might look like us returning a negative error code from a function instead of -1 for us to signal to the caller that a particular error case occurred. We need not implement that now, of course, but I bring it up in case we want to accommodate that in our design now for future us.

I do agree with Peff that this may not necessarily provide all of the insight you want, since it can be hard to distinguish why the error occurred. For example, in Git LFS, we sometimes will pass objects we don't have to git rev-list (with --missing) and that's completely expected, whereas a missing object with git fsck would generally be cause for alarm. Provided you're comfortable with some ambiguity, I think this would be a nice improvement.

On Wed, May 19 2021, Jonathan Nieder wrote:

Having read the thread at large (and some of this is a more general response) a few points, not against or as a retort to this, just related thoughts, complimentary suggestions etc:

 1. As shown in my f6d25d78789 (api docs: document that BUG() emits a
    trace2 error event, 2021-04-13) all of BUG/die/error/warning just
    emit "error" under trace2.

    It seems to me a good place to start with this effort would be for
    someone to split that up. It requires changing the trace2 schema,
    but it can be done in some backwards compatible way. Perhaps event:
    error, error_type: [bug,die,error,warning] ?

 1.5. Split up error_errno() from error() for trace2 purposes? This gets
      you partway to your "d".

 2. Similarly we need to log the correct line numbers for
    die/error/warning. They need to be a macro/function like BUG() /
    BUG_fl().

 3. You can then key error events/frequencies on the "fmt".

 4. To the extent tha #3 isn't true on client machines due to i18n we
    could change the API in a backwards-compatible way from
    e.g. error(_("string") to error(_N("string")). We'd then always
    transmit the C locale "fmt".

Basically I wonder if a more granular approach with just better logging of information we have now (but lose in trace2) + maybe some split-up of the current functions, e.g. having a user_error() distinct from repository_error() or whatever wouldn't get us most/all of the way to this.

Maybe, but that seems highly problem-dependant, and not e.g. something where we'd like to just do a blind retry in one of our own porcelain tools if a plumbing one failed with a "had an issue, retries might work" code.