-
Notifications
You must be signed in to change notification settings - Fork 624
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
Respect suppress instrumentation key in gRPC client #559
Respect suppress instrumentation key in gRPC client #559
Conversation
05889c6
to
b0e315f
Compare
@@ -122,6 +123,9 @@ def _start_guarded_span(self, *args, **kwargs): | |||
return _GuardedSpan(self._start_span(*args, **kwargs)) | |||
|
|||
def intercept_unary(self, request, metadata, client_info, invoker): | |||
if context.get_value(_SUPPRESS_INSTRUMENTATION_KEY): |
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.
Does this functionality need to be added to the server interceptor too?
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.
From all the implementations under contrib repo,
_SUPPRESS_INSTRUMENTATION_KEY
is mainly used under the client side instead of server side.
So, I did not implement the server side at first place.
My preference is first to make sure the client side works as expected.
Then, we can investigate the server side implementation later on.
I want to make each PR only containing a single feature.
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.
It should definitely be done for the server side as well if you get a chance 😃
@RyanSiu1995 do you mind commenting on #476 so I can assign it to you?
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.
@RyanSiu1995
Would you like to contribute that in a separate PR as well?
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.
Yes, I will!
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.
Thank you for the contribution!
@@ -122,6 +123,9 @@ def _start_guarded_span(self, *args, **kwargs): | |||
return _GuardedSpan(self._start_span(*args, **kwargs)) | |||
|
|||
def intercept_unary(self, request, metadata, client_info, invoker): | |||
if context.get_value(_SUPPRESS_INSTRUMENTATION_KEY): |
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.
It should definitely be done for the server side as well if you get a chance 😃
@RyanSiu1995 do you mind commenting on #476 so I can assign it to you?
Yes, sure! I will open up the change in server instrumentor really soon. |
@RyanSiu1995 please resolve the conflicts and we can get this merged, thanks! |
@codeboten Done and done! |
Description
Make gRPC client instrumentor respect
_SUPPRESS_INSTRUMENTATION_KEY
Fixes # (issue)
#476
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.