-
Notifications
You must be signed in to change notification settings - Fork 353
docs(errors): add MADR modern error handling strategy #15112
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
Open
Automaat
wants to merge
1
commit into
master
Choose a base branch
from
madr-094-modern-error-handling
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| # Modern Go Error Handling Strategy | ||
|
|
||
| * Status: accepted | ||
| * Supersedes: [MADR 073 - Error Wrapping](073-error-wrapping.md) | ||
|
|
||
| ## Context and Problem Statement | ||
|
|
||
| Inconsistent error handling in Kuma: | ||
| - [MADR 073](073-error-wrapping.md) recommends `fmt.Errorf` + `%w` (stdlib) | ||
| - Codebase uses `github.com/pkg/errors` (30+ usages in `pkg/`) | ||
| - `pkg/errors` archived 2024 - no maintenance/security updates | ||
|
|
||
| Requirements: stack traces, best performance, `errors.Is/As` support, no timeline pressure. | ||
|
|
||
| ## Design | ||
|
|
||
| ### Option 1: Standard Library (`fmt.Errorf` + `%w`) | ||
|
|
||
| Current MADR 073 approach. | ||
|
|
||
| ```go | ||
| var ErrNotFound = errors.New("not found") | ||
| return fmt.Errorf("resource %w: type=%q", ErrNotFound, rt) | ||
| ``` | ||
|
|
||
| **Pros:** No deps, Go team maintained, stdlib compatible | ||
| **Cons:** No stack traces, 5x slower with `errors.Is()` | ||
|
|
||
| ### Option 2: Keep `github.com/pkg/errors` | ||
|
|
||
| Status quo. | ||
|
|
||
| **Pros:** No migration, stack traces | ||
| **Cons:** Archived (security risk), 5x perf degradation, author abandoned | ||
|
|
||
| ### Option 3: Migrate to `github.com/cockroachdb/errors` | ||
|
|
||
| Drop-in replacement, actively maintained. | ||
|
|
||
| ```go | ||
| import "github.com/cockroachdb/errors" | ||
| return errors.Wrapf(err, "sync failed: zone=%s", zone) | ||
| ``` | ||
|
|
||
| **Pros:** Stack traces, better perf, drop-in replacement, maintained, stdlib compatible | ||
| **Cons:** External dep, migration effort | ||
|
|
||
| ### Option 4: Custom Hybrid | ||
|
|
||
| Build custom error types with manual stack capture. | ||
|
|
||
| **Pros:** Full control | ||
| **Cons:** High effort, maintenance burden, reinventing wheel | ||
|
|
||
| ## Security implications and review | ||
|
|
||
| - `pkg/errors`: No security updates, CVE risk | ||
| - `cockroachdb/errors`: Active maintenance, production-hardened | ||
|
|
||
| ## Reliability implications | ||
|
|
||
| - CP handles many DPs, error handling in xDS hot paths | ||
| - `pkg/errors` + `errors.Is()` = 5x slowdown | ||
| - Stack traces essential for multi-zone debugging | ||
| - KDS error propagation benefits from stack context | ||
|
|
||
| ## Implications for Kong Mesh | ||
|
|
||
| Inherits Kuma error handling. Migration synchronizable. | ||
|
|
||
| ## Decision | ||
|
|
||
| **Migrate to `github.com/cockroachdb/errors`** | ||
|
|
||
| Rationale: Stack traces + performance + maintained + drop-in replacement + stdlib compatible. | ||
|
|
||
| ### Migration | ||
|
|
||
| Gradual, no timeline: | ||
| 1. `go get github.com/cockroachdb/errors` | ||
| 2. Update imports: `github.com/pkg/errors` → `github.com/cockroachdb/errors` | ||
| 3. Convert `fmt.Errorf` → `errors.Wrap` for stack traces | ||
| 4. Configure golangci-lint to enforce error handling patterns (errorlint, wrapcheck) | ||
|
|
||
| ### Usage | ||
|
|
||
| ```go | ||
| // Sentinel errors (from MADR 073) | ||
| var ErrNotFound = errors.New("not found") | ||
|
|
||
| // Construction | ||
| func ErrorResourceNotFound(rt, name, mesh string) error { | ||
| return errors.Wrapf(ErrNotFound, "resource: type=%q name=%q mesh=%q", rt, name, mesh) | ||
| } | ||
|
|
||
| // Checking | ||
| if errors.Is(err, ErrNotFound) { /* handle */ } | ||
| ``` | ||
|
|
||
| ## Notes | ||
|
|
||
| Alternative: `gitlab.com/tozd/go/errors` (less adoption) | ||
| Stack overhead: negligible, only on error creation | ||
|
|
||
| ## References | ||
|
|
||
| ### Go Error Handling | ||
|
|
||
| - [Best Practices - JetBrains](https://www.jetbrains.com/guide/go/tutorials/handle_errors_in_go/best_practices/) | ||
| - [Working with Errors - Go Blog](https://go.dev/blog/go1.13-errors) | ||
| - [Error Wrapping - Bitfield](https://bitfieldconsulting.com/posts/wrapping-errors) | ||
|
|
||
| ### Sentinel vs Custom Types | ||
|
|
||
| - [Go Error Handling Techniques](https://arashtaher.wordpress.com/2024/09/05/go-error-handling-techniques-exploring-sentinel-errors-custom-types-and-client-facing-errors/) | ||
| - [Sentinel vs Custom - alesr](https://alesr.github.io/posts/go-errors/) | ||
|
|
||
| ### fmt.Errorf vs pkg/errors | ||
|
|
||
| - [Wrapf vs Errorf - Stack Overflow](https://stackoverflow.com/questions/61933650/whats-the-difference-between-errors-wrapf-errors-errorf-and-fmt-errorf) | ||
| - [Can stdlib replace pkg/errors?](https://blog.dharnitski.com/2019/09/09/go-errors-are-not-pkg-errors/) | ||
|
|
||
| ### Performance & Alternatives | ||
|
|
||
| - [errors.Is() 500% slowdown](https://www.dolthub.com/blog/2024-05-31-benchmarking-go-error-handling/) | ||
| - [CockroachDB errors library](https://dr-knz.net/cockroachdb-errors-everyday.html) | ||
| - [cockroachdb/errors docs](https://pkg.go.dev/github.com/cockroachdb/errors) | ||
Automaat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I’m not fully convinced we should use a new library for this. It seems to include many features we don’t need, which would introduce indirect dependencies and potentially increase our security surface. I do like the stack traces—since
fmtdoesn’t provide them—but given the migration effort and the lack of clear performance benefits, I’m not sure it’s worth adopting.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.
there is not much stuff to migrate actually, it would be similar if we would like to move to
fmtin the future