Combine encoding's ReqMeta into a single top-level ReqMeta

[HelloGrayson]

Currently, we have 3 different ReqMetas:

• raw.ReqMeta
• json.ReqMeta
• thrift.ReqMeta

The only difference between the 3 is that thrift.ReqMeta does not have a Procedure field, since we determine the procedure name based on the idl method they are calling. We chose this path because, in theory, each encoding might have different request metadata. This also makes it really clear that you don't need to set Procedure when making Thrift calls.

I'd like to reconsider that position. Maintaining a separate ReqMeta object per encoding adds mental overhead, makes it a bit awkward to add additional encodings, and is not the most obvious experience. We've received feedback that this is a bit surprising.

Instead, I'd like to see a single top-level yarpc.ReqMeta and yarpc.ResMeta structs:

type ReqMeta struct {
    Context context.Context
    Procedure string
    Headers Headers
    TTL time.Duration
}

type ResMeta struct {
    Headers Headers
}

This produces an experience like so:

// raw

resBody, resMeta, err := rawClient.Call(
    yarpc.ReqMeta{
        Procedure: "yolo-raw",
        Context: context.Background(),
        Headers: yarpc.Headers{"bob": "ishome"},
        TTL: time.Second,
    },
    []bytes{"raw bytes yall"},
) 

// thrift
// note we don't provide a Procedure
// that will get written (or overwritten) by the encoding,
// you could even imagine allowing someone to override at the call-site if provided

resBody, resMeta, err := userServiceClient.Search(
    yarpc.ReqMeta{
        Context: context.Background(),
        Headers: yarpc.Headers{"bob": "ishome"},
        TTL: time.Second,
    }, 
    userservice.Filter{FirstName: "bob"},
    userservice.LastUUID("..."),
)

// protobufs (feels same as thrift, except only 1 respBody struct)

resBody, resMeta, err := emailServiceClient.Send(
    yarpc.ReqMeta{
        Context: context.Background(),
        Headers: yarpc.Headers{"bob": "ishome"},
        TTL: time.Second,
    }, 
    emailservice.To{Email: "bob@sacajawea.com", From: "Your Friends"},
)

• Every encoding uses the same ReqMeta, but has varying request bodies
• Every encoding uses the same ResMeta, but has varying response bodies

[kriskowal]

ResMeta needs to carry a Context.

[HelloGrayson]

@kriskowal that's an aside, but yup, agreed.

[abhinav]

I'm in favor of per-encoding *Meta objects because, it gives us more flexibility around per-encoding metadata. Besides Thrift/Protobuf having fewer fields in the ReqMeta, we have the possibility of having more fields in the ResMeta for encodings that don't have a standard concept of application errors (JSON/Raw).

For example, you could imagine having an IsApplicationError bool field on ResMeta for JSON and Raw which would let users indicate that their payload contains an application error (which the other side needs to know how to decipher). We would not want this field available in Thrift ResMeta because the IDL defines the application errors.

func MyHandler(req *json.ReqMeta, body *GetValueRequest) (interface{}, *json.ResMeta, error) {
  ...
  if ... {
      return &CustomResponseShape{Error: "stuff"}, &json.ResMeta{ApplicationError: true}, nil
  }
  return &CustomResponseShape{Success: result}, nil, nil
}

Combining these Meta objects puts us in a tough spot when we need to add fields that other encodings don't need. I don't like that the contract will become "leave this as zero value if you think you don't need to specify this."

This can also increase user confusion: When people see the godocs for the combined Meta objects, they'll see these fields that they don't need to fill if they're using Thrift/gRPC with YARPC. If they do, we'll overwrite them (or worse, use what they gave and end up making an invalid request), which IMO is an API design smell in itself. We'll have this jank-sounding documentation: "Don't fill these fields if you're using Thrift because we'll overwrite them."

I don't think it's very confusing to have per-encoding meta objects. It's surprising the first time you see it because it's easy to assume that there's a single Meta object, but after that it's fairly straightforward. You're already doing, $encoding.New(..) to build clients for a specific encoding, so there's nothing too confusing about using $encoding.Foo{..} to build objects that those clients rely on.

[yurishkuro]

Disclaimer: I don't have a very strong opinion, and don't know enough details about yarpc architecture.

One thing that strikes me as odd as an end user is the leak of abstraction when encoding have to be considered in client/handler code. One big promise of yarpc is that the end user code does not need to think about the transport. That's a great abstraction. But another abstraction can be the encoding. This abstraction is already partially implied by the example

