uber-go / zap

Blazing fast, structured, leveled logging in Go.
https://pkg.go.dev/go.uber.org/zap
MIT License
22.08k stars 1.44k forks source link

Error encoder: extra zap fields on new interface type #1462

Open SirSova opened 2 months ago

SirSova commented 2 months ago

Is your feature request related to a problem? Please describe

So, right now in case of error I just put all the information inside the error message (fmt.Errorf/errors.WithMessage) and later on if I want to see specific fields such as IDs on the entry level -- I need to care about it above. It generates duplications: error contains too much information + logs with multiple zap.Field passed inside for more comfortable searching. Typical scenario:

err := fmt.Errorf("something wrong happened, id: %s, status: %d", id, status) // returned from a function
logger.Error("log message", zap.Error(err), zap.String("id", id), zap.Int("status", status))

Describe the solution you'd like

Introduce new interface type zapcore.ErrorMarshaler which gives opportunity to add extra inline fields to the entry.

type ErrCustom struct {
    Message string
    ID string
    Status int
}

func (e *ErrCustom) Error() string { return e.Message }

func (e *ErrCustom) MarshalLogError(enc ObjectEncoder) error {
    // enc.AddString("error", e.Error()) <- this one should probably stay the same by default, inside `encodeError` func
    enc.AddString("id", e.ID)
    enc.AddInt("status", e.Status)
    return nil
}

In code:

err := &ErrCustom{Message: "something wrong", ID: "{UUID}", Status: 1}
logger.Error("log message", zap.Error(err))
// {"level": "error", "message": "log message", "error": "something wrong", "id": "{UUID}", "status": 1, ...}

Describe alternatives you've considered

Reuse zapcore.ObjectMarshaler instead on a new interface, but it may confuse a little. + unexpected behavior in case someone used this interface for their errors already.

Is this a breaking change?

Since we introduce absolutely new interface -- it should be backward compatible, or at least I don't see the possible issues with it... In case of re-usage existing one (ObjectMarshaler), I can imagine unexpected fields to appear for some extraordinary cases, but it's very unlikely from my perspective.

Additional context

  1. The problem may appear in the wrapped error. For example:

    err := fmt.Errorf("wrap original : %w", &ErrCustom{Message: "something wrong", ID: "{UUID}", Status: 1})
    logger.Error("log message", zap.Error(err)) 

    In this scenario, we need to perform Unwrap which may lead to performance problem (is it?) The question is what to do if multiple underlying errors implement the ErrorMarshaler interface. The right solution is probably to call MarshalLogError recursively for each one.

  2. The API can actually provide wrappers for errors that allow easily insert zap.Field into it.

    
    // zap.ErrorWithFields() 
    // OR 
    // zap.FieldedError()

func Do() error { id := "{UUID}" status := 1 originalErr := errors.New("something wrong happened") zapErr := zap.ErrorWithFields(originalErr, zap.String("id", id), zap.Int("status", status)) return zapErr }



3. In case this behavior leads to breaking changes or performance issues -- we can simply add a config/option flag for it, or provide a possibility to change `zapcore.encodeError` behavior in a way.