Work-around for zerolog v1.32.0

[madkins23]

The change in zerolog v1.32.0 that breaks things appears to be:

  // should returns true if the log event should be logged.
  func (l *Logger) should(lvl Level) bool {
+ 	if l.w == nil {
+ 		return false
+ 	}
	if lvl < l.level || lvl < GlobalLevel() {
		return false
	}

which is called a couple of stack frames below:

evt := l.Log()

which is present in at least two places in zeroslog code. Up until this change, passing in blank zerolog.Context objects worked fine. Now it is necessary to pass in a proper logger inside of the zerolog.Context object so that the non-nil check for the logger's io.Writer shown above will not fail.

Sadly the l (i.e. logger) field within zerolog.Context is private and there is no public function provided within the zerolog code base to set it. The only way I could find to return a new context object is to use zerolog.Logger.With(). However, that function manipulates a further private field in the zerolog.Logger object named context,¹ an array of byte. This field is used to pre-format attributes for output via zerolog.Logger.WithAttrs() or zerolog.Logger.WithGroup() to speed up output. Using a previously modified zerolog.Logger object to generate the required zerolog.Context object cascades any previously changes made to the logger object down to the "new" logger within the returned context object.

The only way I could work around all of the above was to capture the root logger object in both zeroslog.Handler and zeroslog.groupHandler. The root field in both places is used with With() to create the new context with the root logger. Any changes made to that logger (added to that logger's internal context field) will thus be done to a "clean" logger and will not percolate to more embedded "with" objects. The With() method is used against a Logger object, not a pointer, so the root is not itself changed AFAIK.

Short of changes to the zerolog code this is the best I could think of. I'm not 100% sure I got it right but it's passing tests now so I'm pushing the PR for evaluation and critique.

Footnotes

1. Note that this is unfortunate naming as it is unrelated to either zerolog.Context or context.Context. This is made clear (or further obfuscated, if you will) in the code for zerolog.Logger.With() as well as the declaration of zerolog.Logger wherein both context and ctx are used as field names, the latter referring to context.Context. ↩

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c4ad5d9) 100.00% compared to head (30fff8c) 100.00%.

Additional details and impacted files

@@            Coverage Diff            @@
##              main        #4   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          148       159   +11     
=========================================
+ Hits           148       159   +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[phsym]

Hi @madkins23 awesome, thanks a lot for the detailed analysis and the PR. I need to find some time to look at this in depth. I will soon

[phsym]

The way I did it was a bit hacky already. Maybe it would be worth to check with zerolog's maintainers for some insights.

[madkins23]

I thought about that, but they have 121 open issues and 25 pending PRs so any response will likely take a while. One of those PRs is mine from last May. In addition, I wasn't quite sure what I wanted or how to describe the request as reasonable usage. I'll think on that some more.

Reading over the zerolog issue about adding slog support (with several of your comments) I found the amusing note about basic slog support being similar to zap performance. I've been using zerolog for several years because it benchmarks faster than zap. My benchmarks of several slog handlers currently puts your zerolog wrapper as fastest among the variants I've been testing.

[madkins23]

There is now a PR on zerolog to clear the context from a logger. This coupled with the With method will make this much simpler. I have no idea how long it will be before that PR is accepted and released.

[jbcpollak]

@madkins23 the PR you mention above has been merged, does that allow you to simplify this PR?

[madkins23]

The PR was merged but there is no v1.33.0 tag yet.

My testing used:

require github.com/rs/zerolog v1.32.1-0.20240306065720-74cf37a3965b

so an update to go.mod in this repository to that value should fix things.

I've been hoping for an update to v1.33.0 (it just seems "neater") but I haven't bugged the rs/zerolog crew about it yet.

A better PR for this issue is #6. This PR was an initial attempt and could be closed now.

[madkins23]

Closing as redundant. See #6.