-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
support non-clone zapcore #1348
Comments
Hey @sasakiyori, making the core behind a logger mutable isn't ideal. It'll have to be safe for concurrent use somehow, and prevent accidental use of the mutable per-request logger core in a non-cloned context. I haven't thought through the implications of this fully, but I think this might be a change we would be wary of. However, there's an ongoing PR (#1319 by @jquirke) that would introduce a I know that it's not the exact solution you're looking for, but does that help address your concerns? |
I agree. It is used for specific scencario, adding it into the basic package is not a good choice.
How about this: #1019. Is there any plans to accept or reject this PR? It seems to be anothoer way to solve the clone cost. |
Is your feature request related to a problem? Please describe.
There is a common use case (such as gin + zap): I have a global zap.Logger, and every session will create a new logger like below.
I want to set different common kvs for every unique logger due to different request param. As the code goes ahead, there are more and more common kvs need to print for every log record. So I may use the
With
function to embed the common k-v just like the code below.But I find the
With
function will clone logger and inside zapcore, It is expensive for memory cost and also the gc.Describe the solution you'd like
I think I can make a new zapcore, which is the same as
ioCore
except theWith
method.So that I can use it like this:
Describe alternatives you've considered
I saw some similar issue requests, and they prefer to solve this by
context.Context
(seems not merged so far). I think it is feasible but not so comfortable for coding?Is this a breaking change?
No breaking change, just add a new zapcore type.
Additional context
The problem of this solution is that users should clearly understand if this zapcore can be used in their scenario, or errors may occur.
Please tell me if you accept this solution. If OK, I'd glad to create a PR for this.
The text was updated successfully, but these errors were encountered: