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

Use proposed new zerolog function to fix breakage #6

Merged
merged 5 commits into from
Sep 23, 2024

Conversation

madkins23
Copy link

@madkins23 madkins23 commented Feb 26, 2024

This is a proposed fix for the rs/zerolog v1.32.0 breakage. It depends on a change to the rs/zerolog code that I have yet to file, the addition of a single function:

func NewContextWithResetLogger(l Logger) Context {
    l.context = enc.AppendBeginMarker(make([]byte, 0, 500))
    return Context{l: l}
}

More detail

My previous PR used zerolog.Logger.With to generate a new zerolog.Context object because there was no other way to generate one with a specific logger. This would be replaced by NewContextWithResetLogger. In addition, in my previous PR I had to plumb a "root" logger through so that the With method wouldn't generate a context with predefined fields. In this case the first line of NewContextWithResetLogger replaces any predefined fields with a single { character via enc.AppendBeginMarke).

Next Steps

If we're both satisfied with this I can file a PR on rs/zerolog to create NewContextWithResetLogger (or something like that). I also wanted to have this PR ready so that I can refer to it when I file that PR.

In the meantime, if you want to test this:

  1. git clone both (examples use ssh):
  2. Edit github.com/madkins23/zeroslog/go.mod:
    • In the replace line change the path to my clone of madkins23/zerolog
      to the location of your clone of that repository in step 1.
  3. Run go test ./... in the madkins/zeroslog clone directory.

If you want to verify that you're using the right version of zerolog you can edit file github.com/madkins23/ctx.go and add a fmt.Println to the beginning of the init function.

I've also used a similar mechanism to pull all of this into my benchmark/verification test harness, which has a bunch of specific tests. No errors there either.

As a side note, I'm mortified that I didn't now how to use the replace line in go.mod until now. Would have saved me some amount of work over the last few years. 😞

@madkins23
Copy link
Author

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

@madkins23
Copy link
Author

I just pushed my latest version of this PR which runs against a local clone of zerolog with their PR #575. Tests in this repository succeed and I can run my verification tests against it successfully. So we're waiting on a zerolog release with that change and then this one can go in.

@madkins23
Copy link
Author

madkins23 commented Mar 6, 2024

PR #575 has been merged. I've tested this branch against the specific commit from that merge and it continues to work fine. This PR is now passing, but only because it's using the unreleased commit.

I would suggest letting this PR rest until the next zerolog release, then fixing it for that release and merging it down.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (0b7a5ea) to head (a54d659).

Additional details and impacted files
@@            Coverage Diff            @@
##              main        #6   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          148       148           
=========================================
  Hits           148       148           

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

@madkins23
Copy link
Author

rs/zerolog now has a v1.33.0 version. All phsym/zerolog unit tests and madkins23/go-slog verification tests succeed.

Time to merge this in?

@madkins23
Copy link
Author

FYI: I just had to block rs/zerolog v1.33.0 from go-slog because it doesn't work without this PR. If this PR is not currently acceptable then it might be wise to block v1.33.0 from phsym/zeroslog as well, in the same way v1.32.0 is currently blocked.

@phsym phsym merged commit 0098b07 into phsym:main Sep 23, 2024
1 check passed
@phsym
Copy link
Owner

phsym commented Sep 23, 2024

Hi @madkins23 sorry for the long delay. I've been quite busy, and your PRs completely went out of my radar after. It's merged now. Thank you very much for your work

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.

3 participants