AddCaller hook can't be wrapped

[akshayjshah]

As currently implemented, the AddCaller hook doesn't work when wrapped (e.g., by Standardize) - we're hard-coding the number of stack frames to skip when finding the caller.

[justyntemme]

Could this be fixed by adding an oop style setSkip function? Then just make sure to call setSkip before calling AddCaller.

[akshayjshah]

That's more or less what I had in mind. I'd planned to add an IncCallerSkip option and have the zwrap constructors automatically pass the right values.

Are you thinking of opening a PR? 😍

[justyntemme]

Just issued a pr to fix this part way. Now we should just have to change the calls to addcaller.

[akshayjshah]

This is a near-duplicate of #83 - they're the same root cause, and should be fixed together.

[mranney]

Any progress on this?

[akshayjshah]

Not really, unfortunately.

The core problem here is that the approach we've taken is fast, but fragile - each logger needs to know exactly how many frames to skip. We can extend this approach to let CheckedMessage and wrapper types increment/decrement the skip count, but this gets pretty fiddly pretty fast.

I'd like to finish the other 1.0-related features, then see if there's a clean way to fix this problem. If there isn't, I'd like to remove the AddCaller hook until we're come up with a fast, clean implementation.

[rogpeppe]

We have this issue, as we are wrapping many our logging calls in a function that obtains the logger from a context value.

Letting CheckedMessage change the skip count would seem a reasonable solution to me, given that that type is explicitly intended for log wrapper use.

Perhaps CheckedMessage.Write method could take a "callDepth" count to let it know what the depth count is (as log.Logger.Output does). Or add a new method (WriteWithCallDepth ?) if backward compatibility is desired. I'm thinking that the method name doesn't need to be that short because it won't be used in many places.

[jcorbin]

Noting that some progress was attempted in the abandoned #183, and that #201 moves forward on this: the global _callerSkip is being dropped in lieu of a new logger.callerSkip field. Going forward we can do either or both of:

• add fields to allow varying skip on a per log site basis (or even opting in to caller without doing it for all log sites)
• add an option to change logger.callerSkip (e.g. zap.AddCallerSkip(int))
• further make any / that option more useful by allowing it to be changed for sub loggers (e.g. the zap.Logger.WithOptions proposal)

[jcorbin]

Updating context: after #227 on dev, the only remaining piece changing skip on an existing logger after creating it. Whatever the mechanism of change is (type casting, with-options, or else) there is no more global skip value on dev.

[akshayjshah]

The last part of the fix for this just landed on dev. Woohoo!