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

Feature: logging deep callers #5

Closed
wants to merge 1 commit into from

Conversation

zonescape
Copy link
Contributor

You mention in #2 that there is a need for logging deep callers. Example.

@umputun
Copy link
Member

umputun commented Mar 16, 2019

Technically, this is a nice and simple change, everything clear and make sense. However, conceptually I have a few issues with this approach:

  1. Intenedent use-case of lgr is not the one you can see in remark42. In fact, I don't like such "global logger" thingy and remark42 uses it as a migration path to properly injected logger interface defined on the caller side. However, the only practical use for ForDepth will be that global logger via DefaultForDepth
  2. Attempt to manage logger with the wrapper "on-demand", i.e. inside of a user's method with l.ForDepth won't be even possible if the user follows README advise "make your own interface with Logf and use it".
  3. Another way to use decorated logger will be making it on top-level (main.go) in addition to the regular one and inject it. Obviously, such "dbl-logger" injection will be unusual and very odd.

The bottom line - I think this approach won't be any practical by the same reason why CallerSkip wasn't any good - it requires some kind of "static logger routing", i.e. making and passing multiple loggers unless we (and user) willing to introduce tight coupling with type assertion to lgr.Logger.

I have mentioned an alternative way - CallerIgnore option for proper dynamic detection and skipping unwanted callers. In this case, the user could make a logger with lgr.New(CallerIgnore(specs ....)) and it will be handled dynamically, i.e. matched frames will be ignored.

@zonescape
Copy link
Contributor Author

All the problems you mention can be solved if we modify L interface to:

type L interface {
	Logf(format string, args ...interface{})
	LogfForDepth(calldepth int, format string, args ...interface{})
}

Clients that don't want to log deep callers can use short form of the interface (the current one), the clients that do want this logging will use an extended form.

README advise "make your own interface with Logf and use it"

I don't see this advice in README. Moreover, I see this advice: "pass the concrete logger as a dependency".

an alternative way - CallerIgnore option for proper dynamic detection and skipping unwanted callers

Consider this from the prospective of some module that needs to log indirect callers. If we combine this approach with "make your own interface with Logf and use it" advice then we must conclude that there must be a method for the module to express a list of unwanted callers which is currently absent.

@umputun
Copy link
Member

umputun commented Mar 17, 2019

Extending the current simple and nice single-method interface with such an exotic method feels very wrong to me.

I don't see this advice in README. Moreover, I see this advice: "pass the concrete logger as a dependency".

yeah, my bad. I didn't communicate this part clear enough in README. The proper use case I had in mind is to avoid any coupling with the particular logger by defining and accepting local interface and not passing lgr.L around. This is, for example, done in go-pkgz/auth. The advice you quoted meant to say "don't use a global logger if you can avoid it"

Consider this from the prospective of some module that needs to log indirect callers. If we combine this approach with "make your own interface with Logf and use it" advice then we must conclude that there must be a method for the module to express a list of unwanted callers which is currently absent.

I don't get it. The one (re)defining and using lgr only need to have a single Logf method implemented/delegated. If our constructor allows such option it won't affect interface in any visible way. This is pretty much the same as all other options we support. And we already have such option CallerIgnore, it just half baked, not documented and respected in narrow cases only.

@zonescape
Copy link
Contributor Author

I don't get it.

I mean this:

// ... inside some package that needs to log indirect callers

func foo() {
	bar() // I want this call to be logged
}

func bar()  {
	lgr.DefaultForDepth(1).Logf("[INFO] xxx")
	// some other work
}

With my approach the author (!) of a package can say lgr.DefaultForDepth(1) to skip one frame. With your approach it is currently impossible. This can be done only by some code outside of the package which is not controlled by the package author.

type L interface {
	Logf(format string, args ...interface{})
}

var l L

func foo() {
	bar() // I want this call to be logged ...
}

func bar()  {
	l.Logf("[INFO] xxx") // ... but not this. How can I express this?
	// some other work
}

@zonescape zonescape closed this Mar 19, 2019
@zonescape zonescape deleted the deep-callers-logging branch March 20, 2019 08:54
@zonescape zonescape mentioned this pull request Mar 21, 2019
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.

None yet

2 participants