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

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

Closed
wants to merge 3 commits into from

Conversation

madkins23
Copy link
Contributor

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:

  • NewContext(logger Logger)
    which would just create the new zerolog.Context object with a logger and
  • (l Logger) ClearContext()
    which would clear the context byte array on the copied Logger.

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
Copy link
Owner

rs commented Mar 2, 2024

Would #575 let you solve your issue?

@madkins23
Copy link
Contributor Author

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
Copy link
Owner

rs commented Mar 2, 2024

Couldn't you use UpdateContext for this?

@madkins23
Copy link
Contributor Author

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
Copy link
Owner

rs commented Mar 3, 2024

Yes I think With should do the job.

@madkins23
Copy link
Contributor Author

Thanks. Looking forward to #575.

@madkins23
Copy link
Contributor Author

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

@rs
Copy link
Owner

rs commented May 23, 2024

Done

@madkins23
Copy link
Contributor Author

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

@madkins23 madkins23 closed this May 23, 2024
@madkins23 madkins23 deleted the fix-zerolog branch May 23, 2024 14:24
@rs
Copy link
Owner

rs commented May 23, 2024

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

@madkins23
Copy link
Contributor Author

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.

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.

2 participants