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

Added support for setting logger for each log level #275

Merged
merged 1 commit into from Jan 11, 2020
Merged

Added support for setting logger for each log level #275

merged 1 commit into from Jan 11, 2020

Conversation

ghost
Copy link

@ghost ghost commented Nov 23, 2019

I’ve added support for using separate loggers for each level. I tried to minimize the changes required to existing code and maintain as much backwards compatibility as I could.

The change required to existing code is the requirement to add a call to SetLoggingLevel() if the default level (LogLevelInfo) is not the desired level. Previously the call to SetLogger() would do this implicitly.

This change also expands the API surface by 1 with a convenience function SetLoggerForLevels() that sets the same logger for the levels provided as parameters. This convenience function simplifies achieving the previous behavior of using just one logger for all levels.

@jehiah
Copy link
Member

jehiah commented Dec 23, 2019

@crazyweave can you describe what your use case is for this change?

@ghost
Copy link
Author

ghost commented Dec 24, 2019

I was debugging the unusual number of high priority tickets generated from go-nsq and determined that instead of only error logs generating tickets, every log message being logged (given a logging level) generates a ticket.

The root of our issue was an incorrect assumption regarding the behavior of SetLogger() - only one logger and one logging level is allowed for the entire package.

This PR enables package consumers to employ a more nuanced approach to setting up logging for go-nsq.

This allows us to redirect error log messages to get the right attention while routing all other log messages elsewhere - including suppressing them by not setting a logger for that log level.

@ploxiln
Copy link
Member

ploxiln commented Dec 24, 2019

I think that, if we are to go forward with this, we should keep more compatibility for existing public methods. SetLogger() should set the logger for all levels in order to be analogous to what it did before, and then you could have a new SetLoggerForLevel() to set one for a single level.

@ghost
Copy link
Author

ghost commented Dec 24, 2019

@ploxiln Sounds perfectly reasonable.

@ghost
Copy link
Author

ghost commented Dec 25, 2019

I've pulled the latest changes from master and made the requested changes.

@ghost
Copy link
Author

ghost commented Jan 9, 2020

@jehiah or @ploxiln

producer.go Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jan 9, 2020

@ploxiln Thanks for reviewing this. Do you see anything else or have any more concerns?

@ploxiln
Copy link
Member

ploxiln commented Jan 10, 2020

Nope, I don't see anything else, looks fine to me. I'm not really one of primary authors of this component, so I'll give @jehiah a bit more time to comment.

One last little thing though, can you squash down to one commit? Thanks :)

@ghost
Copy link
Author

ghost commented Jan 10, 2020

@ploxiln - squashed to single commit.
@jehiah - Any concerns with this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants