Skip to content

DESIGN.md

Latest commit

 

History

History
251 lines (208 loc) · 11.9 KB

DESIGN.md

File metadata and controls

251 lines (208 loc) · 11.9 KB
Raw

AsertoError & Logging Redesign

Status: Draft — for discussion, no code changes yet.

1. Motivation

AsertoError was built to do three jobs at once: a sentinel-error registry, a structured-context builder (mirroring zerolog's Event API), and a grpc<->HTTP status bridge. Reviewing the type against how it's actually used in the three consumers that matter (go-directory/pkg/derr, topaz, azm) turned up two confirmed bugs and a design mismatch between what the package offers and what's actually used:

  • Copy() shallow-copies the errs []error slice (errors.go:62-77), so two errors derived from the same parent via .Err() can share a backing array and silently overwrite each other once Go's slice growth leaves capacity slack. Reproduced directly — a 3-deep chain branched into two siblings and one silently absorbed the other's appended error. azm already does derr.ErrX.Err(derr.ErrY.Msgf(...)) (safe/relation.go and others), so this is live exposure, not a lab curiosity.
  • FromGRPCStatus (errors.go:280-301) mutates the package-level singleton returned by asertoErrors[code] in place (result.data = t.Metadata) instead of operating on a copy. Reproduced directly — calling it once permanently changes the shared data on every future use of that error code, and it's an unsynchronized concurrent write on top of that.
  • The package's own GRPCStatus() (errors.go:249-260) is never called by any of the three consumers, and it's the broken half of the design: it never writes HTTPCode into the errdetails.ErrorInfo metadata that CustomErrorHandler (custom_error_handler.go) reads back out. The actual grpc<->HTTP correlation in production is hand-rolled a second time in topaz/internal/grpc/middlewares/gerr/gerr_middleware.go, which manually calls .Int(aerr.HTTPStatusErrorMetadata, asertoErr.HTTPCode) before building the status — the one thing this package exists to provide is duplicated and has drifted from the library's own version of it.
  • Usage sweep across go-directory, topaz, azm (grep for real AsertoError-typed call chains, not zerolog's identically-named builder methods): NewAsertoError, .Msg/.Msgf, and .Err are the workhorses. .Str/.Int32/.Int64/.Bool/.Duration/.Time/.FromReader/ .Interface/SameAs/Equals/WithGRPCStatus/WithHTTPStatus have zero real call sites in any of the three repos. aerr.Logger/.Data()/ .Fields()/ContextError have exactly one call site — gerr_middleware.go — but it's the single choke point every gRPC error response in topaz passes through, so it's load-bearing despite the low count. azm never touches the logging surface at all, yet imports it transitively (zerolog, grpc-ecosystem/grpc-gateway/v2/runtime) for zero benefit.

This is why the fix isn't a patch to Copy() — the logging/gRPC-gateway coupling and the wrapping mechanism are the same code paths that need to change shape anyway.

2. Non-goals

  • Not rewriting topaz's or azm's own general-purpose logging (their existing .Str().Msg() zerolog call sites outside the errors-package boundary are untouched).
  • Not touching the error registries (derr, internal/eds/pkg/ds/error.go) — they keep calling NewAsertoError(code, grpcCode, httpCode, msg) unchanged.
  • Not picking a new wire format for cross-service error propagation — errdetails.ErrorInfo inside the grpc Status stays; it's the right mechanism for the stated goal (survives the grpc-gateway boundary, standard idiom).

3. Proposed design

P1 — Slim the core type

Keep exactly what's used; deprecate the rest for one release cycle before removal (see §5).

type AsertoError struct {
    Code     string
    GRPCCode codes.Code
    HTTPCode int
    Message  string
    data     map[string]string // copy-on-write, as today
    wrapped  error              // see P3 — was []error
}

Kept: NewAsertoError, Msg/Msgf, Err, Str (used to attach a single metadata field — keep, low cost, unlike the rest of the typed setters), Data/Fields, Error/Unwrap, Copy, Ctx.

Deprecated for removal: Int/Int32/Int64/Bool/Duration/Time/ FromReader/Interface, SameAs, Equals, WithGRPCStatus, WithHTTPStatus. Zero real call sites found across all three consumers.

P2 — One source of truth for grpc<->HTTP correlation

Replace the broken GRPCStatus() with a helper that both the library and gerr_middleware.go can use, including the Reason field the middleware currently sets itself (a correlation/error ID, which is caller-specific and shouldn't be generated by this package):

// ErrorInfo returns the errdetails.ErrorInfo for this error, with HTTPCode
// folded into Metadata under HTTPStatusErrorMetadata.
func (e *AsertoError) ErrorInfo(reason string) *errdetails.ErrorInfo

// GRPCStatus builds the grpc Status via ErrorInfo(""). Satisfies the
// GRPCStatus() *status.Status interface some grpc tooling expects.
func (e *AsertoError) GRPCStatus() *status.Status

gerr_middleware.go then becomes:

errResult, err := status.New(asertoErr.GRPCCode, asertoErr.Error()).
    WithDetails(asertoErr.ErrorInfo(errID.String()))

removing its hand-rolled duplicate of WithDetails/ErrorInfo construction, and guaranteeing CustomErrorHandler always finds the HTTP status key, because the same code path is now the only path.

P3 — One wrapping mechanism

errs []error plus custom Error()/Unwrap() string-joining coexists today with pkg/errors-style wrapping (ContextError.Cause) and native errors.Unwrap. Nothing in go-directory, topaz, or azm chains more than one .Err() call in practice (the multi-error chain is only exercised by this package's own tests). Replace the slice with a single wrapped error field:

func (e *AsertoError) Err(err error) *AsertoError {
    if err == nil {
        return e
    }
    c := e.Copy()
    c.wrapped = err
    return c
}

This removes the aliasing bug by construction — there's no shared backing array left to alias — rather than papering over it with a deep copy that still leaves a subtler version of the same hazard if the slice comes back later. If genuine multi-error wrapping turns out to be needed somewhere, the stdlib errors.Join is the right tool for that, orthogonal to this type.

This changes Error()'s multi-wrap output format (TestDoubleCerr, TestError's 4-chain case) — those tests get rewritten, not preserved.

P4 — Logging: slog at the boundary, zerolog stays the engine in topaz

  • errors core module drops the zerolog dependency entirely.

  • AsertoError implements slog.LogValuer:

    func (e *AsertoError) LogValue() slog.Value

    replacing MarshalZerologObject.

  • A new ctxlog package owns the "logger in context" convention that ContextError/extractLogger already half-implement today, but as a standalone utility rather than only reachable by wrapping an error (log/slog deliberately ships no such helper — this has to be owned):

    package ctxlog

func With(ctx context.Context, logger *slog.Logger) context.Context
func From(ctx context.Context) *slog.Logger

  • Logger(err error) walks the error chain like today but returns *slog.Logger, sourced via ctxlog.From.

  • topaz keeps zerolog as its actual encoder by writing (or adopting) a small slog.Handler backed by a zerolog.Logger — the 4-method interface (Enabled/Handle/WithAttrs/WithGroup) is cheap to own directly rather than take on a third-party bridge dependency. Two things this bridge must handle deliberately, or they silently regress:

    • gerr_middleware.go's .Stack() call (via pkg/errors) — no native slog stack-trace attr convention exists; the handler needs to recognize and format it.
    • Allocation behavior — zerolog's whole pitch is zero-allocation logging; routing everything through generic slog.Record/Attr conversion adds some. Almost certainly noise next to network/DB calls on topaz's request path, but worth one benchmark on the per-decision logging path before assuming it's free, not after.

P5 — Dependency isolation needs module boundaries, not just packages

azm importing errors today pulls in zerolog and grpc-ecosystem/grpc-gateway/v2/runtime transitively even though it uses neither. Moving CustomErrorHandler and the zerolog bridge into subpackages of the same module doesn't fully fix this — Go's module graph still resolves those requirements for anything that imports the module at all. Getting azm genuinely free of zerolog/grpc-gateway requires separate go.mod files:

  • github.com/aserto-dev/errors — core type, no zerolog, no grpc-gateway.
  • github.com/aserto-dev/errors/zlog (or similar) — the slog.Handler zerolog bridge + ctxlog. Only topaz imports it.
  • github.com/aserto-dev/errors/httpgw — CustomErrorHandler. Only whatever actually runs a grpc-gateway ServeMux imports it.

This is a real increase in release/versioning overhead (three modules to tag instead of one) — flagged as an open question in §6, not a foregone conclusion.

4. What doesn't change

  • NewAsertoError(code, grpcCode, httpCode, msg) registry pattern — used ~45+ times in go-directory/derr alone, untouched.
  • errdetails.ErrorInfo inside grpc Status as the wire mechanism.
  • Context-carried logger as the standard, replacing the current inconsistent mix of direct-pass and context-pass in topaz (per prior discussion) — this is what ctxlog formalizes.

5. Sequencing

Phase Scope Depends on
0 Decide open questions below —
1 errors: P1 (slim type) + P3 (single wrapped) + P2 (ErrorInfo/GRPCStatus fix) — pure logic, no logging changes Phase 0
2 errors: split zlog/httpgw into separate modules (P5), drop zerolog/grpc-gateway from core go.mod Phase 0 (module-split decision)
3 errors/zlog: ctxlog + zerolog-backed slog.Handler + LogValue() on AsertoError (P4) Phase 2
4 topaz: migrate gerr_middleware.go to ErrorInfo()/ctxlog, removing its duplicate grpc-status construction Phase 1, 3
5 topaz/azm: bump to the new errors version; deprecated methods removed once the same repo-search technique used in this review confirms zero remaining call sites Phase 4

Both go-directory/derr (v0.34.0 pinned in topaz) and azm (v0.0.17 pinned — notably far behind topaz's pin) are still pre-1.0, so Go's semver convention allows breaking changes on a minor bump without a /v2 module path. That gives more room to move than a post-1.0 package would have, but azm's stale pin is worth checking — it may mean nobody has attempted an upgrade in a while for a reason not visible from usage grep alone.

6. Open questions

  1. Module split (P5). Worth the added release overhead of three versioned modules instead of one, to fully isolate azm from zerolog/grpc-gateway? Or is "core module free of those deps, topaz-only subpackages that azm simply never imports" (accepting the module-graph resolution cost, not the compile cost) good enough in practice?
  2. Urgency of the Copy() fix. P3 removes the aliasing bug by construction, but it's bundled with the larger redesign. Given azm already exercises the vulnerable pattern in production, does the errs→wrapped change (or a minimal interim deep-copy patch) need to ship on its own first, ahead of the rest of this doc?
  3. Deprecation window. How long do the dead builder methods stay present (marked Deprecated:) before hard removal — tied to how quickly topaz/ azm can bump their pins.
  4. Where ctxlog lives. It's a generically useful "logger in context" helper with no inherent connection to errors. Keep it inside this module (even if split per P5) since it grew out of ContextError, or should it be its own small standalone module from the start?