Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: Tracing errors with the tracer interface. #23271

Closed
jaekwon opened this issue Dec 28, 2017 · 17 comments
Closed

proposal: Tracing errors with the tracer interface. #23271

jaekwon opened this issue Dec 28, 2017 · 17 comments
Labels
FrozenDueToAge Proposal v2 An incompatible library change
Milestone

Comments

@jaekwon
Copy link

jaekwon commented Dec 28, 2017

The error system is great, but sometimes it's hard to figure out where an error came from. Here's a proposal that could more or less be implemented today with a Golang source code transpiler, but would benefit from official Golang support. This is a proposal for Go1. (For Go2 I would rather propose that error be a tracer itself)

The proposal is to support the following:

  • a global interface like type terror interface {} (name can change)
  • in "pkg/errors", func Terror(e error) terror {...} as implemented below (name can change)
  • in "pkg/errors", keep the same signature in errors.New() error {...} but have it return a terror that can be run-time converted.
// New global type.
// Like `error`, a global interface.
// Trace calls `runtime.Caller` to get the file and line number.
type tracer interface {
    Trace()
}

// New global type.
// A new type of error that traces.
type terror interface {
    error
    tracer
}

// In pkg/errors.
func Terror(e error) terror {
  if t, ok := e.(terror); ok {
    return t
  }
  return newTerror(e)
}

// In pkg/errors.
// Same method signature, but actually returns a terror.
func New(...) error {..}

// Usage:
var e error = errors.New("bark")
var t1 terror := e // compile error
var t2 terror := e.(terror) // ok
var t3 terror := errors.Terror(e) // ok

Everything else can be handled by third-party libraries and code transpilers. What follows is just one example of syntax support from a transpiler with support for an assignment modifier !.

func foo() terror {
  var x terror = errors.New("bark").(terror)
  var x! terror = x // trace!
  var y error = x
  var z! error = x // compile error
  return x
}

x := foo()
x! = x // trace!
x! = <-ch // trace!

var y, z *terror
y = z
y! = z // trace! (but is this what we want?)
*y = x
*y! = x // trace!

type MyStruct struct {
  t terror
}
s := MyStruct{x}
s = MyStruct{field!:x} // trace!
s2 := s
s2! := s // compile error s.(tracer) is not ok

var i interface{}
i = x
i2 := i.(terror)
i3! := i.(terror) // trace!

func bar(t! terror) {...}
bar(x) // trace @ bar()!

The pain-point with error is that it doesn't trace. This seems like a way to inject tracing with minimal syntactic change and good performance.

The transpiler could also work without explicit !, but rather automatically every time something is assigned to a var _ terror.

@gopherbot gopherbot added this to the Proposal milestone Dec 28, 2017
@mvdan mvdan added the v2 An incompatible library change label Dec 28, 2017
@jaekwon
Copy link
Author

jaekwon commented Dec 28, 2017

Alternatively, the ! could be implicit, but given Golang's no-hidden-computational-complexity rule, I figured something had to be introduced at the grammar level.

To help detect tracing "bugs" earlier on, the compiler could enforce that if a trace assignment happens to some var, that all assignments to that var must also trace assign.

@jaekwon jaekwon changed the title proposal: Tracing errors with terror proposal: Tracing errors with the tracer interface. Dec 28, 2017
@pciet
Copy link
Contributor

pciet commented Dec 28, 2017

The only time I’ve needed to trace to source in my applications is with panic calls that have the stack trace. Any time I’ve had to look up errors in libraries (such as github.com/lib/pq) I’ve been able to find the source of the error.

If an application has multiple sources of an error and can’t panic why not include the trace in the error type already available with Go 1?

type AppError struct {
    Trace []string
    Err   string
}

func (the AppError) Error() string {
    return fmt.Sprintf(“%v: %v”, the.Err, the.Trace)
}

func DoWorks() error {
    err := Work1()
    if err != nil {
        out := err.(AppError)
        out.Trace = append(out.Trace, “via Work1”)
        return error(out)
    }
    err = Work2()
    if err != nil {
        out := err.(AppError)
        out.Trace = append(out.Trace, “via Work2”)
        return error(out)
    }
}

