proposal: change for Go beginners (stack trace)

[pierrre]

What I understand

• WithMessage() adds a message
• WithStack() adds the current stack
• Wrap[f]() adds a message + stack
• New|Errorf() creates an new error with message + stack

History

If I remember correctly:

• Wrap[f]() was added first
• WithMessage() + WithStack() were added later, because Wrap[f]() was "too much" => we don't need a stack trace for all "wrapped error". It is only useful for the "deepest" error.

When should we use each function

• New root error => New|Errorf() (it will contain a stack trace)
• Error returned by the std lib or a 3rd party lib => Wrap[f]() (we want to add a stack trace)
• Error returned by our own code (in my app) => WithMessage() (we don't want to add more stack trace)
• Error returned by a different goroutine => WithStack() (I don't use it very often)

Who am I working with

• ~10 Go developers
• All beginners in Go (they were working with other languages previously)
• Not always very strict/careful

The problem

• They don't/can't understand where to use Wrap[f]() vs WithMessage()
• We're losing a lot of time during code review
  • Checking if Wrap[f]() or WithMessage() should be called
  • Explaining in which case we must use which function
  • Starting again for each newcomer

The risks

• If we call WithMessage() instead of Wrap[f]() => we're missing the stack trace
• If we call Wrap[f]() instead of WithMessage() => we "pollute" our error logs

What we need

A function that wraps an error with a message, and optionnaly with a stack trace if it doesn't have already one.

What I tried

My initial idea was to write a custom helper function that would do as described previously.
Sadly, this is not easily doable.
I can't call WithStack() or Wrap[f]() because it would include the helper function call in the stack.

The solutions

New function

Add a new function to github.com/pkg/errors, that is exactly like Wrap[f](), but doesn't add a stack trace if there is already one.

I don't really like this one, because it makes the package's interface more complex

Change behavior

Change Wrap[f](), and don't add a stack trace if there is already one.

This is my favorite solution.

I agree, it will change the existing behavior.
However, I think this is worth it.
I can't think of any use case that requires to call Wrap[f]() several times and have several redundant stack traces.

Just fork

I can perfectly understand if you disagree, and don't want to update the current code.
In this case, I will just copy the code to our internal repo, and adapt the code to match our needs.

I don't really like this, because I would stop using github.com/pkg/errors.
We're currently using it everywhere in our code.
I've been able to "extend" the behavior properly since the beginning.

The implementation

I've seen #122 and I really dislike the implementation.
It means that only internal github.com/pkg/errors types can be used.
We can't create our own types anymore.

What I would do instead:

• iterates over all causes
• check if at least 1 error implements StackTrace()

[davecheney]

Thank you for your detailed issue. I agree that #122 is problematic because I don't know how to express the business logic of "add a stack trace, unless there is one" in a comprehensive way and I'm not keen in trying for an 80% solution.

Re your proposed solutions

• New function. Probably not, we already have too many here, and given I cannot remove any at this point, I'm very wary of adding more.
• Change behaviour. Possibly, but I don't know how to do it.
• Fork. You should probably do this if the errors package doesn't meet your needs. As long as your error types implement Cause they will interoperate fine.

Your proposed solution sounds interesting; given that none of the tests passed with #122 (i haven't checked why, it may be unrelated), the field to attempt to implement this proposal is still open.

[pierrre]

Hello,

For the technical implementation, my idea was:

• iterates over all causes
• check if at least 1 error implements StackTrace()

So, I suggest to create a function:

func hasStackTrace(err error) bool {
	type stackTracer interface {
		StackTrace() errors.StackTrace
	}
	type causer interface {
		Cause() error
	}
	for err != nil {
		if _, ok := err.(stackTracer); ok {
			return true
		}
		if errc, ok := err.(causer); ok {
			err = errc.Cause()
		} else {
			return false
		}
	}
	return false
}

Then call this function in Wrap[f](), and don't add

	&withStack{
		err,
		callers(),
	}

if there is a stack trace.

I can submit a PR if you agree.

About my motivation, I repeat:

• I don't do it for myself, I'm personally fine with the current behavior (I know when I must call Wrap[f]() or WithMessage()
• I'm thinking about Go beginners that don't really understand the difference between Wrap[f]() and WithMessage()
• This problem has real consequences:
  • missing stack trace
  • time lost during code review
  • more complex for beginners

Fork. You should probably do this if the errors package doesn't meet your needs.

I would really prefer to not fork github.com/pkg/errors.
I really like this package and I want to continue to use it.
Also, if I can make it easier to use for beginners, I'm happy to contribute :)

[davecheney]

If I had to do this package again I would not include wrap, it was a mistake before I understood the problem well enough. However as I have said elsewhere, I cannot remove anything from this package at this point. My advice to beginners would be to use only errors.New or errors.Errorf until they can describe precisely the limitation of that advice.

…

On 30 Oct 2017, at 21:06, Pierre Durand ***@***.***> wrote: Hello, For the technical implementation, my idea was: iterates over all causes check if at least 1 error implements StackTrace() So, I suggest to create a function: func hasStackTrace(err error) bool { type stackTracer interface { StackTrace() errors.StackTrace } type causer interface { Cause() error } for err != nil { if _, ok := err.(stackTracer); ok { return true } if errc, ok := err.(causer); ok { err = errc.Cause() } else { return false } } return false } Then call this function in Wrap[f](), and don't add &withStack{ err, callers(), } if there is a stack trace. I can submit a PR if you agree. About my motivation, I repeat: I don't do it for myself, I'm personally fine with the current behavior (I know when I must call Wrap[f]() or WithMessage() I'm thinking about Go beginners that don't really understand the difference between Wrap[f]() and WithMessage() This problem has real consequences: missing stack trace time lost during code review more complex for beginners Fork. You should probably do this if the errors package doesn't meet your needs. I would really prefer to not fork github.com/pkg/errors. I really like this package and I want to continue to use it. Also, if I can make it easier to use for beginners, I'm happy to contribute :) — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[pierrre]

If I had to do this package again I would not include wrap, it was a mistake before I understood the problem well enough.

Do you mean that Wrap[f]() shouldn't exist ?
And you want to remove it, but you can't ?

So, my proposal to update Wrap[f]() is obsolete, right ?

[davecheney]

On 31 Oct 2017, at 21:30, Pierre Durand ***@***.***> wrote: If I had to do this package again I would not include wrap, it was a mistake before I understood the problem well enough. Do you mean that Wrap[f]() shouldn't exist ?

Basically, yes.

And you want to remove it, but you can't ?

Correct.

So, my proposal to update Wrap[f]() is obsolete, right?

I’d rather not change anything about wrap, although this is all caught up in the meta issue of having one and only one stack trace attached to an issue.

…

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[pierrre]

New function. Probably not, we already have too many here

I’d rather not change anything about wrap

So, if I understand correctly, you'd rather not add new function or change existing ones.
No problem, I understand your point of view :)

I've got a different (but related) question.
Currently if we want to get an error's stack trace we need to "assert" with the interface:

type stackTracer interface {
        StackTrace() errors.StackTrace
}

Do you think it's a good idea if we add a method StackFrames() *runtime.Frames ?
If I remember correctly, this is important for "mid stack inlining" (but I'm not 100% sure).
I plan to add a similar method in my own/internal "errors" implementation.

[davecheney]

On 2 Nov 2017, at 08:25, Pierre Durand ***@***.***> wrote: New function. Probably not, we already have too many here I’d rather not change anything about wrap So, if I understand correctly, you'd rather not add new function or change existing ones. No problem, I understand your point of view :)

Changing any public symbol is out of the question. Adding any new symbols is something that I want to proceed very carefully on because it’s hard to undo. I have on my backlog to do a survey in support of the issue (I don’t have he exact details to hand) about adding a ‘f variant to WithMessage (I may be mistaken). This will hopefully inform any decisions to add new features.

I've got a different (but related) question. Currently if we want to get an error's stack trace we need to "assert" with the interface: type stackTracer interface { StackTrace() errors.StackTrace } Do you think it's a good idea if we add a method StackFrames() *runtime.Frames ?

Extending an interface is not possible because it will break backward compat. I think there is a proposal to switch to runtime frames to support mid stack in lining which will be becoming more urgent as 1.10 ships. Again, sorry I don’t have the details to hand (phone)

…

If I remember correctly, this is important for "mid stack inlining" (but I'm not 100% sure). I plan to add a similar method in my own/internal "errors" implementation. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[pierrre]

Extending an interface is not possible because it will break backward compat.

Sorry, I expressed myself badly.
I meant: add a new method StackFrames() *runtime.Frames (probably on the stack type).

It will not change the existing method/interface StackTrace() errors.StackTrace.
The goal is to allow us to get the runtime.Frames from an error.