tonic icon indicating copy to clipboard operation
tonic copied to clipboard

tonic: inject an GrpcMethod extension value to request

Open linw1995 opened this issue 2 years ago • 10 comments

Motivation

Resolve #772

Solution

Add an extension GrpcMethod to Request via the NamedService trait. And update tonic-build for making GrpcMethod as a static reference.

This only works on this case

Server::builder()
     .add_service(TestServer::with_interceptor(Svc, interceptor))
// ...

not for the case like

Server::builder()
     .layer(tonic::service::interceptor(interceptor))
     .add_service(TestServer::new())
// ...

linw1995 avatar Dec 17 '22 15:12 linw1995

Yes, we can find a better name than "GrpcMethod" and "grpc_method".

The "grpc_method" is applied for ensuring the "GrpcMethod" actually exists in the protobuf definition. And providing static lifetime variables instead of generating by the runtime, I believe will acquire more performance improvement.

And the extra constant variables are useful for references and preventing typo accidents.

Please correct me if something is incorrect. Thank you @LucioFranco

linw1995 avatar Dec 21 '22 13:12 linw1995

@linw1995 We have a need for something very similar to this for reporting prometheus request metrics. We need one small piece of additional information for us to be able to leverage this completely. We need the method type, which is one of four types.

Curious if you're interested in extending this a tad bit further to include that information on the GrpcMethod struct? Below is an example of what I'm referring to:

pub enum MethodType {
    Unary,
    ClientStream,
    ServerStream,
    BidiStream,
}

Adding that enum as a field on the GrpcMethod struct would make things significantly easier to get the information we need.

RyanAmos avatar Feb 02 '23 16:02 RyanAmos

@RyanAmos I am excited about including this in the future. But this pull request currently needs to be reviewed first, then I can put some effort into further work.

linw1995 avatar Feb 04 '23 04:02 linw1995

I think I am not convinced yet on the server side of this, but the client side I think would be useful. Could you break this PR into those two sections? I believe the client one should be pretty small and should be easy to merge.

LucioFranco avatar Feb 14 '23 14:02 LucioFranco

I think I am not convinced yet on the server side of this, but the client side I think would be useful. Could you break this PR into those two sections? I believe the client one should be pretty small and should be easy to merge.

@LucioFranco I see. I will split it into another PR for the client's implementation.

linw1995 avatar Feb 15 '23 12:02 linw1995

Hey guys is there any status on this? :-)

We can't use gRPC without the ability to log times on requests, it's essential for operations to see which routes are taking a long time.

ollyde avatar Dec 30 '23 19:12 ollyde

We can't use gRPC without the ability to log times on requests, it's essential for operations to see which routes are taking a long time.

I don't think there is much progress on this but you should be able to do what you want via the http service layer and tracking the paths which correlate to the grpc method being called.

LucioFranco avatar Jan 05 '24 16:01 LucioFranco

@LucioFranco thanks for the reply. We decided to go with another framework that supports it by default. Might be something to consider, this framework was fairly nice :-)

ollyde avatar Jan 05 '24 17:01 ollyde

@linw1995 You note that this change doesn't work for the case where the interceptor is made a layer over multiple services. Is there some way that could be done? I'm trying to use an interceptor layer for authentication, but I need it to not run on the health check service.

CodingAnarchy avatar Jun 27 '24 15:06 CodingAnarchy

@CodingAnarchy You might need to use the tonic-health crate in this PR. This will allow you to get the correct service name in your interceptor.

linw1995 avatar Jun 28 '24 07:06 linw1995