-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
proposal: log/slog: builtin support for helper functions #59145
Comments
I raised a similar concern that 3rd party Handlers don't benefit from HandlerOptions #56345 (comment) |
I raised this on the slog proposal, and jba seemed open to the idea, but he wanted to wait and see what the demand was. Just to repeat something I said there, several of the logging libraries mentioned in the initial slog proposal as "Prior Work" offer an easy way to adjust call depth: pohly mentions zap and logr, but I'll copy my full list below. (I removed logr and hclog from the original list since as jba pointed out, the methods in those libraries modify the original logger.)
|
There's one thing to consider when implementing this: the call depth needs to become an attribute of Bottom line: in addition to |
Right now, it's not a lot of code to create your own helper. For reference, here's the code from the wrapping example:
That's not great, but as you say, it's doable. How could we make it better? We could add a method to We could add a We could add a special
and slog would figure out that the PC should refer to the line calling
The Lastly, there are the XXXDepth functions, as we had originally. Maybe that is the best solution. |
I even find the degrees of freedom alluring, but definitely agree about the foot-gunning. Just to toss out an idea ... a pretty narrow function with an implementation small enough to fit into its own doc string: func Caller(depth int) (fileAndLine slog.Value) {
if _, file, line, ok := runtime.Caller(depth + 1); ok {
fileAndLine = slog.StringValue(fmt.Sprintf("%s:%d", file, line))
}
return
} |
That depends. Your example is simple only because it does printf-style handling of its arguments. When doing the same for a helper that takes key/value pairs like
To me that makes sense. Here's an example:
In this example, both helpers emit log records that point towards where they are called. When The caveat is passing such a logger into a function which isn't considered a helper doesn't work as expected:
That's a problem. Every single function along the call chain has to know that it needs to call this function. It's not possible to optimize cases where a helper is always called n functions down from some top-level API call such that there is a single |
We now have |
@AndrewHarrisSPU, this is like the |
I believe it would be a mistake to tie the call depth into the logger as a WithCallDepth method implies. More generally we went through the whole problem with WithContext of trying to provide a fluent API, and we walked away from that. I think we would need a very good reason to go back to fluent-style APIs. Let's focus on solutions that don't overload Logger with extra state. |
This proposal has been added to the active column of the proposals project |
Based on the discussion above, this proposal seems like a likely decline. |
No change in consensus, so declined. |
I've evidently late to the party, but my use case is to provide a consistent structured logging whilst using packages that use a variety of logging interfaces. That boils down to the requirement to set a 'depth' value when initialising the logger which is not modified thereafter. It would be nice to see that supported here to reduce our reliance on third-party packages where possible. It could take years for log/slog to become the de facto logging solution in Go, and support to achieve that by stealth would be helpful. As an example, sarama expects an interface similar to that of |
@mattdowdell, why wouldn't the approach in my earlier comment work? |
When calling a
Logger
method inside a helper function, the recorded source code is the source code of the helper:Other logging libraries (zap, go-logr) have a
WithCallDepth
call for this.testing.T
hasHelper
, but that is less flexible.It would be useful to add
Logger.WithCallDepth
, then this example would become:The only solution right now is for
logSomething
to translate its arguments into aRecord
, do stack unwinding, then callHandler.Handle
. This is doable, but implies copying quite a bit of code from slog because the helper functions which do that inLogger
are not exported (and probably shouldn't be).cc @jba
The text was updated successfully, but these errors were encountered: