log icon indicating copy to clipboard operation
log copied to clipboard

Add caller (file:line number) to non-tracing entries?

Open jesse-c opened this issue 7 years ago • 8 comments

Any opinions on this? It's available from runtime.Caller. Following the current design, it could be enabled by having a function like log.IncludeCaller(bool) and then every entry would include a field caller with value file:line number). Could also implement this as the default log library handles flags, e.g. log.SetFlags(log.LstdFlags | log.Lshortfile).

I'm happy to create a PR for this if you'd like.

jesse-c avatar Jul 22 '17 18:07 jesse-c

It's currently enabled for Error() calls. Personally I don't rely on file or line numbers at all, with pkg/errors wrapping them I find it easy to pinpoint, but an option to enable it for non-error calls couldn't hurt!

For reference it's https://github.com/apex/log/commit/89ce6f11b2dc2a7603f3429d7b3076c526980841. The hacky splitting stuff is because pkg/errors only exposes the obscure Format string :(

tj avatar Jul 22 '17 18:07 tj

Sounds good. :) I'll throw something together.

jesse-c avatar Jul 22 '17 18:07 jesse-c

I decided to not add the runtime.* stuff because I almost always return errors, only my top-most app level has logging, so at that point the file/line is pretty irrelevant, happy to hear what other people think there though. I'm not sure it's super useful.

tj avatar Jul 22 '17 18:07 tj

At least for me (and where I work), one of use cases is that it helps to quickly track exactly where that log message occurred. If a log message is unique, then you can grep for that—but even so, instead of having to grep, you have a reference to where it's from.

jesse-c avatar Jul 22 '17 18:07 jesse-c

I probably should have named the other fields something else, think this should take precedence if enabled? the pkg/error one would be a bit more informative, or a full stack I suppose would be ideal there but that's a lot of noise

tj avatar Jul 22 '17 20:07 tj

Implementation-wise, it might be easier to let the WithError() source take precedence. From my cursory look, I was thinking of a change like this:

func NewEntry(log *Logger) *Entry {
 	e := &Entry{
		Logger: log,
	}

	if logger, ok := Log.(*Logger); ok {
		if logger.IncludeSource {
			// Set source field
		}
	}

	return e
}

Then when WithError() creates its own Fields, then the above source will effectively be ignored.

jesse-c avatar Jul 23 '17 11:07 jesse-c

that never happened did it?

aep avatar Jun 28 '20 12:06 aep

Nope, and my needs changed at the time, IIRC.

jesse-c avatar Jun 28 '20 12:06 jesse-c