Proposal: Client-Side Hooks in Go

[iheanyi]

Similar to how the server-side hooks area thing, it'd be beneficial to have client-side hooks as well for automating things such as client-side logging, retries, etc.

Here's a rough draft of the API that I've thought up.

type ClientHooks struct {
  // RequestPrepared is called as soon as a request has been created and before it has been sent 
 // to the Twirp server.
  RequestPrepared func(context.Context, *http.Request) (context.Context, error)

 // RequestFinished (alternatively could be named ResponseReceived and take in some response from 
 // the server as well) is called after a request has finished sending. Since this is terminal, the context is 
 // not returned. 
 RequestFinished func(context.Context)

 // Error hook is called whenever an error occurs during the sending of a request. The Error is passed
 // as an argument to the hook.
  Error func(context.Context, twirp.Error) context.Context 
}

Looking forward to hearing y'alls thoughts!

[dpolansky]

Hi @iheanyi, sorry for the late turnaround on this issue.

I think it is important to provide the context that Twirp has server hooks partly because 1.) you can run into issues building HTTP server middleware by wrapping http.ResponseWriter (more details in golang/go#18997) and 2.) the hooks provide functionality that would be difficult to emulate with HTTP server middleware.

With this in mind, we should consider how client hooks would differ from HTTP client middleware (using a type that fulfills the Twirp HTTPClient interface) and whether there is enough of a use case to warrant expanding the public API to add client hooks.

For the RequestPrepared, RequestFinished, and Error hooks that you propose, I think they can all be implemented as a custom HTTPClient. Are there other use cases that you are interested in where HTTP client middleware is not an appropriate solution?

[iheanyi]

Hey there @dpolansky, thanks for circling back on this. I was getting this idea as I was working on the twirp-opentracing package. I think you're right in that the twirp.HTTPClient interface can be a good middleground. I was confused by this at first because while working on twirp-opentracing, I didn't realize that interface existed because it's only defined within the generated code (as seen here), but it's easy to get around this but redefining the HTTPClient interface in your package. You can see my implementation of a client middleware here.

I think that the HTTP Client middleware can be an appropriate solution and allow for greater flexibility. One question that came up when I worked on this is that there were questions on why wrap the exported HTTP client interface rather than just using a RoundTripper. Is it because the request object can be safely modified in the Do method of the middleware?

[gorzell]

I was also curious about something like this. The main use case that came to mind for me was being able to get access to and take action based on twrip.Error, in a way that is generalized/consistent for the entire client. For example to log, record statistics, or perhaps even do retries (I am not yet convinced a hook would give you a way to do this generically)?

As far as I can tell there isn't really a way to safely get at the twirp.Error (which has a ton of extra and useful information) from the HTTPClient. But perhaps I am missing something?

Tangent: One, perhaps interesting, thought that I had was: What if the server could pass a backoff as part of the Error.Meta, and the client could automatically retry with that backoff for certain error codes that also included the back off?

[dpolansky]

@iheanyi The Go standard library makes important distinctions between an http.Client and the http.RoundTripper interface.

From https://golang.org/pkg/net/http/#Client:

A Client is higher-level than a RoundTripper (such as Transport) and
additionally handles HTTP details such as cookies and redirects.

Twirp purposely uses its HTTPClient interface to signal that users have control over those details.

@gorzell I think you're right that the underlying Twirp error coming from the server is hard to get at (without doing something very funky in the HTTPClient). I'm not sure yet what the best way to expose that underlying error is yet, but I think you bring up a a good point about the limitations of client middleware.

[spenczar]

@gorzell is right: client middleware today is too weak to let you do interesting things in a generic way with errors.

Is there a way we can improve that situation specifically? Hooks might be the right tool for the job, but I'd like to understand you'd "turn on" hooks for a client - I don't want to break the signature we have for client construction.

[Bankq]

Twirp purposely uses its HTTPClient interface to signal that users have control over those details.

@dpolansky can you help me understand why Twirp generates HTTPClient for each package instead of defining it in twirp package directly?

[dpolansky]

@Bankq There is some discussion about the TwirpServer interface in #96 that explains why moving that interface to the twirp package may be troublesome. I'm not sure that argument holds as much weight with the HTTPClient interface, which is unlikely to change unless the preferred standard library HTTP implementation changes. Do you have a use case where having it in the twirp package would be helpful?

[Bankq]

@dpolansky thanks for the pointer! Spencer's argument makes sense to me. The issue is I ran into earlier is adding generated twirp code to same package will lead to build error due to the redeclaration.

e.g
Before

from/foo.proto -> to/foo.twirp.go

Then I add bar.proto, and invoke protoc on it and generates bar.twirp.go. At this point I can't simply move it to to package since both foo.twirp.go and bar.twirp.go will have same HTTPClient declared.

I realized that I might be doing something not idiomatic and managed to resolve this by tweaking the workflow. But just curious what's the reason behind it and whether generated something like ServiceHTTPClient instead will allow more workflow flexibility

[spenczar]

@Bankq The right way to deal with that is to invoke protoc on both .proto files in one call, like protoc --twirp_out=. foo.proto bar.proto. It will generate a separate .twirp.go for each, but only one will have the utility types and functions.

[iheanyi]

Do you have a use case where having it in the twirp package would be helpful?

@spenczar Addressing this, HTTPClient will be something used within all client middlewares created by the community and open-sourced. I think having it be in the twirp package will be beneficial so people developing these client middlewares won't have to redefine type HTTPClient interface{} every single time. Just get it from twirp using twirp.HTTPClient.

[iheanyi]

Is there a way we can improve that situation specifically? Hooks might be the right tool for the job, but I'd like to understand you'd "turn on" hooks for a client - I don't want to break the signature we have for client construction.

This was roughly sketched out in the GitHub comment box, so bear with me. I'm wondering if we can use a functional options pattern here...

package twirp

type ClientOption func(*ClientOptions)

type ClientOptions struct {
    Client HTTPClient // requires moving HTTPClient to the twirp package
    Hooks ClientHooks
}

func DefaultClientOptions(client HTTPClient) {
 return &ClientOptions {
   Client: client,
   Hooks: nil,
  }
}

func WithClientHooks(hooks *ClientHooks) ClientOption {
   return func(o *ClientOptions) {
       o.Hooks = hooks
   }
}

The function signature for NewServiceNameProtobufClient and NewServiceNameJSONClient` will end up changing a bit to using a variadic option parameter and change their underlying structs a little:

func NewHaberdasherProtobufClient(addr string, client HTTPClient, opts ...twirp.ClientOption)

But it can be expanded as well to handle these new options:

type haberdasherProtobufClient struct {
	client HTTPClient
	urls   [1]string
	opts   twirp.ClientOptions
}

func NewHaberdasherProtobufClient(addr string, client HTTPClient, opt ...twirp.ClientOption) Haberdasher {
	// Keep a variable to store the client type
	var httpClient twirp.HTTPClient = client
	if c, ok := client.(*http.Client); ok {
		httpClient = withoutRedirects(c)
	}

	opts := twirp.DefaultClientOptions(client)
	for _, o := range opt {
		o(&opts)
	}

	prefix := urlBase(addr) + HaberdasherPathPrefix
	urls := [1]string{
		prefix + "MakeHat",
	}

	return &haberdasherProtobufClient{
		client: httpClient,
		urls:   urls,
		opts:   opts,
	}
}

I think by taking this route, we let the hooks be "turned on" by being a variadic parameter while not breaking the extant type signatures. Let me know what you think and if I overlooked anything.

[iheanyi]

I've opened #164 as a way to explore the initial direction I mentioned in #145 (comment).

[spenczar]

@iheanyi Thanks a lot for that proof of concept. It revealed a few things that I dislike about the options approach:

• The client can be specified either through options, or through the second argument of the constructor. This feels really weird.
• The HTTPClient type must move out of generated code.

But some things I like:

• It's backwards compatible.
• It's flexible enough to support future optional stuff and richness.

I'd like to flesh out a different direction, which is to support a rich client through an optional interface. Optional interfaces have caused a lot of pain in (net/http), but we might be able to do this safely, so I'll at least talk this out.

Introduce a new type in generated code:

// generated server.twirp.go:
type HTTPClient interface {
  Do(req *http.Request) (*http.Response, error)
}

// TODO: better name
type HTTPClientWithHooks interface {
  HTTPClient
  Hooks() twirp.ClientHooks
}

Add a new twirp.ClientHooks type:

// Something like this, anyway.
type ClientHooks struct {
  // TODO: Should any of these return `, error`, to permit early exit?
  RequestPrepared(context.Context) context.Context
  RequestSent(context.Context) context.Context
  ResponseReceived(context.Context) context.Context
  ResponseDeserialized(context.Context) context.Context
  Error(context.Context)
}

And the constructor is unchanged, but it's a little more clever:

func NewHaberdasherProtobufClient(addr string, client HTTPClient) Haberdasher {
  prefix := urlBase(addr) + HaberdasherPathPrefix
  urls := [1]string{
    prefix + "MakeHat",
  }
  if httpClient, ok := client.(*http.Client); ok {
    return &haberdasherProtobufClient{
      client: withoutRedirects(httpClient),
      urls:   urls,
    }
  }
  c := &haberdasherProtobufClient{
    client: client,
    urls:   urls,
  }
  // <<<<<<<<<<<<<<<<<<<
  // NEW MAGIC HERE
  if hookClient, ok := client.(HTTPClientWithHooks); ok {
    c.hooks = hookClient.Hooks()
  }
  // >>>>>>>>>>>>>>>>>>>
  return c
}

---

Optional interfaces are backwards compatible, preserve generated codes' independent definition of the HTTPClient type, and are moderately less weird than the doubled-up parameterization that shows up with options.

However, optional interfaces are painful because they are hard to wrap. For example, suppose I want to wrap HTTPClients to set authentication headers:

func WithAuthHeaders(h HTTPClient) HTTPClient {
  return authHeaderClient{base: h}
}

If the base that I'm wrapping implemented HTTPClientWithHooks, I'm hiding the hooks, here. That's bad! I need to do this:

func WithAuthHeaders(h HTTPClient) HTTPClient {
  if hc, ok := h.(HTTPClientWithHooks) {
    return authHeaderClientWithHooks{base: hc}
  }
  return authHeaderClient{base: h}
}

which means I have two struct implementations of my logic. This gets worse as we add optional interfaces - we need 2^n for n optional interfaces. But more likely, people will just fail to correctly wrap HTTPClients. We could have lots of broken implementations, which I think is worst of all.

[spenczar]

I think we have room to continue to improve this design still. Let's keep identifying options.

One possibly fruitful avenue is to focus on the error-handling portion of this. I don't think the RequestPrepared and ResponseReceived parts of the hooks are nearly as useful as the Error part. Can we focus instead on just letting users handle errors better? That would include logging them, recording metrics about them, and setting retry rules, all through generic libraries that can be applied to any twirp client.

[iheanyi]

You're welcome! I have some follow-up to your thoughts.

The client can be specified either through options, or through the second argument of the constructor. This feels really weird.

@spenczar I agree with this sentiment. The only reason I introduced it through the second argument of the constructor was for a backwards compatibility reason. That option for overriding the client via an option can be removed, I just had it there as an example for a client option.

Looking at the other option, I think we can push it further just a bit. I'm still thinking on it, but I am still not sold on having that interface existing within the generated code. For people to generate external clients with hooks in a third party package, they'd have to redefine that interface every time, not a fun developer experience. I think extensibility for community-created plugins should kept in mind here as well.

One possibly fruitful avenue is to focus on the error-handling portion of this. I don't think the RequestPrepared and ResponseReceived parts of the hooks are nearly as useful as the Error part. Can we focus instead on just letting users handle errors better? That would include logging them, recording metrics about them, and setting retry rules, all through generic libraries that can be applied to any twirp client.

While I do feel that the Error hook is probably one of the most important, I do think that RequestPrepared and ResponseReceived are really important for having a hook for tracing spans, so that's where my head was. Granted, given that the we have this ClientWithHooks pattern, I think there could be a happy middle-ground by focusing only on the Error hook. I just do not know how well the two of them would play together.

Let's keep on ideating this, I think we're close to figuring something out, I'll keep on thinking of alternatives on my end from both directions as well.

[spenczar]

I'm still thinking on it, but I am still not sold on having that interface existing within the generated code. For people to generate external clients with hooks in a third party package, they'd have to redefine that interface every time, not a fun developer experience. I think extensibility for community-created plugins should kept in mind here as well.

I'm of the opinion that interfaces are for describing the inputs to a package, not describing the outputs, so I actually generally think it's preferable for interfaces to be re-defined in each library. It successfully decouples Twirp from those libraries be declaring exactly which behaviors the library relies upon.

I'm not alone in this view - that's in the language's official code review guidelines ("interfaces generally belong in the package that uses values of the interface type, not the package that implements those values") and is a common tip from Dave Cheney.

Redefining the interface is not always going to involve using all of the methods on the interface - like, if you're using TwirpServer in your library, you might not care about ProtocGenTwirpVersion() string. That liberates your library to work with old generated code that doesn't have new fancy methods, unless you explicitly need them.

---

Agreed that we're close! I don't think optional interfaces are great because they are hard to wrap, but I wanted to list it for completeness, at least.

[spenczar]

One last point on interface placement: we have a very concrete case of the benefit of putting the interface in generated code right in #167.

This PR adds a new method to the generated TwirpServer type, PathPrefix() string, which returns their URL path prefix. It's also added to the generated implementations in the same file.

Suppose the PR added it to a type in github.com/twitchtv/twirp, and generated code just referenced that twirp.TwirpServer type. If we merged that PR, all generated code would break! None of the old generated code would have that new method, so none of them would actually implement the twirp.TwirpServer type. You have to regenerate to upgrade to the latest Twirp.

It gets worse: suppose you are responsible for a service that is both a Twirp server and a Twirp client of other services. You want to upgrade to the most recent version of Twirp. You regenerate your server... but your application still won't compile, because you haven't regenerated all the client code. But the client code is owned by different teams. Now you have to go convince them to upgrade - and they run into the same problem.

Worst of all, if you find a cycle of services who are clients and servers of each other, there's just no way out; you have to hack in some non-compiling commits and upgrade them all in lockstep.

Anyway: that's why we put interfaces in the generated code. It decouples things tremendously.

[iheanyi]

@spenczar Yeah, that makes sense to me. Ideally, this would be solved by the Go standard library exporting an interface for HTTPClient like is done within Twirp, but alas, beggars can't be choosers. I think I understand it more around the TwirpServer sense, I guess it's just that HTTPClientwasn't necessarily Twirp specific, so that's where my confusion sets in. In the default case for Twirp, the implementation to me would behttp.Client. But I do see where generated code is more backwards compatible, especially if we were to introduce something like a new ClientWithHooks` interface.

I went through the Twirp codebase and realized that there aren't really that many interface in the top-level code and the benefit of decoupling the two makes sense. Also allows you all to not have to do major version releases with every breaking change to the interface. I appreciate your patience with explaining this and giving context, it further helps me understand the thinking behind y'alls design decisions with Twirp. 😄

[ccmtaylor]

is there any update on this? I'd really like to have hooks to add client-side instrumentation (e.g. metrics, tracing)

@spenczar, AFAICT, your issues with @iheanyi's sketch in #164 were the following:

• The client can be specified either through options, or through the second argument of the constructor. This feels really weird.

• The HTTPClient type must move out of generated code.

I don't think there's anything in the PR that requires moving the HTTPClient type into package twirp. I've created #194 to illustrate. That keeps functional options around to ensure backwards compat, and avoids the issues with optional interfaces.

I'm happy to flesh that out into a "proper" PR if the general aproach is fine.

[iheanyi]

@ccmtaylor I like your approach to this, this approach gets the best of both worlds with backwards compatibility plus the ability to have hooks. I don't know why I moved it out in the first place, heh.

[spenczar]

@ccmtaylor sorry for the long turnaround here! I've been a little swamped lately.

I agree with you that nothing requires moving the type into the twirp package.

Optional arguments are an interesting approach here. There are a few obscure ways in which they won't be backwards compatible (as you saw in the testcase code), but it's pretty close.

I am somewhat wary of ClientOptions; they leave a lot of room for future expansion of the API which can be hard to resist... but I think some vigilance may be sufficient there.

So, overall, yes - this looks like a reasonable direction. Go for it, let's do a real PR.

[iheanyi]

@spenczar Awesome to hear! @ccmtaylor I'm happy to get started on this this week and hopefully land it soon, if that's cool with you? Also happy to jam on this functionality together.

[iheanyi]

Welp, had some time to kill on a flight today, here's a more fleshed out version of Client Hooks: #198.

Let me know your thoughts @ccmtaylor and @spenczar.

[ccmtaylor]

wow, that was quick :). I took a look and left some comments on #198.

[iheanyi]

@ccmtaylor Hm, not seeing anything. Did you submit your review?

[ccmtaylor]

@ccmtaylor Hm, not seeing anything. Did you submit your review?

d'oh, I didn't . Hit the send button now 🤦‍♂️