New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gRPC Python Client and Server Interceptors #12778

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@mehrdada
Member

mehrdada commented Oct 2, 2017

This PR adds machinery to intercept RPCs on client and server sides to gRPC Python.
gRFC grpc/proposal#39

Includes parts of the contribution by @rnburn in pull request #10256:

commit c54afc766cd5bd9102b4aeae8cf90a41a7c8d0be
Author: rnburn <ryan.burn@gmail.com>
Date:   Fri Mar 24 22:02:44 2017 -0400

mehrdada added some commits Sep 29, 2017

Add client interceptors to gRPC Python
Implements the client-side interceptor machinery for gRPC Python.
Add server interceptors to gRPC Python
Implements the service-side interceptor machinery for gRPC Python.
@mehrdada

This comment has been minimized.

Show comment
Hide comment
@mehrdada

mehrdada Oct 2, 2017

Member

@nathanielmanistaatgoogle What do you think about having this as a separate grpc_interceptors package as opposed to being part of the core gRPC package?

Member

mehrdada commented Oct 2, 2017

@nathanielmanistaatgoogle What do you think about having this as a separate grpc_interceptors package as opposed to being part of the core gRPC package?

@tedsuo

This comment has been minimized.

Show comment
Hide comment
@tedsuo

tedsuo Oct 2, 2017

Glad to see this @mehrdada!

tedsuo commented Oct 2, 2017

Glad to see this @mehrdada!

and returns a response value.
method: The full method name of the RPC.
request: The request value for the RPC.
servicer_context: The context of the current RPC.

This comment has been minimized.

@amitsaha

amitsaha Oct 10, 2017

I have been playing with the current implementation. Is servicer_context supposed to be an instance of grpc._server._Context (which subclasses ServicerContext) and not ServicerContext?

Context: I wanted to get the method_name, and i could only do so via servicer_context._rpc_event.request_call_details.method)

@amitsaha

amitsaha Oct 10, 2017

I have been playing with the current implementation. Is servicer_context supposed to be an instance of grpc._server._Context (which subclasses ServicerContext) and not ServicerContext?

Context: I wanted to get the method_name, and i could only do so via servicer_context._rpc_event.request_call_details.method)

This comment has been minimized.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Oct 12, 2017

Member

servicer_context is supposed to be an instance of grpc.ServicerContext. We wouldn't expose in our API (even an experimental API) an internal class like grpc._server._Context.

Why isn't the method parameter passed to this method the right way to get the method name?

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Oct 12, 2017

Member

servicer_context is supposed to be an instance of grpc.ServicerContext. We wouldn't expose in our API (even an experimental API) an internal class like grpc._server._Context.

Why isn't the method parameter passed to this method the right way to get the method name?

This comment has been minimized.

@mehrdada

mehrdada Oct 12, 2017

Member

Sorry I originally missed getting back to this comment notification. Definitely we don't want exposure of the private API. I suspect the issue here might be that the method name would be full HTTP path e.g. /Greeter/Hello and the desired part would be Hello? The reality is the gRPC server object does not want to couple itself with any specific form of handler, and a GenericRpcHandler can choose to service the RPC however it wants. In other words, there is no need for a one to one mapping to exist between RPC method names (as given to the interceptor) and methods on the Python servicer object. I think the best thing would be to parse the method name out of full method name string.

@mehrdada

mehrdada Oct 12, 2017

Member

Sorry I originally missed getting back to this comment notification. Definitely we don't want exposure of the private API. I suspect the issue here might be that the method name would be full HTTP path e.g. /Greeter/Hello and the desired part would be Hello? The reality is the gRPC server object does not want to couple itself with any specific form of handler, and a GenericRpcHandler can choose to service the RPC however it wants. In other words, there is no need for a one to one mapping to exist between RPC method names (as given to the interceptor) and methods on the Python servicer object. I think the best thing would be to parse the method name out of full method name string.

@nathanielmanistaatgoogle

I like a lot of what's here, but I also find myself asking "well this one part could be this way but it could also be some other way". Is there a (have you written a) language-agnostic specification for interceptors? Something that specifies, regardless of what gRPC langauge is being used, what interceptors must afford to applications?

return channel.stream_stream(_STREAM_STREAM)
class _Interceptor(

This comment has been minimized.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Oct 12, 2017

Member

This object should do something "interesting" with the RPCs and RPC values that it intercepts. Double the value of an integer in the request payload, or insert metadata, or duplicate each message in the request stream, or triplicate each message in the response stream, or... all those things!

Consider an implementation that calls the interceptor, but after calling the interceptor fails to make use of the RPC values returned by the interceptor and instead continued executing the RPC with the RPC values that it passed to the interceptor. This test, in its current form, would fail to detect that defect, right?

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Oct 12, 2017

Member

This object should do something "interesting" with the RPCs and RPC values that it intercepts. Double the value of an integer in the request payload, or insert metadata, or duplicate each message in the request stream, or triplicate each message in the response stream, or... all those things!

Consider an implementation that calls the interceptor, but after calling the interceptor fails to make use of the RPC values returned by the interceptor and instead continued executing the RPC with the RPC values that it passed to the interceptor. This test, in its current form, would fail to detect that defect, right?

This comment has been minimized.

@mehrdada

mehrdada Oct 12, 2017

Member

Agreed. Will add a few tests and a couple examples as well.

@mehrdada

mehrdada Oct 12, 2017

Member

Agreed. Will add a few tests and a couple examples as well.

@mehrdada

This comment has been minimized.

Show comment
Hide comment
@mehrdada

mehrdada Oct 12, 2017

Member

@nathanielmanistaatgoogle There is no cross language spec, as each language has slightly different idiosyncrasies, but the C# and Ruby proposals on the proposal repo (and the Go implementation beforehand) are closely related and comparable in design. I am curious to hear about an alternate designs (in broad sense) or refinements on this that might be sensible.

Member

mehrdada commented Oct 12, 2017

@nathanielmanistaatgoogle There is no cross language spec, as each language has slightly different idiosyncrasies, but the C# and Ruby proposals on the proposal repo (and the Go implementation beforehand) are closely related and comparable in design. I am curious to hear about an alternate designs (in broad sense) or refinements on this that might be sensible.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Oct 17, 2017

Member

@mehrdada: I think what I might have in mind something bigger than a section of this page, but not as large a single page unto itself as this page (although, possibly?).

Right now I feel like I'm looking at code that does some things, and it... does those things, but I feel like I want to look outside the code change for the answer to "well, what should it do?".

Member

nathanielmanistaatgoogle commented Oct 17, 2017

@mehrdada: I think what I might have in mind something bigger than a section of this page, but not as large a single page unto itself as this page (although, possibly?).

Right now I feel like I'm looking at code that does some things, and it... does those things, but I feel like I want to look outside the code change for the answer to "well, what should it do?".

@wkiser wkiser referenced this pull request Nov 4, 2017

Merged

Add support for Python. #32

@mehrdada

This comment has been minimized.

Show comment
Hide comment
@mehrdada

mehrdada Dec 12, 2017

Member

Closing in favor of new PR on 1.8.x branch: #13722

Member

mehrdada commented Dec 12, 2017

Closing in favor of new PR on 1.8.x branch: #13722

@mehrdada mehrdada closed this Dec 12, 2017

@jtattermusch jtattermusch added area/api and removed area/api labels Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment