log: add a DebugLogger that proxies to Logger #1275
Conversation
As originally designed the only log output during normal golden-path operation should be the metadata update heartbeat (one line every ten minutes). If the lines in question are what you're seeing regularly, I would not expect normal operation to require initializing clients after the app itself has finished starting up. |
|
Agreed, if you are only launching one or two clients in your application and keeping them running then the existing logging is quite lightweight. However, if you were (e.g.,) maintaining a large pool of clients that are being started and stopped regularly then the current output can soon escalate. The intention of this PR was move the existing non-error case logging onto the DebugLogger, and also to provide a home for the addition of finer grained logging in Sarama akin to librdkafka's protocol/broker/msg/queue debug tracing. |
I don't have any idea of a use-case where this would make sense, but yes I can see the logging getting messy in that case. I'm not opposed to a PR like this, I'm just wary of adding complexity without good reason. |
|
Thank you for your contribution! However, this pull request has not had any activity in the past 90 days and will be closed in 30 days if no updates occur. |
ghost
added
the
Currently Sarama has the single `Logger` instance of a `StdLogger` that
is used for all log statements within the module. This works well
because user applications can trivially redirect it to their own logging
library of choice simply by implementing the StdLogger interface and
changing the package root variable.
However, currently there are a fair number of quite verbose log statements in the
module that output during normal golden path operation and it would be
helpful to be able to differentiate those from the abnormal output.
e.g.,
```go
Logger.Println("Initializing new client")
Logger.Printf("client/metadata fetching metadata for %v from broker %s\n", topics, broker.addr)
Logger.Printf("client/metadata fetching metadata for all topics from broker %s\n", broker.addr)
```
etc.
This PR adds an additional `DebugLogger` variable that will by default
just proxy all requests to whatever is referenced by the existing
`Logger` variable.
For now, none of the existing log statements have been switched to use
the new DebugLogger as I wanted to confirm that people were happy with
the proposal.
|
Just resurrecting this old PR with a rebase. I'm not sure what (if anything) we want to do about this old idea — I'm happy to just close it if you don't think it's a good fit? |
|
Thank you for your contribution! However, this pull request has not had any activity in the past 90 days and will be closed in 30 days if no updates occur. |
|
Thank you for your contribution! However, this pull request has not had any activity in the past 90 days and will be closed in 30 days if no updates occur. |
|
@dnwe I think this is a good idea, I was searching issues and PRs about this problem. Since there are no log severity levels having a separate logger for problems and non-problems is the next best thing. Who shall we contact for reviewing this PR? |
| // debug information to. By default it is set to redirect all debug to the | ||
| // default Logger above, but you can optionally set it to another StdLogger | ||
| // instance to (e.g.,) discard debug information | ||
| var DebugLogger StdLogger = &debugLogger{} |
There was a problem hiding this comment.
I assume in another PR/this PR you would use this DebugLogger to diversify the different log lines?
There was a problem hiding this comment.
Yes exactly, this was the original intention
|
Is this still something we need? |
|
It is hard for me to respond to this question without being biased as I originally proposed it, but yes I think there is still merit in Sarama having two tiers of log levels 😄 |
|
It would be beneficial in cases like #1565; I ended up creating a wrapper logger so that I could separate the failures from the debug lines and give them the proper highlight. If Sarama can support a logger interface this becomes sensibly more maintainable. |
|
@dnwe Other than agreeing we want this, is there anything else that's preventing this from being shipped? If not, shall we shit this? |
|
yes we can 💩 this if you'd like to review and merge :) I can also follow-up with another PR to move a small number of some of the the more verbose log entries over to using it too |
Currently Sarama has the single
Loggerinstance of aStdLoggerthatis used for all log statements within the module. This works well
because user applications can trivially redirect it to their own logging
library of choice simply by implementing the StdLogger interface and
changing the package root variable.
However, currently there are a fair number of quite verbose log statements in the
module that output during normal golden path operation and it would be
helpful to be able to differentiate those from the abnormal output.
e.g.,
etc.
This PR adds an additional
DebugLoggervariable that will by defaultjust proxy all requests to whatever is referenced by the existing
Loggervariable.For now, none of the existing log statements have been switched to use
the new DebugLogger as I wanted to confirm that the maintainers are
happy with the proposal.