edmodo/thrift — pull request #2 by dvander (2014-01-15 19:50:47+00:00)
Title: Refactor thrift processing for multiplexing and logging

(This is more of a request-for-comments than a review.)

For post-mortem debugging, we want to log the parameters of all requests Thrift services. Unfortunately, the API provides no clean way to do this, even on a per-service basis. Even if it could, multiple layers of glue abstraction exist in between the initial message routing and the eventual call into user code. This makes it difficult to track a single request from start to finish.

Conceptually, Thrift's TProcessor is a good idea in that it allows a great deal of flexibility in how messages are routed. Realistically, it is difficult to use: to re-route a message it must be partially unpacked, then re-packed into a new stream type. It makes more sense to define a "header" component of a message that can be used for *all* routing.

This patch does the following things:
(1) Defines a new "TRouter" interface. Thrift's generated code will now make handlers that are compatible with TRouter, instead of TProcessor, which is too low-level.
(2) Defines a new "Request" interface which describes the header component of a message and its in/out streams. The responsibility of a TProcessor is now to generate a Request and give it to a TRouter.
(3) Defines a THandler interface, which has a generic logging callback. User handlers must conform to THandler, though it can just be empty stubs:

func (s *MyServiceHandler) Log(request Request, args ...interfaces{}) {
}

This approach seems like a reasonable start, but there are things I don't like:
(1) If your processor uses a custom Request object, it must be explicitly casted in Log callbacks to get at custom fields. To fix this we would need the code generator to understand that certain services have specific request types.
(2) My golang-fu was not strong enough to make the Log callback optional, so right now it is a type error not to have one.