-
Notifications
You must be signed in to change notification settings - Fork 491
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
base: main
Are you sure you want to change the base?
Juju errors #17378
Conversation
internal/errors/errors.go
Outdated
// Trace implements Errors.Trace. | ||
func (l link) Trace() Error { | ||
return link{newTraced(l.error, 1)} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
Removing a debug statement that had been left in.
/build |
Also removes the Trace() func from Error. We introduce pc based tracing that can be deferred.
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
andError
. These new types and all associated static helper functions are expected to reside in a new packagegithub.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 aboutConstError
.Error
A new type introduced and not to be confused with any existing
Error
types that exist ingithub.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 newerror
in the chain is offering a new piece of information to help the end user make better decisions.The following type
Error
is proposed.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 stderrors
. These functions also act as the means for retrieving aError
type.std errors Helpers
A set of helper functions for dealing with errors.
AsType
github.com/juju/juju/internal/errors
HasType
github.com/juju/juju/internal/errors
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
Tracing errors
To maintain tracing support in the library we will maintain the following functions.
juju/errors
Utility Types
juju/errors
currently maintains a list of utility types for common error scenarios. Example of this areerrors.NotFound
anderrors.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 newcore/errors
package that are references onto thejuju/error
types.For example:
core/errors
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: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
orIs
as defined ininternal/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
[ ] Integration tests, with comments saying what you're testingQA steps
Unit tests for days.....
Documentation changes
Internal documentation has been included in the PR.