Add support for Twirp as a Transport

[austinylin]

Twirp (https://github.com/twitchtv/twirp) is an RPC system that works
over HTTP/1.1 using either JSON or Protobuf. It's leverages protobuf for
generating servers and clients.

I still need to do a few things, but wanted to get initial feedback.

Todo

• Add tests (planning to mirror what GRPC has)
• Add Twirp support to addsvc example
• Add documentation

[basvanbeek]

Some quick initial feedback:

• add finalizer hooks for client and server just like http and grpc
• add http.Header to the Request and Response Funcs for allowing transportation of metadata (think tracing, correlation contexts, etc.)

[peterbourgon]

Excited about this 🎉

[basvanbeek]

yes this will be a great addition to the supported transports in Go-kit :)

[groob]

This type matches the endpoint.Endpoint function. Maybe we should just use that?
It's odd to have the NewClient function require a private type as a param.

[austinylin]

@basvanbeek what am I missing here in terms of your comment about http.Header?

[basvanbeek]

So SetResponseHeader is to report back headers to the client. What is missing is the ability to add headers to a client request and for a server to pick them up. Easiest is to think of
http.Header to be similar to gRPC's metadata.MD.

Client side you'd create a fresh empty http.Header which you'd pass to the server before funcs which can then add custom headers to this http.Header map potentially based on information found in client context.

Server side you'd provide the received http headers from the client with a func that provides the server context and returns a potentially updated server context. This allows server before funcs to add stuff to context based on received metadata found in those client headers.

[austinylin]

@basvanbeek I didn't see any finalizers in the grpc client/server, but tried to mirror the http ones without actually getting into the http requests/responses since twirp should in theory abstract that away a bit.

[groob]

@austinylin commenting based on the launcher PR that uses the client

	requestEnrollmentEndpoint := twirptransport.NewClient(
		func(ctx context.Context, request interface{}) (interface{}, error) {
			req := request.(*pb.EnrollmentRequest)
			return apiClient.RequestEnrollment(ctx, req)
		},
		encodeProtobufEnrollmentRequest,
		decodeProtobufEnrollmentResponse,
		attachUUIDHeaderTwirp(),
	).Endpoint()

The first argument is a bit awkward to work with, and a little inconsistent from how grpc.NewClient and http.NewClient are constructed. I didn't look into it too closely, but is there a reason the Twirp API has to require this functional argument?

[austinylin]

@groob I'm not a Twirp expert by any stretch, but best I can tell Twirp doesn't have something like grpc.Invoke which would allow you to call a Twirp method via a function in the Twirp code itself. Instead it generates all the RPC methods at build time from the protobuf files. Thus the ugly code you included above.

There may be a smarter way to do this with reflection, but that is at the end of my go skills :). Open to ideas if anyone has them, agree the current situation is sub-par.

[groob]

@austinylin I think we could implement invoke as a helper using reflect. I'll try to propose what that looks like in a timely fashion.

[basvanbeek]

@austinylin yes gRPC finalizers are not there yet. There is an upcoming PR for this which currently is being worked on.

[austinylin]

@basvanbeek I took a deeper look at the response, request functions per your earlier comment about making it simpler to interact with headers. I was able to update the request functions to make that cleaner, but I wasn't able to do the same for the response functions. Two reasons:

1. On the client side Twirp doesn't support getting headers out of the context for the response.
2. On the server side Twirp doesn't support assigning headers to the response in a robust way. For example, the method Twirp provides to set response headers only supports header.Set meaning that it can't handle multiple values. I didn't implement this at all since it seemed better to just have no support than something that would cause confusion.

You can see the Twirp side of the code here: https://github.com/twitchtv/twirp/blob/master/context.go.

Let me know if you think this is acceptable (with callouts in the docs).

[pauldub]

Hi @austinylin

I'm really interested in you work on this Twirp transport, but I'm not sure how I feel about the reflection based Client from your last commit.

A few days ago I wanted to try using your work on one of my projects but I did not figure how to correctly use the transport and came up with something of my own.

You can find an example here: https://gist.github.com/pauldub/0d9c1b26e1d182c816ba19b126a914ef

I've found that defining the endpoints was really simple and did integrate well with go-kit, as for the server side it is the raw Twirp implementation.

