gravitational/trace

Package for error handling and error reporting

Trace

Package for error handling and error reporting

Read more here:

https://goteleport.com/blog/golang-error-handling/

Capture file, line and function

import (
     "github.com/gravitational/trace"
)

func someFunc() error {
   return trace.Wrap(err)
}


func main() {
  err := someFunc()
  fmt.Println(err.Error()) // prints file, line and function
}

trace's Issues

Configuring UDPHook struct variables

Can we make fields clientAddr and clientAddr exportable. If we want to make it configurable, how do we do it?

Add Github Action to run test suite on PRs

Pretty much as the title says, we can craft a relatively simple GH action here to run the test suite against this repository on PRs and commits.

Bonus points if it calculates test coverage as well 😉

Packaging report

Trace.Wrap(nil) should return nil

trace.Wrap and trace.WrapWithMessage mutate TraceError

Relevant snippets:

func Wrap(err error, args ...interface{}) Error {
	if err == nil {
		return nil
	}
	var trace Error
	if traceErr, ok := err.(Error); ok {
		trace = traceErr
	} else {
		trace = newTrace(err, 2)
	}
	if len(args) > 0 {
		trace = trace.AddUserMessage(args[0], args[1:]...)
	}
	return trace
}

func (e *TraceErr) AddUserMessage(formatArg interface{}, rest ...interface{}) *TraceErr {
	newMessage := fmt.Sprintf(fmt.Sprintf("%v", formatArg), rest...)
	e.Messages = append(e.Messages, newMessage)
	return e
}

func (e *TraceErr) Error() string {
	return e.UserMessage()
}

In trace.Wrap we can see that if err is already a Trace error, we don't create a new error: instead we reuse the error passed in. If we also pass args to trace.Wrap, then we mutate *TraceErr.Messages. This can lead to very subtle data races where one thread can be reading from *TraceErr.Messages while another is writing to it. It's not obvious that trace.Wrap can mutate the error here. (trace.WrapWithMessage does the same thing).

I discovered this when the race detector detected a race in this code during a test case I wrote:
https://github.com/gravitational/teleport/blob/775bf09989f51962822407a3ba28603754b805c9/lib/reversetunnel/transport.go#L90-L100

	addr, err := t.Resolver(ctx)
	if err != nil {
		if strings.Contains(err.Error(), "certificate is not trusted") {
			err = trace.Wrap(
				err,
				"Your proxy certificate is not trusted or expired. Please update the certificate or follow this guide for self-signed certs: https://goteleport.com/docs/setup/admin/self-signed-certs",
			)
		}
		t.Log.Errorf("Failed to resolve tunnel address: %v", err)
		return nil, trace.Wrap(err)
	}

Just by calling err.Error() and trace.Wrap() we got a data race; that's pretty subtle! The reason is that t.Resolver(ctx) is a caching resolver and returns a pointer to the same error in separate goroutines. The caching resolver needs a change as well, but the fact that trace.Wrap mutates the error is a footgun in and of itself. I think we should change trace.Wrap to always create a new error rather than mutating the input.

Support `errors.Is`/`errors.As` with Aggregate errors

I've come across a scenario where in Teleport we attempt to connect to multiple ports to determine where the Proxy is hosted, and, as it stands we return the last error that is encountered if none succeed. This is quite sub-optimal, as it often means that a more useful error (e.g the certificate being self signed) is lost and a less useful error (connection refused) is propagated up to the user.

Returning an aggregate error here would be awesome, but unfortunately, we really lack a way of exploring an error hierarchy once an aggregate is involved within it. This makes it difficult for a check higher up in the application to say something like:

In this wrapped error, at any point in the chain of wrapped errors, is there an x509.UnknownAuthorityError ?

