Skip to content

feat: add structured error wrapper for controller errors#628

Draft
Yetkin Timocin (ytimocin) with Copilot wants to merge 2 commits into
mainfrom
copilot/add-error-logging-convention
Draft

feat: add structured error wrapper for controller errors#628
Yetkin Timocin (ytimocin) with Copilot wants to merge 2 commits into
mainfrom
copilot/add-error-logging-convention

Conversation

Copilot AI commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Constructors like NewAPIServerError / NewUnexpectedBehaviorError logged via klog.ErrorS as a side-effect while returning, causing one failure to produce multiple log lines. The fmt.Errorf("%w: %v", Err, err.Error()) pattern also broke errors.Is against the inner error chain.

New API — pkg/utils/controller/errors.go

Symbol Purpose
wrappedError Holds sentinel + inner error + accumulated kv pairs; Unwrap() []error returns both so errors.Is resolves the full chain
WithValues(err, kv...) Appends klog-style kv pairs while bubbling up; chainable
LogAndUnwrap(err, msg) Emits exactly one klog.ErrorS with all accumulated kvs, returns unwrapped inner error
// intermediate layer — attach context, don't log
func fetchConfig(ctx context.Context, name string) error {
    if err := k8sClient.Get(ctx, key, obj); err != nil {
        return controller.WithValues(controller.NewAPIServerError(false, err), "config", name)
    }
    return nil
}

// handler — log once, here
func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
    if err := fetchConfig(ctx, req.Name); err != nil {
        return reconcile.Result{}, controller.LogAndUnwrap(err, "failed to reconcile")
        // → klog.ErrorS(inner, "failed to reconcile", "fromCache", false, "reason", "NotFound", "config", "my-config")
    }
    ...
}

Constructor changes — controller.go

Removed klog.ErrorS side-effect from all four constructors; structured kvs previously passed to the log call are now embedded in the wrapper:

Constructor Removed side-effect kvs preserved
NewUnexpectedBehaviorError klog.ErrorS(err, "Unexpected behavior...", "stackTrace", ...) "stackTrace"
NewExpectedBehaviorError klog.ErrorS(err, "Expected behavior...")
NewAPIServerError klog.ErrorS(err, "Error returned...", "fromCache", ..., "reason", ...) "fromCache", "reason"
NewUserError klog.ErrorS(err, "Failed to process...")

Existing Err* sentinels and their public identity are unchanged. Call sites that currently do klog.ErrorS(controller.NewXxxError(...)) still compile; migration to WithValues/LogAndUnwrap is a follow-up.

Convention — CONTRIBUTING.md

Added "Error handling convention" section: wrap with WithValues while returning, emit once with LogAndUnwrap at the handler.

Refs #627

- Add pkg/utils/controller/errors.go with wrappedError, WithValues, LogAndUnwrap
- Remove klog.ErrorS side-effects from NewUnexpectedBehaviorError, NewExpectedBehaviorError, NewAPIServerError, NewUserError
- Fix fmt.Errorf(%w: %v) pattern that lost inner error chain
- Add errors_test.go with table-driven tests for all new functionality
- Update CONTRIBUTING.md with error-handling convention

Refs #627

Agent-Logs-Url: https://github.com/kubefleet-dev/kubefleet/sessions/38f6461d-8fff-48b7-a1cb-5d3d76438229

Co-authored-by: ytimocin <5220939+ytimocin@users.noreply.github.com>
Copilot AI changed the title [WIP] Add convention for logging errors where handled feat: add structured error wrapper for controller errors Apr 23, 2026
@ytimocin

Copy link
Copy Markdown
Collaborator

Copilot some checks are failing. Please fix.

Copilot AI commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/graphql
    • Triggering command: /usr/bin/gh gh pr checks (http block)
  • https://api.github.com/repos/kubefleet-dev/kubefleet/actions/runs
    • Triggering command: /usr/bin/gh gh run list --branch copilot/add-error-logging-convention (http block)
  • prices.azure.com
    • Triggering command: /tmp/go-build684420947/b1203/azure.test /tmp/go-build684420947/b1203/azure.test -test.testlogfile=/tmp/go-build684420947/b1203/testlog.txt -test.paniconexit0 -test.gocoverdir=/tmp/go-build684420947/b1203/gocoverdir -test.coverprofile=/tmp/go-build684420947/b1203/_cover_.out -test.v=true -test.timeout=30m0s RNitEvrZN -errorsas -ifaceassert t pkg/mod/golang.o-mode -ato�� -bool ortcfg rg/toolchain@v0.-outfilelist -errorsas -ifaceassert t rg/toolchain@v0./home/REDACTED/work/kubefleet/kubefleet/test/apis/v1alpha1/zz_generated.deepcopy.go (dns block)
    • Triggering command: /tmp/go-build4274049578/b1203/azure.test /tmp/go-build4274049578/b1203/azure.test -test.testlogfile=/tmp/go-build4274049578/b1203/testlog.txt -test.paniconexit0 -test.gocoverdir=/tmp/go-build4274049578/b1203/gocoverdir -test.coverprofile=/tmp/go-build4274049578/b1203/_cover_.out -test.v=true -test.timeout=30m0s pkg/mod/golang.org/toolchain@v0.-plugin-opt=-pass-through=-lc -errorsas (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

// wrappedError carries a sentinel error, an inner error, and accumulated klog-style
// key-value pairs. It preserves the full error chain so that errors.Is resolves both
// the sentinel and any error nested inside the inner error.
type wrappedError struct {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd suggest moving this to its own internal package.
maybe wrap, or errwrap? unless we forsee more error related helpers, and just have it in an errors package?
errors.Wrap(err, ...)
or wrap.Err(err, ...) both read nicely.
preference for the latter as it is clearer that it's our package, and therefore has special wrapping handling.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Convention: log errors where handled, wrap when returning

3 participants