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

feat: implement an optional ErrorWriter interface #365

Closed
wants to merge 1 commit into from

Conversation

Deiz
Copy link

@Deiz Deiz commented Sep 25, 2021

@rs no idea if you'll want to merge this - this is something I'll be adopting internally so that errors logged via zerolog at warn level or above can be enriched with request context, then pushed into an external error aggregator; Sentry in my case.

This builds on the idea proposed in #329

A few design notes:

  • The reason I went for an optional interface on the writer is because
    errors need level context, generally. In my use case, I don't want to
    send errors below "warn" level to an external error aggregator.
  • Intended use of this requires an ErrorWriter implementation, which
    should be injected into the logger on a per-request basis such that
    all logged errors are enriched with the same request context.
  • The []error embedded within the Logger, Event, and Array structs is
    there to guarantee Discard() behaves correctly, and also that e.g. an
    Array that is built up can benefit from the ErrorWriter that may be
    embedded within the event it will eventually be attached to.

This builds on the idea proposed in rs#329

A few design notes:
- The reason I went for an optional interface on the writer is because
  errors need level context, generally. In my use case, I don't want to
  send errors below "warn" level to an external error aggregator.
- Intended use of this requires an ErrorWriter implementation, which
  should be injected into the logger on a per-request basis such that
  all logged errors are enriched with the same request context.
- The []error embedded within the Logger, Event, and Array structs is
  there to guarantee Discard() behaves correctly, and also that e.g. an
  Array that is built up can benefit from the ErrorWriter that may be
  embedded within the event it will eventually be attached to.
@Deiz
Copy link
Author

Deiz commented Nov 20, 2023

@rs I'm curious if you'd be open to this or a similar change at this point.

I was in the middle of updating this branch (it's over two years old now 🙂) for internal usage and noticed that #559 solves some of my use case, but ultimately the core issue that motivated this PR remains.

Chiefly, Zerolog doesn't offer a way to retain errors as error - they're immediately marshalled, just like everything else, which means losing some fidelity in custom error types, etc.

My question to you is whether you feel the use case of needing to do something special with errors when a Zerolog event is written rises to the level where you'd consider it worthy of being integrated into Zerolog itself.

@rs
Copy link
Owner

rs commented Nov 20, 2023

I think this should not be part of a logging library, but part of your code that handle errors.

@Deiz
Copy link
Author

Deiz commented Nov 20, 2023

There are use cases like the following, where one might want to log, but otherwise ignore an error:

for _, item := range items {
  result, err := fallibleOp(item)
  if err != nil {
    log.Err(err).Str("id", item.ID).Msg("Failed to perform operation")
    continue
  }

  ...
}

That's where we find it useful to ensure that we dispatch errors externally (to Sentry, in my company's case) as an operation that's coupled with actually writing the Zerolog event out.

In scenarios where errors are fatal and you bubble them up, I agree that having a standard error handler a) write to third-party integrations, b) log via Zerolog makes complete sense.

There's nothing stopping us from having something that looks like func ReportError(ctx context.Context, event *zerolog.Event, message string, err error) to accomplish this in scenarios like the above example, of course - it'd just be nice to have this in Zerolog's own ergonomics.

That said, seems like you're got a pretty strict separation of concerns in mind - so I suspect this'll remain a behaviour unique to my fork.

@rs
Copy link
Owner

rs commented Nov 20, 2023

Yes I think the logger should be a destination and not a hub for all your app events. For instance if you decide to sample your logs, should your errors handlers be sampled as well? Probably not.

@rs rs closed this Mar 2, 2024
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.

None yet

2 participants