Re: RFC: error codes on exit

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 26, 2021, 08:21 UTC
Message-ID: <87sg29n9mt.fsf@evledraar.gmail.com>
In-Reply-To: <YKWggLGDhTOY+lcy@google.com>

On Wed, May 19 2021, Jonathan Nieder wrote:

Show 32 quoted lines

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

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.

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

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.