-
Notifications
You must be signed in to change notification settings - Fork 61
feat: add basic interceptor to client #1206
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
Conversation
chalmerlowe
left a comment
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.
I am approving, pending:
- See the #QUESTIONs and determine whether any changes need to be made, if no, then carry on.
- See the #PREFERENCEs and #NITs and see if there is merit to including them. If no, then carry on.
| row_key = b"bulk_mutate" | ||
| new_value = uuid.uuid4().hex.encode() | ||
| row_key, mutation = self._create_row_and_mutation( | ||
| (row_key, mutation) = self._create_row_and_mutation( |
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.
#PREFERENCE
Why are we encapsulating all of these values in parens? To the best of my knowledge, this is not required by either PEP 8 or the Google Python Style Guide nor have I heard it referred to as a best practice. I don't see any other examples of this practice anywhere else in this codebase.
Feels unnecessary. I recommend ditching them unless we have a really good reason to add them.
| (row_key, mutation) = self._create_row_and_mutation( | |
| row_key, mutation = self._create_row_and_mutation( |
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.
This code auto-generated, so this wasn't a conscious choice. Something must have changed in how black is post processing the generated code.
It feels unnecessary to me too, but it doesn't bother me enough to fight with the generation system to change it back
chalmerlowe
left a comment
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.
LGTM
This PR adds a new grpc interceptor to the client, to be used for metrics collection. Currently, the interceptor is a no-op. Interception logic will be added in a future change.