In an ideal situation, something like the following code would work (it's a little verbose and psuedocodey, but intending to demonstrate that no amount of wrapping/aggregating should obscure an error in the chain):

firstErr := trace.Wrap(flux.NewCapacitorUnderchargedErr())
secondErr := flux.NewBackupCapacitorMissingErr()

err := trace.NewAggregate(firstErr, secondErr)
err := trace.Wrap(err) // simulating the aggregate being wrapped somewhere in callstack
if errors.Is(err, flux.ErrCapacitorUndercharged) {
  fmt.Println("Your flux capacitor is undercharged, you may want to try charging it before trying again")
} else {
  fmt.Println("We don't have any good advice, the raw error might help: %s\n", err)
}

I believe https://github.com/hashicorp/go-multierror provides a good reference implementation for this sort of behaviour.

Ability to include contextual information / tags with error results

Looking at another issue, there is a desire to suppress errors in portions of code due to user input to keep the logs clean. For example, an API where hitting an invalid input generates an error, that's presented to the user and not considered worth logging.

if !trace.IsNotFound(err) {
  log(err)
}

However, for code that's unrelated to the API, that generates a NotFound error will inadvertently be suppressed as well.

The idea Sasha had, would be to be able to Tag errors, so for example the API code could tag it's NotFound errors, and then only suppress logging errors with a matching tag. I think there is another area that would benefit, is we use retries in several area's, but often we're missing context on errors, on whether the errors is a permanent or temporary failure. A project could adopt a set of best practices for which tags to include with certain types of errors.

logrus for structured logging provides a similar methodology with fields that may be worth considering.

Render aggregated errors properly

Wrapping and aggregating errors cause the error report to be unreadable. This is the number 1 cause of confusion for users attempting to join an instance to a Teleport cluster. Users cannot read the join error properly because it's malformed: they focus on the last part of the error "certificate is not valid" (coming from the direct auth join attempt) and ignore the actual cause (something broken during proxy joining).

Example

The following go code:

package main

import (
	"fmt"
	"github.com/gravitational/trace"
)

func main() {
	proxyErr := trace.Wrap(trace.BadParameter("the fizzbuzz buzzed too hard"), "joining via proxy")
	authErr := trace.Wrap(trace.BadParameter("auth server unavailable: cert invalid"), "joining via auth server")
	err := trace.Wrap(trace.NewAggregate(proxyErr, authErr), "failed to join")
	fmt.Println(trace.UserMessage(err))
}

outputs:

failed to join
        joining via proxy
        the fizzbuzz buzzed too hard, joining via auth server
        auth server unavailable: cert invalid

A sane output would be:

failed to join
        joining via proxy
                the fizzbuzz buzzed too hard
        joining via auth server
                auth server unavailable: cert invalid

Implementation suggestion

We could kill two birds in one stone by changing the trace.Wrap behaviour to encapsulate already wrapped errors instead of adding a message toError.Messages. This would make trace behave like go's standard wrapping and allow us to easily build an error tree. With an error tree, fixing the error rendering would be way easier.

This would likely mean doing a trace/v2 library but would allow us to better integrate with the go ecosystem and address other outstanding trace issues.

Related errors

• gravitational/teleport#32983 caused by malformated aggregate

Wrap does not wrap as expected for external errors

Wrap's current behavior is threading the captured instance of TraceErr (or any other implementation of the Error interface) all the way back to the caller.
This is fine if the error originated and is handled on the same call path.

However, when the wrapped error is of external nature (for instance, deserialized from a remote HTTP request), it is replacing the current call stack with a foreign one and the context is lost. In this case, the expected behavior is that both stacks are preserved.

New proposal to classify errors

Problem

os package has helpers like IsNotFound or IsExists, and the trace package has a similar facility. It has issues:

• It is not compatible with how os helpers work. This constantly introduces subtle logic errors, where the code behaves differently based on how the error was wrapped, as opposed to what the error is.
• HTTP errors are thrown away and replaced with trace's own internal error structures.
• trace also keeps messing with the original error message, sometimes prefixing it with its own prefix, or sometimes (like with HTTP) by replacing it completely, this prevents trace to be used by API clients who rely on meaningful error bodies (JSON) and also prevents HTTP servers from returning user-facing errors.

Proposal

At this point (after fixing several issues in Teleport caused by this) I am firmly convinced there is a better approach:

• trace should drop its own error structures
• helpers like trace.IsNotFound() should work by examining the original error, instead of checking for the correct wrapper
• trace should stop attaching its own "error message" on top of what's already inside the original error. The original error's message should always be preserved and delivered as-is without any alterations.

End Result

If implemented, this proposal will:

• Make trace easier to use as well. The usage will be dead-simple: just call trace.Wrap(err) everywhere.
• Checking with helpers like trace.IsNotFound() will always work, even for errors that weren't wrapped.

README needs updating

trace package has grown and gained a ton of useful features. would be nice to expand the README.md with more examples when time permits.