proposal: errors: let GODEBUG=errstacktrace request stack backtraces

[ianlancetaylor]

Errors in Go serve two main purposes: reporting problems to the user of the program, and reporting problems to the developer of the program. The user essentially never wants to see stack traces. For a developer, they can be helpful. Thus it is reasonable for developers to use packages like https://pkg.go.dev/github.com/go-errors/errors to collect developer errors--errors that the user is not expected to see.

That said, while developing a program, there are times when it is helpful for a user error to include a stack trace. For example, this is useful when some program bug is causing an unexpected user error. Getting a stack trace may help speed up debugging. This can also be useful when a user reports an inexplicable user error that is repeatable with proprietary user data; here again a stack trace may help the developer debug the problem.

Therefore, I propose a new GODEBUG setting: GODEBUG=errstacktrace=1. When this GODEBUG entry is set in the environment, the errors.New and fmt.Errorf functions will change to incorporate a stack trace into the message. This stack trace will be part of the string returned by the Error method (which will now be a lengthy, multi-line, string).

This will permit developers debugging an obscure but repeatable problem to set an environment variable to request a stack trace for the error that they are seeing.

We could also consider providing a simple mechanism for other error generation packages to check whether this GODEBUG setting is turned on.

This was inspired by a comment by @fsaintjacques : #60873 (comment).

[apparentlymart]

The general idea of this resonates with me, but it feels strange to me that this would be a runtime-changeable setting rather than a compile-time-changeable setting. In my application it would mean that setting this environment variable would expose the detail of which end-user-facing error messages are being derived from error.Error() results and which are hand-written specifically for our application.

I tend to bristle a bit each time the Go runtime gains another way to globally change some runtime/library behavior out from under my application, since that adds yet another dimension of variation to consider when someone reports some unexpected behavior, but I suppose I am more sensitive to that than most Go application developers given that I use Go to develop CLI tools for which it should be irrelevant to the user what language they are written in. Probably it's more common to use Go to write server processes that are designed to run only in a particular execution environment and where everybody knows well that the program is written in Go.

This is honestly a more broad piece of feedback than just on this proposal, and so I should probably record it somewhere else 🤔 but I would enjoy some way to compile an application such that it does not respond to GODEBUG feature-switching at all, but have a way to explicitly turn on some of these features programmatically based on mechanisms that make more sense for my application, such as the application's configuration file. Then I would expose only the ones I know about and can see a reason for a user to turn on, and exclude any that have questionable value to my application.

---

Since I've already contributed a large number of words to this issue across two comments I'm going to extend this comment to include a response to Ian's later comment about it being an advantage to extend the error string, even though that makes this response appear out of order, just because it's an extension of my existing point above.

I consider it to be a disadvantage that it would affect the behavior of existing programs that have not been modified to request or expect stack traces, since some error messages are written for users and turning this on would significantly change those error messages.

Even though it's opt-in for a user, there are some applications (such as my own) where the user is not the person that ought to be making this decision, or if it is the user making the decision then it would be better to make it in a way that is more natural for the application.

[fsaintjacques]

Note that the real reason I want this is to report errors to some error reporting services like sentry. Thus the stacktrace must be retrievable via some programatic way, either a public function in errors, or via unwraping to an exported type, i.e. not string parsing.

This type of functionality is common for long running services were we want to report exceptions / unexpected errors. As a single data point, here's how common it is in our monorepo...

 $ ag -i '(fmt.Errorf|errors.New|errors.Wrap)' pkg | wc -l
10573

Once we encounter an unexpected error, we have both grpc/http middlewares auto-magically extracting the stack trace and submitting it to sentry. As a success criteria of this change, the previous line count should be drastically reduced. In other words, most callsites should be changed from

if err := myFn(...); err != nil {
  return errors.Wrap("myFun: %w", err)
}

To simply

if err := myFn(...); err != nil {
  return err
}

[apparentlymart]

The above comment from @fsaintjacques makes me consider a different design, which also happens to be a solution to my earlier feedback. 😄

Building on the design approach for errors.Is and errors.As with special optional methods that error implementations could implement, add errors.StackTrace(err error) *Stacktrace which:

1. Tests whether err has a method ErrorStackTrace() *Stacktrace. If not, immediately returns nil.
2. If so, return err.ErrorStackTrace()

The default situation is that no errors have backtraces.

When GODEBUG=errstacktrace is enabled, the errors.New and fmt.Errorf functions include a stack trace in the generated error value. The types returned by both implement the ErrorStackTrace method as returning the saved stack trace, if present. Whether they do that by always returning a type containing a *Stacktrace field that will often be nil or by returning a different type depending on whether the stack trace functionality is enabled is an implementation detail up to the implementer, as far as I can tell.

