Add support for method_attribute()

[snawaz]

As we know, prost_build::Config supports field_attribute() and type_attribute(). Since tower-grpc provides implementation of the trait prost_build::ServiceGenerator, it'd be really good to have method_attribute() on the generator, so that the generated methods can do things like initiating distributed tracing (zipkin):

#[zipkin_span]
pub fn client_api(&mut self, foo) -> bar {
}

And then the users of tower-grpc can implement their own zipkin_span or whatever they want for their cross-cutting concerns.

If the idea is good, then I can implement this feature and create the PR. Please review and let me know your thoughts.

[seanmonstar]

I don't quite follow. What would the user code look like?

[snawaz]

I don't quite follow. What would the user code look like?

By user code if you mean how would the generated method would eventually look like?

Let's consider this proto definition (taken from the examples):

// The greeting service definition.
service Greeter {
  // Sends a greeting
  rpc SayHello (HelloRequest) returns (HelloReply) {}
}

// The request message containing the user's name.
message HelloRequest {
  string name = 1;
}

// The response message containing the greetings
message HelloReply {
  string message = 1;
}

and from it, the client's method generated by tower-grpc would look something like this:

[zipkin_span]      // assume we add support for method_attribute
[arguments(info)]  // and this
fn say_hello(&mut self, req: Request<HelloRequest>) -> Response<HelloReply> {

     //body
}

which, depending on the implementation of zipkin_span and arguments, may eventually be converted into

fn say_hello(&mut self, req: Request<HelloRequest>) -> Response<HelloReply> {
     start_zipkin_span_for(req, etc);         // added this by macro!
     info("say_hello() called with: ", req);  // and this too

     //body
}

Does that make sense? There are many such possibilities.

Also, the support for method_attribute() can be added in the similar way as field_attribute() and type_attribute() exist in prost_build.

[seanmonstar]

And what's the proposed API here? What would a user write in their build.rs? (This seems fine so far!)

[snawaz]

And what's the proposed API here? What would a user write in their build.rs? (This seems fine so far!)

Here is what I think:

• tower_grpc_build::Config
  • It will contain a new field: method_attributes: HashMap<Path, Attribute>; pretty much like prost_build::Config does. The difference would be that Path could be an enum instead of String, to make it more expressive.
  • and a new method: pub fn method_attribute(&mut self, path: Path, attribute: Attribute) -> &mut Self
• tower_grpc_build::client::ServiceGenerator
  • It would use the method_attributes to generate the attributes for methods. Any conflict or redefinition would result in error (it'd be easier to detect the such error at this stage).

And the same can be done for tower_grpc_build::server:: ServiceGenerator as well.

As for how would users build.rs may look like, here is one example,

  let mut tower_config = tower_grpc_build::Config::from_prost(prost_config);
  tower_config.enable_client(true);

  tower_config.method_attribute(Path::MethodName("SayHello"), Attribute::new("zipkin_span"));
  tower_config.method_attribute(Path::All, Attribute::new("arguments(info)")); 
  tower_config.method_attribute(Path::MethodName("SayBye"), Attribute::new("latency"));

  //use tower_config as usual.

Path can be made more expressive, like applying certain attributes to certain Service only, and not to others. It's more like selector.

[seanmonstar]

Would love to hear from @carllerche and/or @per-gron as well.

[carllerche]

I think the general idea is good. I have not considered the details yet.

One question worth exploring is what functionality can be added via this strategy vs. modifying the tower Service that the client dispatches into. Obviously, using attributes on methods allows avoiding a conditional on each request to check the gRPC method, but is that useful in practice? I would think so... what are some specific examples of method specific transformations?

[snawaz]

@carllerche

One question worth exploring is what functionality can be added via this strategy vs. modifying the tower Service that the client dispatches into.

I think modifying the tower service wouldn't be flexible enough, as it doesn't probably allow plugging arbitrary behavior to the generated code. Also, think of cross-cutting concerns using the attribute-approach such as distributing tracing, regular loggings, measuring latency, call-counts, success-counts, failure-counts, other metrics generations, etc.

Obviously, using attributes on methods allows avoiding a conditional on each request to check the gRPC method, but is that useful in practice? I would think so... what are some specific examples of method specific transformations?

I've given few examples in the posts directed at @seanmonstar. Please have a look at them.

I'd also like to add that adding this feature doesn't harm tower-grpc-build in any way, as it'd not generate any attribute unless the users ask it to generate by specifying what-to-generate usingmethod_attribute(). So in that sense, it is safe.

[carllerche]

@snawaz i saw the examples, I just wasn't sure how common it would be to apply those annotations to only specific methods. Why not track latency and use zipkin for all methods?

Anyway, I'm generally fine w/ the proposed strategy.

[snawaz]

@carllerche

@snawaz i saw the examples, I just wasn't sure how common it would be to apply those annotations to only specific methods. Why not track latency and use zipkin for all methods?

Anyway, I'm generally fine w/ the proposed strategy.

I understand that most annotations would probably apply to all methods. However, since annotations allow arbitrary code to be plugged-in and it is not only about things which we can imagine like latency or zipkin. Maybe, users wouldn't want to use logging annotation (i.e arguments(info)) for methods which does authentication for security reasons? Or maybe, they want it differently logged?

I feel I shouldn't prevent users from using it for specific methods just because I cannot imagine such a scenario — especially when it's kinda opt-in feature. And at the same time, I don't want to make the implementation too complex, so I'll use the following selectors:

enum Path {
       MethodName(&'static str, &'static str),  // specific method of a specific service
       ServiceName(&'static str),               // all methods of a specific service
       All,                                     // all methods of all services
}

to keep things relatively simple.

If you like the idea, please assign this to me (I don't have the permission). I'll plan and start working on it.

[per-gron]

I think this seems like a cool idea. One question I have is if it isn't a bit too low level? This proposal seems to add two distinct features:

1. The ability to add middleware-like things to specific methods.
2. The ability to add Rust macros that can modify the actual implementation of methods.

My thoughts:

• It seems like (1) is a good thing to have for sure.
• I'm not so sure about the value of the attribute macro aspect of it. I agree that it will be able to change the code arbitrarily but what extra things will be possible to do in practice? Do you have an example of something that can not be implemented as tower middleware but that can be done with an attribute macro?
• With this API tower-grpc would end up with two separate kinds of middleware-like things: Tower middleware and attribute macros. Instead of doing it like this, would it be possible to add a build.rs level API that applies Tower middleware to specific methods? That seems like it would make a more unified API.

If there is value in the attribute macro aspect of this, is it possible to avoid the attribute vs middleware incompatibility by making something that can convert an attribute into a tower middleware, or is that impossible by definition?

Separate comment re your proposed Path enum: It seems like it could be made more type safe by replacing the &'static strs with generated enums. (Maybe this doesn't matter if it's all evaluated at build time anyways.)

[per-gron]

One possibly large benefit of applying middleware for individual methods vs how Tower middleware is currently used in tower-grpc is that it would allow the middleware function to be instantiated for the specific request and response types of a given method rather than some generic "prost message"/"blob of bytes" type.

It seems like this improvement is on its own nearly enough to implement the [zipkin_span] and [attribute(info)] examples above (the middleware would have to be given the grpc method name too). I like this because it fits nicely with the overall Tower is like Finagle narrative and seems to encourage more reusable, easier to write and easier to understand middleware code than if they were attribute macros.

[per-gron]

Another piece of feedback that is orthogonal to the other comments I've made:

I would personally prefer to keep this out of build.rs if possible, because

1. On a philosophical level, I think this kind of logic does not belong in the build system.
2. On a more practical level, I think this functionality would be more practical to use and more flexible if the method attributes were not locked in at build time. For example, what if you want to have a special implementation of a service for testing and you don't want zipkin there? Then you'd need to build the service definition twice which seems awkward.
3. Also: Because this functionality is so closely related to middleware I would prefer if the code to set it up was placed close to where middleware is set up.
4. build.rs does not fit well with build systems that actually care about hermetic builds so it would be painful to use this for users that use for example Bazel.

So I think it's worth exploring if it would be possible to make it so that the API for this is used where the services and middleware are set up. Not sure how to best do this... If it's not possible to do this with normal Rust generics this seems to me like a reasonable place to use a macro. Let me know if you want me to come up with a concrete example for how this could be done.

[per-gron]

A use case to consider:

Middleware/attribute methods that need to be instantiated with run-time state. For example:

• Lookaside load balancing/logging/tracing which needs to talk to an external service.
• Request quota enforcement which needs some kind of per RPC method or per service or per server state and possibly something to be reconfigurable at run-time.

I'm not sure that it is required to support this*, but to me it seems like this could be easier to implement in a middleware (because they are closures that can naturally contain state)/non-build.rs (because then the middleware can be instantiated in Rust code along with the other middleware and service instantiation code) style approach.

*: It's possible to make the per-method attributes add metadata to the request and have some global middleware do the stateful things but it seems like an awkward approach to me.