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

Adding gRPC instrumentor #788

Merged
merged 36 commits into from
Jul 22, 2020
Merged

Adding gRPC instrumentor #788

merged 36 commits into from
Jul 22, 2020

Conversation

aravinsiva
Copy link
Contributor

@aravinsiva aravinsiva commented Jun 8, 2020 •
edited by codeboten
Loading

fixes #661

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 8, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

@aravinsiva aravinsiva marked this pull request as ready for review June 16, 2020 01:08
@aravinsiva aravinsiva requested a review from a team June 16, 2020 01:08
@aravinsiva aravinsiva changed the title Adding gRPC instrumentor [WIP] Adding gRPC instrumentor Jun 16, 2020
@aravinsiva aravinsiva changed the title Adding gRPC instrumentor Adding gRPC instrumentor [WIP] Jun 16, 2020
@aravinsiva aravinsiva changed the title Adding gRPC instrumentor [WIP] Adding gRPC instrumentor Jun 17, 2020
cnnradams
cnnradams suggested changes Jun 17, 2020
View reviewed changes
Copy link
Member

@cnnradams cnnradams left a comment

Choose a reason for hiding this comment

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

Looks like good progress from a first pass, but I think there is issues with auto-instrumentation wrt using _instrument to return the server/client instance instead of setting up a wrapper instead

ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-grpc/tests/test_server_interceptor.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py Outdated Show resolved Hide resolved
cnnradams
cnnradams approved these changes Jun 19, 2020
View reviewed changes
Copy link
Member

@cnnradams cnnradams left a comment

Choose a reason for hiding this comment

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

LGTM other than needing to return server/client instead of setting self.server/self.client

ext/opentelemetry-ext-grpc/setup.cfg Outdated Show resolved Hide resolved
ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-grpc/tests/test_server_interceptor.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py Outdated Show resolved Hide resolved
@cnnradams cnnradams mentioned this pull request Jul 3, 2020
Closed
codeboten
codeboten suggested changes Jul 17, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

A couple of changes requested but overall it looks pretty good. Please update the CHANGELOG.md file as well.

ext/opentelemetry-ext-grpc/setup.cfg Outdated Show resolved Hide resolved
ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-grpc/tests/test_server_interceptor.py Outdated Show resolved Hide resolved
codeboten
codeboten suggested changes Jul 20, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Couple of changes requested, setup.cfg won't work as is, i added a suggestion. The test update would be nice but once the setup.cfg is updated, im happy to move to approve.

codeboten
codeboten approved these changes Jul 20, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten 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 all the updates.

lzchen
lzchen reviewed Jul 21, 2020
View reviewed changes
ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py
# pylint:disable=import-error
# pylint:disable=no-self-use
from contextlib import contextmanager

Copy link
Contributor

Choose a reason for hiding this comment

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

Some example code in the comments would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added examples for both client and server

@lzchen lzchen mentioned this pull request Jul 21, 2020
Merged
@cnnradams
Copy link
Member

can you update the grpc client tests (#896) to use this method instead of calling the interceptors themselves?

@lzchen lzchen merged commit 5ff9600 into open-telemetry:master Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjbrunst sjbrunst sjbrunst left review comments

@cnnradams cnnradams cnnradams approved these changes

@AndrewAXue AndrewAXue AndrewAXue left review comments

@codeboten codeboten codeboten approved these changes

@lzchen lzchen lzchen approved these changes

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

Successfully merging this pull request may close these issues.

ext/grpc: Implement BaseInstrumentor interface
6 participants
@aravinsiva @cnnradams @codeboten @sjbrunst @lzchen @AndrewAXue