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@xxxxxxxxx>
[...]
+ /*
+ * 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
