Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)

[roger peppe]

On 27 May 2014 04:02, Tim Penhey <tim.penhey at canonical.com> wrote:
> On 23/05/14 02:47, roger peppe wrote:
>> Any particular reason to wrap the errgo functions rather than using errgo
>> directly? Personally, I see the role of the errors package to be about
>> specific error classifications (something that errgo tries hard to be entirely
>> agnostic about), but errgo could, and I believe should, be used directly for all
>> the more general calls.
>
> Sure.  There were several reasons that kept me away from using errgo
> directly or modifying errgo.
>
> 1) Probably the most important, I don't feel that errgo is a 'juju'
> library but very much a personal library of yours, one that you have
> very strong feelings about, and I feel that proposing changes that
> fundamentally change how it behaves will get bogged down in
> bike-shedding and disagreement.  I was wanting to get the behaviour that
> we wanted, and I found a way to get that behaviour without touching
> errgo, so I went that way.

errgo is now under github.com/juju precisely because I wanted to bring it into
the juju fold - it's there *for* juju and it should do what juju needs.

It's true that I have definite preferences for how some of the behaviour
works, because I have thought a lot about the emergent effects of
the way we handle and generate errors in juju, and I believe that
a using errgo's approach consistently will lead to code that
is easier to maintain and understand. See
http://rogpeppe.wordpress.com/2014/04/23/some-suggested-rules-for-generating-good-errors-in-go/
for some background on that.

I understand that you don't agree entirely with the approach,
but there *was* a long discussion in which several of us ended up agreeing
a) to use errgo
b) on the naming scheme that it currently uses.

Rather than unilaterally deciding to do things differently because
you don't agree with those decisions, wouldn't it be better
to seek some agreement first, as I did?

> 2) The default behaviour of errgo to mask the underlying cause was not
> the behaviour that juju wants.  For example the Notef function returns
> an error where the Cause of that error is different to the Cause of the
> error that is being annotated.  I know that there is NoteMask, but that
> isn't what I think of when all I want to do is add a note.

When you're writing the code, you know what kinds of
errors you want to pass back ("we just want to annotate, and leave the
NotFound error as is"), but when reading the code later, that
is obscure. I still believe that it's better to *explicitly* annotate the
code with that knowledge at each level.

In the vast majority of cases (I know this empirically) we do *not*
want or need to pass back the error cause. Look at all the current
uses of fmt.Errorf(...., err) - none of them passes back the error cause,
and that's a Good Thing. We have had subtle bugs because
error causes for two very different things were conflated.

I think it's a reasonable principle that the behaviour we want most often should
be the default.

> 3) I found the functions in errgo to be confusing when what I wanted was
> three simple things: add stack information, annotate the error (and add
> stack info), or change the cause (and add stack info).  There is no way
> to just change the cause with errgo without providing some annotation,
> and I think that if you are a user of the library and wanting to provide
> a more detailed error to describe the problem, then that error is likely
> to contain all the information that you want to see, and as such,
> forcing them to also add a message is not needed.

You don't *need* to add a message - just pass the empty string
to WithCausef and no message will be added. It should
probably be documented that it's OK to do that - or if this
functionality is used a lot, a WithCause function would work
just fine. FWIW, when I converted the whole of juju to use
errgo, I didn't use that function once. If you're changing the
cause, you probably want some annotation to reflect that
though (but see below).

> 4) The string representation of the error was not what we wanted with
> juju.  The errgo implementation walks up the entire error stack
> outputting annotations up to the top, and then the underlying error.
> The behaviour we wanted was to walk up the annotations and stop when the
> cause of the error changes and write out the cause.  That way actually
> changing the cause of the error as it is passed down but keeping the
> entire call stack gives a string representation of the actual (most
> recent) cause of the error, not the original, and yet the entire call
> stack could be output.

I have definitely struggled to find a good behaviour here for errgo.
The problem is that Cause is orthogonal to Underlying, and
you really want to be able to see *both* in the error string output,
because both are important.

The reason I chose the solution that I did was that the "underlying error"
is really the thing that is designed for the user to see, so we show that
to the user and we can try to ensure that it will appear meaningful when shown;
the "cause" is the thing that's designed for the program to analyse, so
we only show that when debugging (and it can be shown by the Details
function - there's definitely an argument to add an argument to enable
the cause to be shown there, rather than having it turned on by
the debug constant).

I would like to see some concrete examples of when one case
or the other would be useful - this is something that can be
easily adjusted in the light of experience.

By the way, the code that currently implements the "cause has changed"
logic inside juju/error is a) bad because it imports unsafe, which
we should never do unless strictly necessary, and this is the
only place we do so inside the whole of juju except for one unavoidable
windows system call and b) broken.

> Getting the stack trace information accessible to debugging and logging
> was my first priority. The errgo discussion started almost a year ago
> and we still hadn't gotten things merged and it was my understanding
> that a key reason for that was a difference of opinion around the 'keep
> the cause the same' vs. 'the cause of the error is different' default
> behaviour.  I was not interested in getting into yet more discussions
> and time delay around getting the functionality we want into the juju
> libraries.

I have had it ready to merge several times - the last couple of times
it did not was because of very small issues. As far as I am aware
there was no remaining difference of opinion - we had agreed on
the approach.

> I was wanting to move the error package out of juju-core any way so it
> could be used by other projects (more easily), and it just seemed timely
> to add the, in my opinion, simpler interface to that package.

I think that moving the error package out of juju-core is a good idea.
As outlined above, I don't think reimplementing the arrar primitives
on top of errgo (awkwardly) is a good idea.

  cheers,
    rog.