// the application authors know this must be an AppError returned
func Work1() error {
...

@cznic
Copy link
Contributor

cznic commented Dec 28, 2017

The effects of proposed mechanism can be achieved with no language changes, see eg: https://github.com/pkg/errors

@puellanivis
Copy link

“terror” and “Terror” are a very poor choice of name, due to a collision with another English word. “t_error” also being a poor choice, because it obscures the elements constructing it. Go does not use io.RWCloser, it uses io.ReadWriteCloser to make it clear all the functions that are a part of the interface. (Yes, Go has sync.RWLock, but this is a concrete type, not an interface.)

So, first thing before any other criticism, it would need to be called “TraceError” or “traceerror”…

Second, an perhaps far more importantly, I do not think I am alone in absolutely detesting the hidden mechanism of tracing, let alone the necessity of adding syntax to allow for the trace.

If one needs to trace errors, then they should implement a Tracer interface, and explicitly call the Trace function themselves.

@jaekwon
Copy link
Author

jaekwon commented Dec 28, 2017

The only time I’ve needed to trace to source in my applications is with panic calls that have the stack trace. Any time I’ve had to look up errors in libraries (such as github.com/lib/pq) I’ve been able to find the source of the error.

I think plenty of people are having issues with this, and it's the reason why things like github.com/pkg/errors exist in the first place. I've had issues certainly, and so has most people who use github.com/pkg/errors. Panics are panics, shouldn't be conflated with errors for this proposal, because they have different usage. I want errors with fast tracing, whereas I wouldn't mind if panics were otherwise extremely inefficient to recover from. Panics should be reserved for things you can't recover from, not used in control flow.

out.Trace = append(out.Trace, “via Work1”)

That example doesn't include filename, funcname, & lineno. I want more or less a stack trace, and I don't want to have to figure out what string to put in there. The point is, this should be easy.

If an application has multiple sources of an error and can’t panic why not include the trace in the error type already available with Go 1?

That's what I'm suggesting, more or less, but with a bit of modification in Golang's stdlib and type system to make it all easier:

  • a global interface like type terror interface {}, though it doesn't need to be called terror.
  • in stdlib pkg/errors, func Terror(e error) terror {...} as implemented. Again, the name can change.
  • in stdlib pkg/errors, keep the same signature in errors.New() error {...} but have it return a terror type.

it would need to be called “TraceError” or “traceerror”…

+1 if that's what we go with, ok. But terror is kinda cute and it saves keystrokes. Also, error is already an exception in that it's globally accessible. We could import "errors" and just use errors.Error but we don't, for convenience. It also makes Golang development more bad-ass.

Second, an perhaps far more importantly, I do not think I am alone in absolutely detesting the hidden mechanism of tracing, let alone the necessity of adding syntax to allow for the trace.

Ha! I added the ! symbol just for you, but you still detest it, eh? That's fine, that can be done outside of the langauge, so you can manually call Trace() in your code. I'll auto-generate those calls because my wrists hurt.

If one needs to trace errors, then they should implement a Tracer interface, and explicitly call the Trace function themselves.

My wrists, they hurt. Also, knowing programmers, they're not going to put trace marks in the right places unless it's easy. Frankly, I'd prefer the code transformer that inserts this trace automatically with no need to type !. But I understand the need to keep that outside of Golang.

@jaekwon
Copy link
Author

jaekwon commented Dec 28, 2017

I've updated the proposal to make it clear what is being proposed for Golang. Any syntax changes can be decided by third-party tools, but it's optional. The name can change from terror to traceerror or whatever the community decides on. This proposal could work for Go1. @pciet @puellanivis @cznic @bontibon @mvdan

@puellanivis
Copy link

Literally each of the first three discussions you link to are, “you are using github.com/pkg/errors wrongly.”

Your solution is in search of a problem.

“error” is an exceptional case of a builtin interface because it is used extensively, and there was no simpler way to achieve the desired simple behavior. Your proposal is already covered by github.com/pkg/errors.

“But some people use github.com/pkg/errors wrong.” Some people use Go lang itself wrong, and end up with infinite recursions from things as simple as func (s StringType) String() string { return fmt.Sprintf(s) }

A stacktrace at the generation of the error itself should be sufficient to isolate where the error was generated, and that is precisely what github.com/pkg/errors.New and github.com/pkg/errors.Errorf do.

@jaekwon
Copy link
Author

jaekwon commented Dec 30, 2017

@puellanivis @davecheney @rsc

Literally each of the first three discussions you link to are, “you are using github.com/pkg/errors wrongly.”

You're right, literally each link was tangential to my point. Oops!

A stacktrace at the generation of the error itself should be sufficient to isolate where the error was generated, and that is precisely what github.com/pkg/errors.New and github.com/pkg/errors.Errorf do.

That's unfortunately not true because most errors are not generated with github.com/pkg/errors. That's why errors.Wrap() exists in the first place. The very prominent and primary way of using github.com/pkg/errors is to use errors.Wrap(). The very first paragraph of that library says,

"The traditional error handling idiom in Go is roughly akin to .... which applied recursively up the call stack results in error reports without context or debugging information. The errors package allows programmers to add context to the failure path in their code in a way that does not destroy the original value of the error."

So even if you generate a stack trace at the source, callers of your function (which returns an error) will in general want to wrap it again to add more context, and the preferred method is to use errors.Wrap(), not errors.WithMessage().

Perhaps there could be a more intelligent automatic way to choose between errors.Wrap() or errors.WithMessage(), but what would that look like? The only obvious 100%-correct-general-solution given the state of github.com/pkg/errors today (without assuming anything about the implementation of externalFunction() error) is to always use errors.WithMessage() but include minimal trace information like the filename and line number.

So OK, if github.com/pkg/errors starts advertising errors.WithMessage() as the primary method of adding contextual information, and its implementation is changed to include a modicum of trace information (filename, lineno), (or if errors.Wrap() were changed to include only minimal trace information) then we're 89% of the way there. But there's still a problem.

In general, it breaks the intent of Golang to wrap an error with github.com/pkg/errors.Error, because it breaks the one way that we're supposed to deal with error control flow... that is, we're supposed to switch on err behavior (or err concrete type, though it's not preferred).

type FooError struct{}
func (_ FooError) Error() string { return "" }

type BarError error

func externalFunc() error { return nil }

func main() {
	err := externalFunc()
	switch err.(type) {
	case FooError:
		// handle FooError
	case BarError:
		// handle BarError
	}
	fmt.Println("Hello, playground")
}

And github.com/pkg/errors violates that. There exists errors.Cause(), but now you have to dig down to the first non-github.com/pkg/errors.Error-error before you can type-check like above. And you can't just take the root-most cause of an error unless you preclude other custom error types from having a cause.

The solution is to not wrap an error, but to add trace information to the same error object. Ultimately it is error itself that needs to be a tracer.

But you're still right in that this could all be done in user-land. We can always do more or less the following: if err, ok := err.(tracer); ok { err.Trace() }, so there's also no need for a native terror type, as we can continue to use error and just branch at runtime.

I just think it's worth fixing, because the state of error handling and tracing is broken as is, and I stand by that statement.

@puellanivis
Copy link

Precise details dealing with issues internal to the github.com/pkg/errors documentation has been posted only to that thread.

“I just think it's worth fixing, because the state of error handling and tracing is broken as is, and I stand by that statement.”

And the public interfaces of these libraries has been established and cannot really be altered. If the pkg/errors package could be rewritten, he would have done it different. This has been extensively documented by that author.

But throwing away all that the github.com/pkg/errors developers have discovered and learned in order to suggest a complete paradigm shift in the Golang language…

@jaekwon
Copy link
Author

jaekwon commented Dec 30, 2017

Precise details dealing with issues internal to the github.com/pkg/errors documentation has been posted only to that thread.

Thanks. Replied!

And the public interfaces of these libraries has been established and cannot really be altered. If the pkg/errors package could be rewritten, he would have done it different. This has been extensively documented by that author.

I wrote in that discussion how the public interface could be preserved while fixing the implementation. It's still suboptimal compared to native support for .Trace() on the global error interface. Any third-party userland library that tries to tack on stack-tracing as github.com/pkg/errors does will be clunky, as errors need to be wrapped and unwrapped, and other reasons I don't care to get into until you first get the broader point I'm trying to make.

But throwing away all that the github.com/pkg/errors developers have discovered and learned in order to suggest a complete paradigm shift in the Golang language…

Dare I say, all that learning has been distilled into this very issue! Ok, that's a cocky statement, but I haven't heard a good rebuttal yet. I'm not suggesting a paradigm shift. I'm suggesting making error a tracer, which alleviates the need for clunky third-party libraries, which only helps enforce the paradigm that Go1's error system wanted in the first place.

The standard library's error.New() is great. It doesn't capture a stack-trace, which is what we want for fast error handling. And, error control flow via switching on behavior (interfaces) is great. This is the Golang way and the desired error paradigm. This is currently not possible in Go1 with the current state of github.com/pkg/errors, and even if github.com/pkg/errors were to be updated as I suggested, it would still be better to support tracing errors natively.

@jaekwon
Copy link
Author

jaekwon commented Dec 30, 2017

@pciet

Any time I’ve had to look up errors in libraries (such as github.com/lib/pq) I’ve been able to find the source of the error.

Knowing the source of the error in the library doesn't tell you where the error was received in your own code. github.com/lib/pq is a bad example because every call to that library generally includes sufficient context in the form of SQL queries, which tells you where the problem is in your own application code. It's not the case in general.

If an application has multiple sources of an error and can’t panic why not include the trace in the error type already available with Go 1?

That's pretty good within a single root-level project, but it doesn't work for modular libraries (and it would make it difficult to refactor such application code to spawn off independent modular libraries).

Callers mid-way in the stack don't know how to append trace information to some particular struct, because they don't know that the error has a Trace []string without using reflection. So the best they can do in general is to wrap the err error with yet another wrapper, which has all the same problems already mentioned here about github.com/pkg/errors.

A better solution is to use interfaces, i.e. with terror, so any caller can add trace info.

@puellanivis
Copy link

It's still suboptimal compared to native support for .Trace() on the global error interface.

One cannot expand the global error interface without breaking all of every error concrete type.

@nhooyr
Copy link
Contributor

nhooyr commented Feb 3, 2018

@puellanivis could you link me to where the @davecheney has documented how he would like to have written pkg/errors?

@puellanivis
Copy link

@nhooyr I looked to find some at some point, but it’s spread out a lot through numerous proposals on the pkg/errors issues, and can be difficult to find any specific examples. It has however been more of a vague, (paraphrased) “your proposal is good, but a lot of people rely upon the API as currently designed… and we would cause great difficulty to lots of people in the event of a fundamental redesign of the API… no matter how noble it might be.”

So, much more like the Cancel <-chan struct{} in the net.Request struct…

@as
Copy link
Contributor

as commented Feb 5, 2018

The error system is great, but sometimes it's hard to figure out where an error came from. Here's a proposal that could more or less be implemented today with a Golang source code transpiler, but would benefit from official Golang support.

The piece missing is the justification for this proposal to be implemented. There's clearly been a discussion on implementation details and third-party packages. In the same vain, third party libraries are great too, but using their existence to demonstrate necessity is incomplete without a very compelling use-case.

@decibel
Copy link

decibel commented Feb 6, 2018

@as, the fundamental problem that made me adopt pkg/errors (despite it's warts) is that simply finding the original errors.New("some unique error message") call still tells me nothing at all about how the code ended up there. While I can (manually) add fmt.Errorf("unable to do whatever I was attempting: %s", err) to all my if err != nil` blocks, now I have a different error than I started with, so I can't test for it. Sure, I can create my own custom interface for every error I need to test for, but that's a significant amount of extra work, and it doesn't guarantee uniqueness since any other package you're using might have used the same interface definition to signify some other specific type of error.

I believe one of the fundamental issues here is that Go assumes all errors must be super-high performance. Certainly, hot pieces of code need that ability, but for many projects the amount of performance-critical code is dwarfed by all the ancillary stuff.

@ianlancetaylor
Copy link
Contributor

Thanks, but we definitely don't want to capture a trace for every call to errors.New. Errors serve two audiences. An error intended for the user should not have a trace. We should not have to pay for a trace just to create an error value.

If you want to make a different proposal about adding error traces as a predefined functionality like the predefined error type, then we can discuss that. But this proposal is not going to happen.

@golang golang locked and limited conversation to collaborators Apr 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal v2 An incompatible library change
Projects
None yet
Development

No branches or pull requests

10 participants