[
https://issues.apache.org/jira/browse/THRIFT-4985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16958257#comment-16958257
]
Duru Can Celasun commented on THRIFT-4985:
------------------------------------------
Fair enough, I didn't think this through. Let's go with your approach. We are
importing stdlib log in 3 places: zlib_transport, debug_protocol and
simple_server. Zlib can be removed, debug protocol and simple server's
innerAccept should use the new logger.
For processRequests though, we should just remove the [panic
recovery|https://github.com/apache/thrift/blob/cecee50308fc7e6f77f55b3fd906c1c6c471fa2f/lib/go/thrift/simple_server.go#L232-L236].
A library shouldn't be recovering from panics caused by user defined functions
(rpc handlers).
Thoughts?
> 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)