The Error() function for each of these types does not change at all: it just returns the specified message, as before.

If nothing in the program ever passes an error to errors.StackTrace then the stack trace collection is just some wasted time collecting a stack trace nobody will refer to. But a program that does want to capture a stack trace, for example to send it to Sentry as described above, can try to pass the error to errors.StackTrace and make use of the result whenever it's non-nil.

---

In the above I've assumed there's a material cost to capturing a stack trace and thus it would be too expensive to enable by default, but I've not actually measured that. If capturing a stack trace is relatively cheap then it would be nicer to not require the extra environment variable at runtime, but I don't know exactly how the Go runtime tracks stack traces internally.

[mitar]

As an author of the gitlab.com/tozd/go/errors stack trace attaching errors package, I think having an opt-in stack trace recording of standard errors would be great. But the current proposal has some issues:

I do not think it is reasonable or even workable to attach stack trace to Error string:

• This makes it behave very differently when stack trace is attached or not. For example, how does fmt.Errorf("%w: extra", err) behaves, when err already contains a stack trace? It attaches extra to the end of the stack trace while without GODEBUG it does it to the end of the Error line.
• Attaching a stack trace AND formatting it is expensive. Adding it blindly to all errors might introduce a lot of unnecessary work of string manipulation.

I think instead:

• Standard errors should implement StackTrace() []uintptr interface which return nil without GODEBUG flag and what callers() provide when GODEBUG is set.
  • Alternatively, standard errors could implement this interface only when GODEBUG is set.
• This records a minimum of the stack trace and this is it. Then you can do with this stack trace information whatever you want. Also, the return type is simply []uintptr to not have another type defined.
• Formatting of standard errors can then include a stack trace after the Error if formatting is done with %+v, which is kinda standard for this these days.

How exactly is recording of stack traces enabled, using GODEBUG or some other mechanism I will leave to others to discuss. But I feel strongly that my proposal above nicely combines with existing approaches while not breaking anything.

[ianlancetaylor]

The advantage of adding the stack trace to the Error string is that it works in a program that has not already been designed to work with error stack traces. If you have to change the code to print or otherwise handle the stack trace, then it seems to me that you may as well use an existing package that manages stack traces for you.

That said, I agree that it makes sense to add a way to retrieve just the stack trace from the error, if there is one.

I also agree that we should arrange that using %v or %w in a fmt.Errorf doesn't blindly copy the stack trace string. We should fetch the error string without the stack trace, format it, and then add in the stack trace. Right now I think it should just preserve the original stack trace, and not add a new one. Perhaps some more clever option is available.

[fsaintjacques]

I also agree with @mitar that the ideal signature is []uintptr, that's what we do internally.

[fsaintjacques]

In the above I've assumed there's a material cost to capturing a stack trace and thus it would be too expensive to enable by default, but I've not actually measured that. If capturing a stack trace is relatively cheap then it would be nicer to not require the extra environment variable at runtime, but I don't know exactly how the Go runtime tracks stack traces internally.

I would expect that this cost is paid in (hopefully) exceptional case, very rarely will you see an error frequently returned in a hot loop.

[mitar]

@ianlancetaylor OK, what about then: we do not change Error string at all, we only unconditionally add stack trace when the error is formatted when the flag is enabled (with whichever verb)? That might be the right trade-off because otherwise I can expect some tests might be failing if they call err.Error() and expect it to be in some structure (yes, bad programming, you should use errors.Is, etc., but still). But if you are doing fmt.Printf("error: %s", err) it is probably safer to include a stack trace?

[neild]

This proposal is to add stack traces to errors.

The Go 1.13 errors proposal was to capture a single stack frame, and allow error wrapping to identify the trace of locations producing the error. This approach reduces the size of error values and can represent a trace that passes across goroutine boundaries, but produces less information in the simplest case.

I think any proposal to add stack information to errors needs to consider full stacks vs. frames, and present an argument for which approach is used.

[ianlancetaylor]

@mitar Let me restate your suggestion. We keep the Error method as is, but change the fmt package (other than fmt.Errorf) so that in any case where we call the Error method, we also check for and call a Stacktrace method, and, if that method is not empty we call it and also print the stack trace. We change fmt.Errorf so that if it wraps an error it does something intelligent with any stack trace. Does that sound right?

@neild This proposal suggests an optional feature that we expect to only be enabled while debugging. As such, I am thinking it should capture a complete stack trace.