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.

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

Add grpc.aio support #1245

Merged
merged 29 commits into from
Oct 31, 2022
Merged

Conversation

cookiefission
Copy link
Contributor

@cookiefission cookiefission commented Aug 25, 2022

Description

This adds support for grpc.aio interceptors/instrumentation. The vast majority of
the code is either re-used wholesale or duplicated with slight modifications
from the existing standard interceptors.

Fixes #1099

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

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

  • Unit tests in this repository
  • Using development version in real apps

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 25, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@cookiefission
Copy link
Contributor Author

cookiefission commented Aug 25, 2022

This is in draft while I wait for my company to OK the CLA, and while I add the client side support. The client should be done today but I've no idea how long the CLA will take.

This adds support for grpc.aio server interceptors. The vast majority of
the code is either re-used wholesale or duplicated with slight modifications
from the existing standard interceptors.
This adds support for instrumenting grpc.aio channels with spans and
telemetry. The instrumentation needed to work differently that the
standard grpc channel support but is functionally the same.
@cookiefission cookiefission marked this pull request as ready for review September 2, 2022 14:01
@cookiefission cookiefission requested a review from a team September 2, 2022 14:01
@cookiefission cookiefission force-pushed the grpc-aio-support branch 2 times, most recently from bbd8ee0 to 34daea3 Compare September 2, 2022 21:40
This fixes assorted issues highlighted by CI, like unused imports,
import ordering, "malformed" docstrings, etc
@cookiefission
Copy link
Contributor Author

I've not abandoned this; it just took so long for my company to sign the CLA that I've got other priorities at the moment.

I've not asked for a review yet because I need to fix the 3.7 tests and add the filtering functionality for these interceptors.

I'm aiming to get it ready again on Friday so I can start working through a review next week.

ocelotl and others added 11 commits September 27, 2022 12:14
unittest.IsolatedAsyncioTestCase was introduced in Python 3.8. It's use
simplifies the grpc.aio tests. Without it, the amount of test code
increases significantly, with most of the additional code handling
the asyncio set up.
This is the precursor for using the filter mechanisms with the aio
interceptors.

There's currently a bug in grpc python that means the the
ClientCallDetails.method field is populated with bytes instead of a
string. This code handles both cases so that it's forward-compatible
with a future fixed grpc version.
ClientCallDetails.method _should_ be a string here but due to a bug in
grpc, it is populated with a bytes object. Handle both cases such that we
are forward-compatible with a fixed version of grpc

More info: grpc/grpc#31092
This _should_ allow CI to pass now..
@cookiefission
Copy link
Contributor Author

@ocelotl @srikanthccv I think this is finally ready for review. Please could you take a look? 🙌

@cookiefission
Copy link
Contributor Author

@aabmass @lzchen are you able to review this, please?

@aabmass aabmass self-assigned this Oct 26, 2022
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 you for the PR, LGTM and has plenty of tests.

In the library code, can you add types if it's not too much trouble (don't worry about tests)? Things are not type checked here but it makes the code more readable imo. I realize the non-aio code is missing types too, so if it's too much hassle don't worry about it. Or feel free to make a follow up PR.

Can you replace the relative imports with absolute? nvm I see this was copied from the non-aio code. No need to change anything

This definitely seems worthy of a changelog entry which will make the Changelog check pass 😃

def instrumentation_dependencies(self) -> Collection[str]:
return _instruments

def _add_interceptors(self, tracer_provider, kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add parameter types?

from opentelemetry.trace.status import Status, StatusCode


def _unary_done_callback(span, code, details):
Copy link
Member

Choose a reason for hiding this comment

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

types?

return await continuation(client_call_details, request)


@pytest.mark.asyncio
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Member

Choose a reason for hiding this comment

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

@cookiefission any thoughts on this?

Add note about adding grpc.aio support
@cookiefission
Copy link
Contributor Author

Thanks for the review @aabmass , I've updated the changelog.

If it's all the same to you, I'll add types in a follow-up PR later this week. I've just been copy-pasting the interceptors to where they are needed so having a released version to use would be quicker.

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.

SGTM, thanks again for contributing!

@srikanthccv
Copy link
Member

If it's all the same to you, I'll add types in a follow-up PR later this week. I've just been copy-pasting the interceptors to where they are needed so having a released version to use would be quicker.

Ok, thanks for the contribution. I will merge this; please address the remaining comments in the follow-up PR.

@srikanthccv srikanthccv enabled auto-merge (squash) October 31, 2022 21:32
@srikanthccv srikanthccv merged commit f58d16b into open-telemetry:main Oct 31, 2022
@cookiefission cookiefission deleted the grpc-aio-support branch November 1, 2022 12:49
saartochner-lumigo pushed a commit to lumigo-io/opentelemetry-python-contrib that referenced this pull request Nov 13, 2022
saartochner-lumigo pushed a commit to lumigo-io/opentelemetry-python-contrib that referenced this pull request Nov 13, 2022
cookiefission added a commit to cookiefission/opentelemetry-python-contrib that referenced this pull request Nov 15, 2022
PR open-telemetry#1245 added grpc.aio instrumentation. It included the auto
instrumentor for grpc.aio but did not include the entry-points hook for
it to be run automatically on boot.

Currently, you need to manually include the instrumentor and call it.
This makes that automatic and in line with the plain grpc
instrumentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for gRPC aio
4 participants