Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Keep client interceptors in sync with grpc client interceptors #442

Merged
merged 2 commits into from
May 7, 2021
Merged

Keep client interceptors in sync with grpc client interceptors #442

merged 2 commits into from
May 7, 2021

Conversation

mihirg
Copy link
Contributor

@mihirg mihirg commented Apr 14, 2021 •
edited
Loading

Description

Keep the opentelemetry interceptors in sync with grpc interceptors

Fixes # (issue)
#320
The discussion on this issue states a more serious refactoring is needed. This fix is to get people unblocked.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Yes. Updated test cases

Does This PR Require a Core Repo Change?

  • No.

Checklist:

  • Unit tests have been added

@mihirg mihirg requested review from a team, aabmass and srikanthccv and removed request for a team April 14, 2021 05:30
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 14, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@mihirg mihirg force-pushed the fix_client_interceptors branch 10 times, most recently from 759b52b to d427742 Compare April 16, 2021 03:10
@mihirg mihirg mentioned this pull request Apr 16, 2021
Closed
@mihirg
Copy link
Contributor Author

mihirg commented Apr 22, 2021

Can someone review this PR?

owais
owais approved these changes Apr 28, 2021
View reviewed changes
Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should use **kwargs instead?

@lzchen
Copy link
Contributor

lzchen commented May 3, 2021

@mihirg

What does the more "serious refactoring" entail? Would it make it so that we would not need these parameters anymore, which in that case, we should go with kwargs like @owais suggested?

ocelotl
ocelotl suggested changes May 3, 2021
View reviewed changes
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix @mihirg! Please remove these comments from instrumentation/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/grpcext/_interceptor.py b/instrumentation/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/grpcext/_interceptor.py

# pylint:disable=arguments-differ
# pylint:disable=signature-differs

Removing them should alert us of any further discrepancies between our implementation and the base classes 👍

@mihirg mihirg force-pushed the fix_client_interceptors branch 2 times, most recently from 8b4e15c to fb82ebd Compare May 4, 2021 07:10
@mihirg
Copy link
Contributor Author

mihirg commented May 4, 2021 •
edited
Loading

@mihirg

What does the more "serious refactoring" entail? Would it make it so that we would not need these parameters anymore, which in that case, we should go with kwargs like @owais suggested?
@owais / @lzchen
Done.

@mihirg
Copy link
Contributor Author

mihirg commented May 4, 2021 •
edited
Loading

Thanks for the fix @mihirg! Please remove these comments from instrumentation/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/grpcext/_interceptor.py b/instrumentation/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/grpcext/_interceptor.py

# pylint:disable=arguments-differ
# pylint:disable=signature-differs

Removing them should alert us of any further discrepancies between our implementation and the base classes 👍

@ocelotl Done.

@mihirg mihirg requested a review from ocelotl May 4, 2021 10:57
@lzchen lzchen requested a review from owais May 4, 2021 16:04
aabmass
aabmass requested changes May 4, 2021
View reviewed changes
instrumentation/opentelemetry-instrumentation-grpc/tests/test_client_interceptor.py Show resolved Hide resolved
...elemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/grpcext/_interceptor.py Outdated
@@ -41,41 +39,67 @@ class _StreamClientInfo(
):
pass

def _get_metadata_timeout(**kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being called as _get_metadata_timeout(kwargs) which would raise _get_metadata_timeout() takes 0 positional arguments but 1 was given.

Suggested change
def _get_metadata_timeout(**kwargs):
def _get_metadata_timeout(kwargs):

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless it is fine to return None as the value for metadata and/or timeout, then this should not use **kwargs. This looks like we should be using positional arguments as before instead of using **kwargs.

...elemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/grpcext/_interceptor.py Outdated Show resolved Hide resolved
ocelotl
ocelotl suggested changes May 5, 2021
View reviewed changes
...elemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/grpcext/_interceptor.py Outdated
@@ -41,41 +39,67 @@ class _StreamClientInfo(
):
pass

def _get_metadata_timeout(**kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless it is fine to return None as the value for metadata and/or timeout, then this should not use **kwargs. This looks like we should be using positional arguments as before instead of using **kwargs.

...elemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/grpcext/_interceptor.py Outdated Show resolved Hide resolved
@mihirg mihirg requested review from ocelotl and aabmass May 5, 2021 08:24
@mihirg
Copy link
Contributor Author

mihirg commented May 5, 2021

@ocelotl / @aabmass Incorporated your feedback.

ocelotl
ocelotl approved these changes May 5, 2021
View reviewed changes
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mihirg! Great first contribution 💪 😎

@ocelotl
Copy link
Contributor

ocelotl commented May 5, 2021

@mihirg apparently there are a few lint issues. No worries, the best way of getting them fixed is by doing this:

  1. Run tox -e lint (this should fail)
  2. From the root repo directory run source .tox/bin/lint/activate
  3. Run black . (this should fix any black-related issues)
  4. Run isort . (this should fix any isort-related issues)
  5. Run deactivate
  6. Run tox -e lint again

If it passes, then you are good to commit the fixes and push your branch again. If there are failures, they should be pylint failures, please take a look at the errors and either make a change for them or ignore them with # pylint:disable=<the-error-name-here>.

aabmass
aabmass approved these changes May 5, 2021
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

@mihirg
Copy link
Contributor Author

mihirg commented May 6, 2021

@ocelotl I have fixed the the linter issues.

@codeboten codeboten merged commit c12591e into open-telemetry:main May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabmass aabmass aabmass approved these changes

@ocelotl ocelotl ocelotl approved these changes

@lzchen lzchen lzchen approved these changes

@srikanthccv srikanthccv Awaiting requested review from srikanthccv

@owais owais Awaiting requested review from owais

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mihirg @lzchen @ocelotl @owais @aabmass @codeboten