func MyHandler(req *json.ReqMeta, body *GetValueRequest) (interface{}, *json.ResMeta, error)

The handler takes body *GetValueRequest argument, which, I recon, is a domain class defined by the user, devoid of encoding concerns. So it would be nice if the whole signature was encoding-neutral. That way the business code can be written once, and both transport and encoding concerns are addressed by configuration.

Again, as I haven't been involved in the design, I don't know if what I'm saying is doable, but I think it would be a nice level of abstraction. On the other hand, if the encoding cannot be abstracted away from the handler signature, the body might as well be json.Message.

My 2c.

[shawnburke]

@yurishkuro yep, exactly. This is RPC so all I care about is getting my call from here to there.

So a few thoughts here:

1. I still believe that the handler implementations should be as consistent as possible and not tied to the encodings and the transport. This results in a simpler, more consistent system that we can more easily tool and evolve. As a service author, I really don't care how errors are communicated over the wire. Just make it work. If I return an error (it's fine if it's an error defined in the IDL, that's orthogonal, but I just return that. The only time I would change this is for app layer semantics (e.g. HTTP REST things would return an HTTPError that had a message and a status code, but that's not an RPC thing).
2. WRT the examples above, I don't think there is very much value in making the client and server ReqMeta objects the same. There's a bunch of stuff that's typically available on the server side that the client knows nothing about. I think trying to have symmetry here just makes life harder in the future. I also think that the encoding should be as abstracted as possible on the client side, FWIW.
3. I don't think the procedure name is a strong enough reason for these to be separate.
4. I think these things should be interfaces and not structs so that we can make them smart based on the encoding type. E.g. there is a thriftReqMeta that gets all of the stuff that it needs to inspect headers and whatever in a polymorphic way.
5. If we are really worried about extra metadata that you "might" have with certain encodings, then we should just stick it into a property bag that hangs off the ReqMeta for handlers that might care about this stuff (do we have examples?). I realize that this raises concerns about type safety but literally every single calling framework I can think of (even in type safe languages) does this. Standard things have strongly typed accessors, other things are just well known keys.
6. Using reflection (smartly and carefully) typically allows a much cleaner user model for users. We should make sure that the cost there is really not worth paying (e.g. it's actually going to matter in real life scenarios, it's not something we could get rid of later with some simple codegen if it was a problem, etc). I realize this is heresy in the Go world but, again, it's how most of these systems end up doing things when the language doesn't give you a type system that's flexible enough to make things nice both for consumers and for performance.

[abhinav]

Okay, I think I'm convinced that a single meta object is better. I prototyped a new API based on the discussion above. I'm half way through implementing it. I figured now is a good time to get feedback.

We now have Re{q,s}Meta interfaces with constructors for both of them in the yarpc package. Both interfaces support fluid setters because users will need to provide these objects at all call sites.

package yarpc

type ReqMeta interface {
    Context() context.Context
    Headers() Headers
    Procedure() string
    SetHeaders(Headers) ReqMeta
    SetProcedure(string) ReqMeta
}

func NewReqMeta(context.Context) ReqMeta

type ResMeta interface {
    Context() context.Context
    Headers() Headers
    SetHeaders(Headers) ResMeta
}

func NewResMeta(context.Context) ResMeta

(Note: ResMeta has context attached to it for backwards context propagation.)

Usage looks like this,

// JSON
var res GetValueResponse
resMeta, err := jsonClient.Call(
    yarpc.NewReqMeta(ctx).
        SetProcedure("getValue").
        SetHeaders(yarpc.NewHeaders().With("foo", "bar")),
    &GetValueRequest{...},
    &res,
)

// Thrift
resBody, resMeta, err := keyValueClient.Call(
    yarpc.NewReqMeta(ctx).
        SetHeaders(yarpc.NewHeaders().With("foo", "bar")),
    &GetValueRequest{...},
)

// Handler implementation in both JSON and Thrift will look similar
func GetValue(reqMeta yarpc.ReqMeta, body *GetValueRequest) (*GetValueResponse, yarpc.ResMeta, error) {
    res := doStuff(reqMeta.Headers(), body)
    return res, yarpc.NewResMeta(reqMeta.Context()).SetHeaders(...), nil
}

Worth noting: For the majority case where headers won't be provided, yarpc.NewReqMeta(ctx) is certainly shorter than the encoding-specific types:

yarpc.NewReqMeta(ctx)
&json.ReqMeta{Context: ctx}
&thrift.ReqMeta{Context: ctx}

---

On top of that, based on @shawnburke's suggestion: I plan on splitting the Re{q,s}Meta objects based on the direction they are traveling in. This will allow us to stick more information on inbound ReqMeta object.

Something like,

// Write-only interface to provide the necessary information for an outgoing
// request.
type ReqMetaOut interface {
    SetProcedure(string) ReqMetaOut
    SetHeaders(Headers) ReqMetaOut
}

func NewReqMetaOut(ctx context.Context) ReqMetaOut

// Read-only interface to read information about an incoming request.
type ReqMeta interface {
    Caller() string
    Context() context.Context
    Encoding() transport.Encoding
    Headers() Headers
    Procedure() string
    Service() string
}

// Write-only interface to provide information for an outgoing response.
type ResMetaOut interface {
    SetHeaders(Headers) ResMetaOut
}

func NewResMetaOut(ctx context.Context) ResMetaOut

// Read-only interface to read information about an incoming response.
type ResMeta interface {
    Context() context.Context
    Headers() Headers
}

Usage will look almost the same as above except the NewRe{q,s}MetaOut constructor will be used.

// JSON
var res GetValueResponse
resMeta, err := jsonClient.Call(
    yarpc.NewReqMetaOut(ctx).
        SetProcedure("getValue").
        SetHeaders(yarpc.NewHeaders().With("foo", "bar")),
    &GetValueRequest{...},
    &res,
)

// Thrift
resBody, resMeta, err := keyValueClient.Call(
    yarpc.NewReqMetaOut(ctx).
        SetHeaders(yarpc.NewHeaders().With("foo", "bar")),
    &GetValueRequest{...},
)

// Handler implementation in both JSON and Thrift will look similar
func GetValue(reqMeta yarpc.ReqMeta, body *GetValueRequest) (*GetValueResponse, yarpc.ResMeta, error) {
    res := doStuff(reqMeta.Caller(), reqMeta.Headers(), body)
    return res, yarpc.NewResMetaOut(reqMeta.Context()).SetHeaders(...), nil
}

Inbound Re{q,s}Meta objects are named just that rather than appending In to their names because these are part of the handler definitions that users write. The outbound versions will usually be constructed at the call site using the NewRe{q,s}MetaOut constructors so they can afford to have Out in their name.

We will not provide constructors for the inbound Re{q,s}Meta types because those will be implemented by the transport as-needed. We may offer an implementation for testing purposes in a yarpctest package, though.

---

Note: Some of this API may be revised based on what we need for backwards context propagation.

---

Thoughts? @yarpc/yarpc @shawnburke @yurishkuro

[HelloGrayson]

Starting to look really good.

• Big fan of fluid interface, love the context as the only required param to the constructor.
• Use reqMeta instead of meta for the param name in the handler, we want to be careful about abbreviating this term - others will adopt the same pattern and soon the term won't be clear in conversation.
• Let's consider renaming yarpc.NewReqMeta() and yarpc.NewResMeta() to just yarpc.ReqMeta() and yarpc.ResMeta(), respectively. (@prashantv can you weigh in here?). This bends the Go constructor naming idiom a bit, but these calls will be made so many times that I'd like to see calls like so:

resBody, resMeta, err := keyValueClient.Get(yarpc.ReqMeta(ctx), &GetValueRequest{...})

• Perhaps we should avoid splitting in & out meta objects initially. At the very least, the short form should go to ReqMetaOut, since these calls will far outweigh handlers. ReqMetaIn in the handler isn't terrible. I still think we should do this change later after living with single top-level req/res metas.

[abhinav]

• Updated to use reqMeta as the parameter name.
• Calling it ReqMeta will require choosing a different name for the interface. They're in the same namespace. I'm happy to do this if we're okay with users consuming yarpc.ReqMetaIn or similar in their handlers.

• Actually, the more I think about it, the more the splitting makes sense. It won't be a cheap change to perform later. Also I realized we can simplify the interface further with the split by removing the Set from the function names:

  type ReqMetaOut interface {
    Procedure(string) ReqMetaOut
    Headers(Headers) ReqMetaOut
  }
  
  type ReqMetaIn interface {
    Caller() string
    Context() context.Context
    Encoding() transport.Encoding
    Headers() Headers
    Procedure() string
    Service() string
  }

  With the other changes you requested, usage could become,

  var res GetValueResponse
  resMeta, err := jsonClient.Call(
    yarpc.ReqMeta(ctx).
        Procedure("getValue").
        Headers(yarpc.NewHeaders().With("foo", "bar")),
    &GetValueRequest{...},
    &res,
  )

[shawnburke]

<3