Skip to content
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

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

Open
SirSova opened this issue Aug 29, 2024 · 0 comments
Open

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

SirSova opened this issue Aug 29, 2024 · 0 comments

Comments

@SirSova
Copy link

SirSova commented Aug 29, 2024

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.

  1. 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
}
  1. 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant