-
Notifications
You must be signed in to change notification settings - Fork 417
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
Support keyword 'target' parameter when wrapping GRPC channels #946
Conversation
@asnr For testing, can you factorize the test function and call it once with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment about testing both scenarios.
@jd I've factorised one |
That's exactly what I meant, thanks @asnr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion, but otherwise looks good to me!
ddtrace/contrib/grpc/patch.py
Outdated
@@ -30,15 +30,25 @@ def unpatch(): | |||
|
|||
def _insecure_channel_with_interceptor(wrapped, instance, args, kwargs): | |||
channel = wrapped(*args, **kwargs) | |||
target = args[0] | |||
|
|||
if kwargs.get('target'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to do if 'target' in kwargs:
instead. In case kwargs['target']
is falsey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I'll make that change.
The
target
for a GRPC channel can be specified with a keyword argument instead of a positional argument in both the functiongrpc.insecure_channel
andgrpc.secure_channel
. See, for example, this usage in the official google pubsub client.Currently, the datadog patch only uses the positional argument to get the
target
parameter, which throws the following exception if the keyword argument is used by a client:For a simple example of client code that reproduces this error, see this gist.
Testing
I wasn't sure what the best way of testing this behaviour was. I settled on changing two existing tests to specify
target
using a keyword argument, but there might be a better way. I'd be happy to hear any suggestions.