[
https://issues.apache.org/jira/browse/THRIFT-4985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16958211#comment-16958211
]
Duru Can Celasun commented on THRIFT-4985:
------------------------------------------
I strongly agree with principal 1. Let's remove all logs that could be replaced
with error returns. To avoid BC breaks, let's add new functions and deprecate
the old ones, e.g given the following function:
{code:java}
func Foo() string {
if fail {
log.Println("foo failed")
return ""
}
return "bar"
}
{code}
let's replace it with:
{code:java}
// Deprecated: Use GetFoo
func Foo() string {
v, _ := GetFoo()
return v
}
func GetFoo() (string, error) {
// logic
}
{code}
For principal 2, I mostly agree but instead of a function it should be an
unexported interface, i.e:
{code:java}
type logger interface {
Print(string)
}
{code}
We can have our own implementation of it (by wrapping stdlib's log) and we only
expose a single function:
{code:java}
var (
log logger
)
func SetLogger(l logger) {
log = l
}
{code}
This way, callers won't have to depend on our interface definition.
> Clean up logging in go library
> ------------------------------
>
> Key: THRIFT-4985
> URL: https://issues.apache.org/jira/browse/THRIFT-4985
> Project: Thrift
> Issue Type: Improvement
> Components: Go - Library
> Reporter: Yuxuan Wang
> Priority: Major
>
> As principals:
> # As a library, the library itself shouldn't do any logging. It should
> always return the error to the caller (library user), and let them to decide
> how to handle them (logging, panic, etc.)
> # In some cases it's not possible to return the error to the caller, so some
> logging is necessary, but those should be kept minimal, and made more
> controllable to the caller.
> Looking at how we do logging in go library now:
> # We have logging inside
> [TZlibTransport|https://github.com/apache/thrift/blob/6e4c581fddae9106c2c5a59c4d0bfbe6ad3e4560/lib/go/thrift/zlib_transport.go#L70],
> which fits Principal 1 above and should be removed
> # We have logging inside TSimpleServer, which fits Principal 2 above (those
> are in the accept loop so there's no easy way to return the errors to the
> caller)
> For Principal 2, one of the problem is that we can't just force all the
> callers to use the same logging library. There are a lot of logging libraries
> and it's not always possible to convert them into each other. Some people
> might be using [go-kit logger|https://godoc.org/github.com/go-kit/kit/log]
> because they want structured logging, or because they are already using
> go-kit for other things, and some people might be using
> [zap|https://godoc.org/go.uber.org/zap] for its performance. They are both
> incompatible to the stdlib log library.
> This, combined with the part that we should keep the logging minimal, is why
> I propose a simple, common ground that's easy to wrap any logging library
> into:
> {code:go}
> type Logger func(msg string)
> {code}
> Whatever logging library the user is already using, it's easy to write a
> lambda to convert that to this common ground, for example for the stdlib log
> library:
> {code:go}
> func StdLogger(logger *log.Logger) Logger {
> return func(msg string) {
> logger.Print(msg)
> }
> }
> {code}
> You can even have one to be used in tests, because the purpose of logging is
> that it's only used for situations that is bad, we can make it that whenever
> the logging is called we should fail the test:
> {code:go}
> func TestLogger(tb testing.TB) Logger {
> return func(msg string) {
> tb.Errorf("logger called with msg: %q", msg)
> }
> }
> {code}
> This way, the caller can make sure that the logging from inside the thrift
> library is using the same logger as what they are already using, or they can
> suppress the logging entirely if they choose to.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