I hope this will be useful to you, and am looking forward to this transport implementation!

[peterbourgon]

Yeah, I haven't dug in to it yet, but I'm generally not on board with package reflect as used here. It is always the case that a given transport/whatever.{Client.Server} wraps explicitly one RPC handler.

[groob]

s/could something/could be something

[groob]

I tried looking at the twirp client. Everything twirp does comes down to calling http.Client.Do(req) with a custom URL path like "/twirp/<package_name>.<service_name>/<method_name"
The trouble is that although there's a lot of very reusable code for creating the request/setting headers/error handling all of that code currently is generated. You could copy/paste that into transport/twirp but then you end up with maintaining something that the twirp codegen changes in the future.

[groob]

I tried making the changes in my fork here groob@4f8e83e I'm not 100% certain I got the reflect strategy correct, although it's just a copy/paste from transport/grpc.

The way you'd construct the client is the following.

1. Create a URL based on the server address + service prefix + method name. and pass it as the tgt argument.
2. Pass in the type of the proto "Reply". struct. This is similar to transport/grpc.

The NewClient method will use the helpers in client_from_twirp.go to create the endpoint. The helpers were all taken from the twirp example https://github.com/twitchtv/twirp/blob/b596491b2e18c4453ff5ba8016a98fb1e663874e/example/service.twirp.go The helpers are all reusable but come from code generation instead of a shared library.

[austinylin]

@peterbourgon, @groob, @pauldub - the reflection commit is basically an attempt to mirror the interface that transport/gRPC provides but without the benefit of the helpers gRPC procides since Twirp doesn’t have then. In my opinion the biggest con of this approach is it can cause a runtime error, vs a compile time error for the user if an invalid method is passed (since it’s a string param). I believe that is the case today for transport/gRPC.

My original approach was just to pass a function to the client constructor which wraps the underlying function in the generated code.

Happy to move forward with either approach depending on what folks think is best. (Edit:) I think we should stay away from breaking the abstractions that Twirp provides simply from a maintainability POV.

Separately, @basvanbeek could you take a look at the updated response/request funcs? I ran into one challenge with the response funcs based on what Twirp supports.

[spenczar]

I discussed this at length with @groob in the #twirp gopher slack. I'd like to throw my own opinion in here, even though I don't know much about kit.

Servers seem to obviously work straightforwardly; all the work here seems to be in making clients nice.

As I understand it, clients are tools to get endpoint.Endpoints. I like @pauldub's solution for clients: the user writes an adapter that converts a particular method into an endpoint.Endpoint.

@groob mentioned that this doesn't leave room for the encoder-decoder pattern used in other transports, though; my understanding is that these encoders and decoders are used to let users define their own types instead of using generated structs throughout the application code.

I think this is easy enough for users to do too, though, in a user-written Endpoint constructor. For example:

// svc is the service interface generated by twirp.
func MakeHatEndpoint(svc example.Haberdasher) endpoint.Endpoint {
	return func(ctx context.Context, req interface{}) (interface{}, error) {
		// Assert that the request object is the right type.
		domainReq, ok := req.(*app.Size)
		if !ok {
			return nil, errors.Errorf("unexpected request type %T", req)
		}
		// Convert the request object to a proto struct, so Twirp can send it.
		protoReq := &example.Size{
			Inches: domainReq.Feet / 12,
		}
		// Actually do the request.
		protoResp, err := svc.MakeHat(ctx, protoReq)
		if err != nil {
			return nil, err
		}
		// Convert the response to the type that the application wants.
		domainResp := &app.Hat{
			Color: protoResp.Color,
		}
		return domainResp, nil
	}
}

This seems clear, clean, and doesn't need reflection, which is good. It doesn't include any forked code out of Twirp - the actual serialization is handled by the underlying svc, which is also good.

The downside is that users need to write a little code. However, I think they'll be doing this regardless: they certainly need to write the encoder/decoder themselves.

[sysbot]

@austinylin when using this code. It seemed that the context.WithValue(ctx, contextkeys.RequestHeaderKey, copied) which suppose to set the ctx value with 5 and encode with the headers and send as a request to the server. However, on server side, this value 5 is never there. What I'm I missing on the client side?

[peterbourgon]

What's the state here? Do we have consensus on a path forward for client?

[austinylin]

