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

Juju errors #17378

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Juju errors #17378

wants to merge 7 commits into from

Conversation

tlm
Copy link
Member

@tlm tlm commented May 15, 2024

Introduction

Since juju/errors was first conceived a lot of changes have happened in the Go errors space that are starting to be in conflict with the design of this library. Originally the plan was to introduce a v2 of juju/errors library that encompassed all of the new concepts within the library. After talking with @SimonRichardson we agreed that the library is not adding any real value and that this proposed errors change is best off living in core Juju.

This work aims to both document my thoughts around how errors would now work in Juju going forward and what our transition path looks like. This work is very much out of band for Juju's current concerns and at the moment this PR aims to solicit feedback on the design and implementation.

Core Idea

With this new design I would like to have it focus around two central types ConstError and Error. These new types and all associated static helper functions are expected to reside in a new package github.com/juju/juju/internal/errors.

Const Error

This is the same new type introduced in juju/errors with no additional changes. The following properties will still remain true about ConstError.

type ConstError string

const ErrorDoohickey = ConstError("doohickey failure")

const ErrorOther = ConstError("doohickey failure")

const ErrorNoMatch = ConstError("no match for doohickey")

fmt.Println(ErrorDoohickey == ErrOther)
// True

fmt.Println(ErrorNoMatch == ErrOther)
// False

newErr := fmt.Errorf("foo bar %w", ErrorDoohickey)
fmt.Println(newErr)
// foo bar doohickey failure

fmt.Println(errors.Is(newErr, ErrDoohickey))
// True

fmt.Println(errors.Is(newErr, ErrNoMatch))
// False

Error

A new type introduced and not to be confused with any existing Error types that exist in github.com/juju/errors. The idea of this type is to introduce a builder pattern where by once you have a variable of this type you can further build on the error adding more and more context.

Errors are to be formed in chains err1 -> err2 -> err3 where each new error in the chain is offering a new piece of information to help the end user make better decisions.

The following type Error is proposed.

// Error represents a Go error generated by this package that can further be built on. All Errors are immutable.
type Error interface {
  // error represents the Go error encapsulated by this type and makes clear that
  // Error conforms to the error interface as well.
  error
  
  // Add adds the error to this error returning an error chain that satisfies the
  // additional error. The newly constructed chain will be origError -> addedError. The
  // new Error returned by add does not change the original error.Error() message.
  // This is useful in cases where the producer of the error would like the error to
  // satisfy some existing error type but not have the added errors Error() string
  // pollute the message of the original error.
  // This functionality was similar to errors.Hide() in juju/errors
  //
  // Example:
  // const ErrorInvalid = ConstError("invalid operation")
  //
  // e := errors.Errorf("user id %q is not understood", userId).Add(ErrorInvalid)
  //
  // fmt.Println(e.Error())
  // // user id "bluey" is not understood
  //
  // fmt.Println(errors.Is(e, ErrorInvalid))
  // // True
  Add(error) Error
  
  // Trace returns a new error indicating the source code file and line number where
  // the Error.Trace() function was called from.
  // See ErrorStack for usage.
  Trace() Error
  
  // Unwrap implements stderrors.Unwrap() and returns the error being encapsulared by [Error].
  Unwrap() error
}

Deliberate design has been made with Error so that it cannot be directly constructed by the end user and can only be obtained by using the global functions of this package. At the moment this new design is pushing the idea that errors should be handled an enriched instead of just annotated with debugging information and passed up the stack blindly.

Static Functions

This section documents the Global static functions on the package and their purpose.

std errors

The following new functions will be added to github.com/juju/juju/internal/errors to maintain compatibility with std errors. These functions also act as the means for retrieving a Error type.

