nsqio / go-nsq

The official Go package for NSQ
MIT License
2.59k stars 444 forks source link

make getLogger public #224

Closed andyxning closed 6 years ago

andyxning commented 6 years ago

This PR will make getLogger public.

For now,

With getLogger being public, we can easily resolve the above two problems.

there is no way to get currently used logger instance

GetLogger() will return log instance and log level

there is no way to just set the log level


logger, _ := consumer.GetLogger()
consumer.SetLogger(logger, LogLevelDebug)
andyxning commented 6 years ago

/cc @mreiferson @jehiah

mreiferson commented 6 years ago

I'm -1 on this. The default logger is from the stdlib (https://github.com/nsqio/go-nsq/blob/master/consumer.go#L169) and if you want to change the log level you can just pass in your own instantiation of it.

If we want to more thoughtfully and broadly rethink logging for go-nsq in light of evolving Go library best practices I'm all for it.

andyxning commented 6 years ago

The default logger is from the stdlib (https://github.com/nsqio/go-nsq/blob/master/consumer.go#L169) and if you want to change the log level you can just pass in your own instantiation of it.

Without reading the source code, i do not know that the default logger is from log. This is fine to developers, but as for normal users we should have a simple way to just change the log level with original default logger. But as for now there is no way to do this. :)

Otherwise, how about we add a function to set the loglevel SetLogLevel.

ploxiln commented 6 years ago

Maybe what this calls for is some more documentation, like adding an example of setting a log level with a default-type logger here: https://godoc.org/github.com/nsqio/go-nsq#Consumer.SetLogger

andyxning commented 6 years ago

Maybe what this calls for is some more documentation, like adding an example of setting a log level with a default-type logger

@ploxiln Thanks for the suggestion. But i still think we should make users available to get the currently used logger. Or on the other hand why we do not expose the currently used logger? This is a common requirements. :)

andyxning commented 6 years ago

@ploxiln @mreiferson Any updates?

mreiferson commented 6 years ago

@andyxning sorry for the delay! I agree with @ploxiln's suggestion:

Maybe what this calls for is some more documentation, like adding an example of setting a log level with a default-type logger here: https://godoc.org/github.com/nsqio/go-nsq#Consumer.SetLogger

andyxning commented 6 years ago

@mreiferson Seems good to me. I will file another PR to do this. :)

mreiferson commented 6 years ago

Thanks!