Think your feedback would be helpful Peter in terms of what you think is the right approach here.

…

On Fri, Feb 23, 2018 at 11:44 Peter Bourgon ***@***.***> wrote: What's the state here? Do we have consensus on a path forward for client? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#656 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgWhAexolQ14GYW_5qbiPWtIZDHBoqTks5tXxUggaJpZM4Rvp0D> .

[groob]

@austinylin @peterbourgon the way I see it after discussing in gophers slack with Peter and @spenczar (slack link) there are two alternatives:

1. Go back to @austinylin's initial implementation which requires the client to accept an endpoint func that calls the generated client method. Example:

	uppercaseEndpoint := twirptransport.NewClient(
		func(ctx context.Context, request interface{}) (interface{}, error) {
			req := request.(*pb.UppercaseRequest)
			return apiClient.Uppercase(ctx, req)
		},
		encodeProtobufUppercaseRequest,
		decodeProtobufUppercaseResponse,
		opts..,
	).Endpoint()

2. Re-implement the http client that twirp generates as a shared library within kit/transport/twirp.
   The reason we would have to do that is because twirp as it stands today has no shared code for creating a new http request.
   This approach looks undesirable at first, because changes to the code generated by twirp would potentially break the kit implementation. But looking at the state of things today, this approach is not so bad. Twirp has a specification which defines the format of the request, including required headers and error handling.

I took the approach of creating a shared lib in my fork. This implementation yields a client that is more consistent with the other transport packages since it only wraps a single RPC.
The resulting client looks like a mix of transport/http and transport/grpc:

	uppercaseEndpoint := twirptransport.NewClient(
                tgt,                                            // an *url.URL
                pb.UppercaseReply{},                            // the proto Reply struct 
		encodeProtobufUppercaseRequest,
		decodeProtobufUppercaseResponse,
		opts..,
	).Endpoint()

Another thing to consider is that Twirp provides support for both proto and JSON encoding, all contained in the client. The implementation in this PR only deals with protobuf. A decision would have to be made about wether the kit implementation should be broken into two transport packages or encapsulated into a single one.

As Austin stated above, this PR needs some guidance from @peterbourgon, @basvanbeek and other maintainers about which approach to take.

[peterbourgon]

I think we can agree that the reflect-based option is probably out. But after reviewing the remaining approaches I feel unqualified to make a judgment. I like the feel of @groob's option more, but it seems like it carries more risk of future breakage, since Twirp isn't exposing the relevant hooks. @spenczar has your opinion shifted since our chat in Slack awhile ago? Is Twirp open to integrating with systems like Go kit, that want to hook into its "transport layer" exclusive of the rest of the project?

With that said I don't really have anything against @austinylin's approach, it just feels different. My superficial understanding is that it carries less risk related to Twirp changes, would others agree?

@basvanbeek do you have any final opinion here? ("No" is a perfectly acceptable answer.)

[basvanbeek]

I'm with you @peterbourgon in liking Groob's option more and I'm personally not overly worried about future breaking change. Ultimately I have no strong enough favorite to feel like I must be tipping the balance.

I would like to see if we can have some votes from actual prospective consumers of this transport as these are ultimately more important then I am as a non user of this particular transport.

[basvanbeek]

@peterbourgon I'd like to move this forward and having had this marinate a bit more I'm more strongly in favor of the option presented by @groob

The Twirp specification is very simple and clear. Changes in the future should be fairly easy to fix and even having a custom implementation of Twirp removing dependency on Twirp library and protoc plugin is an option.

[peterbourgon]

I'm in favor of this. What needs to happen in this PR? Should it be a separate PR at this point?

[peterbourgon]

@groob @basvanbeek Absolutely no-pressure ping on this one

[nussjustin]

This slice is never used

[nussjustin]

Since client and method are set in NewClient we could move this check there and change it to a panic or (IMO even better) just take a func / method value in NewClient instead of an interface + a method.

This way we can also avoid a few allocations here.

[drscre]

There is a suggestion to use HTTPClient interface instead of *http.Client in this pull request #754

Just in case you will be using *http.Client in this pull request.

[arranf]

I see this PR has been open for a while - I'm wondering if there are still plans to pull Twirp into Go Kit? It'd be awesome if there were!

[peterbourgon]

Closing due to age.