```go
// Join returns an error that wraps the given errors. Any nil error values are
// discarded. Join returns nil if every value in errs is nil. The error formats as
// the concatenation of the strings obtained by calling the Error method of each
// element of errs, with a newline between each string.
//
// A non-nil error returned by Join implements the Unwrap() []error method.
func Join(errs ...error) Error

// As is a 1:1 implementation of errors.As
func As(error, any) bool

// Is is a 1:1 implementation of errors.Is
func Is(error, error) bool

// New is a 1:1 implementation of errors.New with the difference being that a
// [Error] is returned as the resulting type so that the error can further be
// enriched.
//
// Changes from juju/errors: New will no longer return either a caused based error or
// a error that has been traced. If the caller wishes to trace the error they
// can perform New("my error").Trace()
func New(string) Error

// Unwrap is a 1:1 implementation of errors.Unwrap
func Unwrap(error) error

std errors Helpers

A set of helper functions for dealing with errors.

AsType
github.com/juju/juju/internal/errors

// AsType finds the first error in err's chain that is assignable to type T. If a match is found AsType returns the error value and true. Otherwise, it returns T's zero value and false.
// AsType is equivalent to errors.As, but uses a type parameter and returns the target, to avoid having to define a variable before the call.
func AsType[T error](err error) (T, bool)

HasType
github.com/juju/juju/internal/errors

// HasType is a function wrapper around AsType dropping the return value from AsType().
func HasType[T error](err error) bool

fmt errors

The following fmt functions will be included in the set of static functions offered by this package.

github.com/juju/juju/internal/errors

// Errorf is a 1:1 implementation of [fmt.Errorf] with the difference being that
// a [Error] is returned as the resulting type so that the error can further be
// enriched.
//
// Changes from v1: Errorf will no longer return either a caused based error
// or a error that has been traced. If the caller wishes to trace the error they
// can perform Errorf("id %q not found", id).Trace()
func Errorf(string, ...any) Error

Tracing errors

To maintain tracing support in the library we will maintain the following functions.

// ErrorStack returns a string with a new line for each error in the chain. If the error contains tracing information that will also be printed with the error.
// Backwards compatability will be kept with errors that have had trace called on them from juju/errors.
func ErrorStack(error) string

juju/errors

Utility Types

juju/errors currently maintains a list of utility types for common error scenarios. Example of this are errors.NotFound and errors.NotValid. It is the hope going forward in Juju that we will move away from these types long term to more finer grained package level errors. To deal with the transition and places where these error types are still required. We will create a new core/errors package that are references onto the juju/error types.

For example:
core/errors

const (
  Timeout = errors.Timeout
  NotFound = errors.NotFound
  ...
)

This will allow legacy code to still reference these types and keep functioning.

On top of the utility types in juju/errors there exists a set of construction methods for these types:

func IsAlreadyExists(err error) bool {...}
func IsBadRequest(err error) bool {}
func IsForbidden(err error) bool
func IsMethodNotAllowed(err error) bool
func AlreadyExistsf(format string, args ...interface{}) error
func BadRequestf(format string, args ...interface{}) error
func Forbiddenf(format string, args ...interface{}) error
func MethodNotAllowedf(format string, args ...interface{}) error
func NewAlreadyExists(err error, msg string) error
func NewBadRequest(err error, msg string) error
func NewForbidden(err error, msg string) error
func NewMethodNotAllowed(err error, msg string) error
func NewNotAssigned(err error, msg string) error
func NewNotFound(err error, msg string) error

As part of this proposal we would not be handling or bring across transition mechanisms for these types. All of their usages can be replaced with either Errorf or Is as defined in internal/errors.

Transition Strategy

With the above proposed interfaces we can maintain backwards compatibility with errors and situations that currently use juju/errors while being able to cut new code against this implementation. This will support a gradual transition while DQlite work is taking place.

The next step after this PR and approval of the design will be whole sale change to Juju uses of juju/errors.

Checklist

  • Code style: imports ordered, good names, simple structure, etc
  • Comments saying why design decisions were made
  • Go unit tests, with comments saying what you're testing
  • [ ] Integration tests, with comments saying what you're testing
  • doc.go added or updated in changed packages

QA steps

Unit tests for days.....

Documentation changes

Internal documentation has been included in the PR.

@tlm tlm added the do not merge Even if a PR has been approved, do not merge the PR! label May 15, 2024
internal/errors/traced.go Outdated Show resolved Hide resolved
internal/errors/traced.go Show resolved Hide resolved
internal/errors/traced.go Outdated Show resolved Hide resolved
Comment on lines 86 to 72
// Trace implements Errors.Trace.
func (l link) Trace() Error {
return link{newTraced(l.error, 1)}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this opt-in or opt-out? Currently, our package is opt-in, if we want to make this change we should socialize this change. When do we want to trace the error, at a boundary layer or every error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss this today. Personally I have never like it but I am happy to keep it as the changes are worth it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outcome from the conversations today.

  • We are going to trace all newly created errors.
  • Traced errors will store the program counter instead of the location.
  • Trace() can be removed from Error as errors are traced by default.

tlm added 2 commits May 16, 2024 07:26
This commit starts off a new stem of work for Juju by giving Juju a new
internal errors package so that it can have it's reliance on the current
juju/errors library removed.

This commit brings in a new package called internal/errors for all the
new types. The first change is we have introduced proxying functions for
all the go stderrors helpers. All of these proxied functions are a 1 to
1 mapping the exception that where an error return value is used we have
changed the type to internal/errors.Error type that allows the caller to
further annotate the error more.

We have introduced a new Error type that allows annotating existing
errors with more information and tracing.

On top of this ConstError from juju/errors has been brought over with
the existing tests established.
Previously Juju has been relying on getting these common error types
from juju/errors. By bringing them into Juju we can start to break our
reliance on juju/errors.

These new core error types are equivelent and comparable to the ones
found in juju/errors.
internal/errors/const.go Outdated Show resolved Hide resolved
internal/errors/const.go Outdated Show resolved Hide resolved
tlm added 2 commits May 16, 2024 07:51
Removing a debug statement that had been left in.
@tlm
Copy link
Member Author

tlm commented May 16, 2024

/build

@tlm tlm removed the do not merge Even if a PR has been approved, do not merge the PR! label May 16, 2024
tlm added 3 commits May 16, 2024 11:01
Also removes the Trace() func from Error.

We introduce pc based tracing that can be deferred.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants