Jonathan Nieder [off-list ref] writes:
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
...
e. we encountered an internal error in handling the user's
legitimate request
and these different cases do not all motivate the same response.
...
In order to do this, I would like to annotate "exit" events with a
classification of the error.
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:
Jonathan Nieder [off-list ref] writes:
quoted
In order to do this, I would like to annotate "exit" events with a
classification of the error.
We already have BUG() for e. and die() for everything else, and
"everything else" may be overly broad for your purpose.
BUG() and die() can call fatal().
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.
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).
--
Felipe Contreras
Hi,
Junio C Hamano wrote:
Jonathan Nieder [off-list ref] writes:
quoted
In order to do this, I would like to annotate "exit" events with a
classification of the error.
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.
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.
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.
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/