Skip to content

Conversation

@Automaat
Copy link
Contributor

Motivation

Kuma has inconsistent error handling:

  • MADR 073 recommends stdlib fmt.Errorf + %w
  • Codebase uses github.com/pkg/errors (30+ usages)
  • pkg/errors archived 2024 - no longer maintained

Requirements: stack traces, best performance, no timeline.

Implementation information

Created MADR 094 analyzing 4 options:

  1. Stdlib only (no stack traces)
  2. Keep pkg/errors (archived, perf issues)
  3. Recommended: cockroachdb/errors (stack traces + perf + maintained)
  4. Custom hybrid (reinventing wheel)

Decision: Gradual migration to cockroachdb/errors.

Supporting documentation

  • MADR 094 with comprehensive references (17 sources)
  • Complements MADR 073 (doesn't replace)

@Automaat Automaat added the ci/skip-test PR: Don't run unit and e2e tests (maybe this is just a doc change) label Nov 27, 2025
@Automaat Automaat changed the title docs(madr): add 094 modern error handling strategy docs(errors): add 094 modern error handling strategy Nov 27, 2025
@Automaat Automaat force-pushed the madr-094-modern-error-handling branch from d34379e to 8c94c51 Compare November 27, 2025 09:20
@github-actions
Copy link
Contributor

Reviewer Checklist

🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
If something doesn't apply please check the box and add a justification if the reason is non obvious.

  • Is the PR title satisfactory? Is this part of a larger feature and should be grouped using > Changelog?
  • PR description is clear and complete. It Links to relevant issue as well as docs and UI issues
  • This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as an image registry)
  • IPv6 is taken into account (.e.g: no string concatenation of host port)
  • Tests (Unit test, E2E tests, manual test on universal and k8s)
    • Don't forget ci/ labels to run additional/fewer tests
  • Does this contain a change that needs to be notified to users? In this case, UPGRADE.md should be updated.
  • Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)

@Automaat Automaat force-pushed the madr-094-modern-error-handling branch from 8c94c51 to eca37b9 Compare November 27, 2025 09:25
@Automaat Automaat force-pushed the madr-094-modern-error-handling branch from eca37b9 to 19e841d Compare November 27, 2025 09:32
Addresses inconsistency between MADR 073 (stdlib) and codebase (pkg/errors).
Recommends migrating to cockroachdb/errors for stack traces + performance.

Co-authored-by: Bart Smykla <[email protected]>
Signed-off-by: Marcin Skalski <[email protected]>
@Automaat Automaat force-pushed the madr-094-modern-error-handling branch from 42ada10 to eb2d91c Compare November 27, 2025 09:35
@Automaat Automaat marked this pull request as ready for review November 28, 2025 07:31
@Automaat Automaat requested a review from a team as a code owner November 28, 2025 07:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@Automaat Automaat changed the title docs(errors): add 094 modern error handling strategy docs(errors): MADR modern error handling strategy Nov 28, 2025
@Automaat Automaat changed the title docs(errors): MADR modern error handling strategy docs(errors): add MADR modern error handling strategy Nov 28, 2025
Comment on lines +36 to +46
### 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
Copy link
Contributor

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 fmt doesn’t provide them—but given the migration effort and the lack of clear performance benefits, I’m not sure it’s worth adopting.

Copy link
Contributor Author

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 fmt in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/skip-test PR: Don't run unit and e2e tests (maybe this is just a doc change)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants