rs / zerolog

Zero Allocation JSON Logger
MIT License
10.33k stars 564 forks source link

Fix phsym/zeroslog breakage due to rs/zerolog v1.32.0 #653

Closed madkins23 closed 3 months ago

madkins23 commented 6 months ago

TL;DR Add function to create a zerolog.Context object with a logger copy and clear the copy's context byte array in order to fix breakage in phsym/zeroslog.

The recent release of rs/zerolog v1.32.0 broke phsym/zeroslog. The latter was creating blank zerolog.Context objects in several places and they worked fine up through rs/zerolog v1.31.0. I documented the issue in a phsym/zeroslog PR, wherein I provided a potential solution to the issue that didn't require any changes to rs/zerolog.

This PR provides a (hopefully) minor change to rs/zerolog that makes fixing phsym/zeroslog simpler. I have already filed a second presumptive phsym/zeroslog PR dependent on the changes in this rs/zerolog PR.

Hopefully this PR will be an acceptable and minor change.

I gave the new function a long name because it is combining two issues. It could be broken down into:

These two calls could be used instead of the single NewContextWithResetLogger call, something like this:

newCtx := NewContext(logger)
newCtx.Logger().ClearContext()

If the latter is preferable I can generate a different PR.

rs commented 6 months ago

Would https://github.com/rs/zerolog/pull/575 let you solve your issue?

madkins23 commented 6 months ago

It fixes part of the problem. The other part would then just require a NewContext(logger) function to be made public to create a new zerolog.Context and set its private logger field.

rs commented 6 months ago

Couldn't you use UpdateContext for this?

madkins23 commented 6 months ago

TL;DR You're right, #575 would solve the more difficult part of my problem and existing methods on zerolog.Logger appear to cover the rest.

My original work-around PR addressing v1.32.0 on phsym/zeroslog used the With method instead. UpdateContext is a pointer method, With isn't. I was thinking that the With method was more likely to return a copy of the original logger as opposed to accidentally modifying it. I admit I didn't do any testing to see if that would happen.

I guess I considered my use of With as off-label and possibly dangerous in the long run. Having a NewContext(logger) in place seemed more correct and I didn't see #575. But if you're blessing UpdateContext (or better yet With IMHO) for that usage I'm good with it.

rs commented 6 months ago

Yes I think With should do the job.

madkins23 commented 6 months ago

Thanks. Looking forward to #575.

madkins23 commented 4 months ago

This is no longer necessary AFAIK. It would be nice to get a v1.33.0 tag to wrap it all up.

rs commented 3 months ago

Done

madkins23 commented 3 months ago

Thanks! I've pushed a PR to phsym/zeroslog based on #575. I'm going to close this PR.

rs commented 3 months ago

@madkins23 have you considered submitting a PR to integrate your slog support in zerolog itself?

madkins23 commented 3 months ago

Not really. I'm not actually part of the phsym/zeroslog development team so it's not really "my" slog support. I stepped in because I've been using rs/zerolog for half a decade and like it very much. I was testing phsym/zeroslog when V1.32.0 was tagged and the zeroslog wrapper broke.

I did comment on doing something like this in #571 but I phrased it as rs/zerolog V2 which (somewhat predictably in retrospect) didn't go down well. :-(

Let me think about it a bit. I might take a whack at it after I get through with the current set of tweaks to my test harness.