Add grpc.aio support #1245

cookiefission · 2022-08-25T07:41:24Z

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 · 2022-08-25T07:41:27Z

The committers listed above are authorized under a signed CLA.

✅ login: cookiefission / name: Sean Kenny (03e021b, ff4403c, a955218, 74c857c)

cookiefission · 2022-08-25T09:07:56Z

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.

This fixes assorted issues highlighted by CI, like unused imports, import ordering, "malformed" docstrings, etc

I missed a bit from previous merge conflict resolution..

cookiefission · 2022-09-27T09:09:35Z

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.

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 · 2022-10-13T13:04:26Z

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

cookiefission · 2022-10-24T14:09:14Z

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

aabmass

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 😃

aabmass · 2022-10-29T15:57:23Z

...tation/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/__init__.py

+    def instrumentation_dependencies(self) -> Collection[str]:
+        return _instruments
+
+    def _add_interceptors(self, tracer_provider, kwargs):


Can you add parameter types?

aabmass · 2022-10-29T15:58:22Z

...ion/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/_aio_client.py

+from opentelemetry.trace.status import Status, StatusCode
+
+
+def _unary_done_callback(span, code, details):


...ion/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/_aio_server.py

instrumentation/opentelemetry-instrumentation-grpc/tests/_aio_client.py

instrumentation/opentelemetry-instrumentation-grpc/tests/test_aio_client_interceptor.py

aabmass · 2022-10-29T16:05:10Z

instrumentation/opentelemetry-instrumentation-grpc/tests/test_aio_client_interceptor.py

+        return await continuation(client_call_details, request)
+
+
+@pytest.mark.asyncio


Is this needed?

@cookiefission any thoughts on this?

Add note about adding grpc.aio support

cookiefission · 2022-10-31T15:29:00Z

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.

aabmass

SGTM, thanks again for contributing!

srikanthccv · 2022-10-31T21:32:23Z

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.

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.

Add support for service-side grpc.aio

03e021b

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.

cookiefission force-pushed the grpc-aio-support branch from 6d70a51 to 13b87a7 Compare September 2, 2022 13:59

Add grpc.aio client-side support

ff4403c

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 force-pushed the grpc-aio-support branch from 13b87a7 to ff4403c Compare September 2, 2022 14:00

cookiefission marked this pull request as ready for review September 2, 2022 14:01

cookiefission requested a review from a team September 2, 2022 14:01

cookiefission force-pushed the grpc-aio-support branch 2 times, most recently from bbd8ee0 to 34daea3 Compare September 2, 2022 21:40

cookiefission added 2 commits September 2, 2022 22:47

Fix assorted CI issues

a955218

This fixes assorted issues highlighted by CI, like unused imports, import ordering, "malformed" docstrings, etc

Add changelog entry

74c857c

cookiefission force-pushed the grpc-aio-support branch from 34daea3 to 74c857c Compare September 2, 2022 21:47

girishc13 mentioned this pull request Sep 19, 2022

feat(instrumentation): add OpenTelemetry tracing and metrics with basic configurations jina-ai/serve#5175

Merged

5 tasks

cookiefission added 3 commits September 21, 2022 10:32

Merge branch 'main' into grpc-aio-support

8cd1c9a

Fix conflict properly

7802001

I missed a bit from previous merge conflict resolution..

Merge branch 'main' into grpc-aio-support

c64b884

cookiefission mentioned this pull request Sep 21, 2022

grpc.aio.ClientCallDetails method attribute typed as str but is bytes grpc/grpc#31092

Open

ocelotl and others added 11 commits September 27, 2022 12:14

Merge branch 'main' into grpc-aio-support

a42dd5d

Merge branch 'main' into grpc-aio-support

1098e64

Skip grpc.aio tests in Python 3.7

9cc91b4

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.

Update grpc.aio client interceptor to handle grpc bug

cf01d06

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

Add filter support to grpc.aio client interceptor

600ca83

Merge branch 'main' into grpc-aio-support

c4c2e86

Add basic server filtering test

6622774

Format code with black

db4f86e

Re-order imports with isort

5de1a4b

This _should_ allow CI to pass now..

Merge branch 'open-telemetry:main' into grpc-aio-support

ac546be

cookiefission added 3 commits October 12, 2022 14:46

Add additional tests for aio server filtering

4824822

Reformat code to pass linting

94ba2f7

Merge branch 'main' into grpc-aio-support

028a64e

cookiefission added 3 commits October 16, 2022 21:56

Merge branch 'main' into grpc-aio-support

c9f2a60

Merge branch 'main' into grpc-aio-support

0935e6a

Merge branch 'main' into grpc-aio-support

e1304ed

Merge branch 'main' into grpc-aio-support

e30a21b

aabmass self-assigned this Oct 26, 2022

Merge branch 'main' into grpc-aio-support

274a6f6

aabmass reviewed Oct 29, 2022

View reviewed changes

aabmass approved these changes Oct 29, 2022

View reviewed changes

Update CHANGELOG.md

fe3cb18

Add note about adding grpc.aio support

aabmass approved these changes Oct 31, 2022

View reviewed changes

srikanthccv added 2 commits November 1, 2022 01:33

Merge branch 'main' into grpc-aio-support

452360d

Merge branch 'main' into grpc-aio-support

5a6060d

srikanthccv enabled auto-merge (squash) October 31, 2022 21:32

srikanthccv merged commit f58d16b into open-telemetry:main Oct 31, 2022

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

Add grpc.aio support (open-telemetry#1245)

8c18c1f

saartochner-lumigo pushed a commit to lumigo-io/opentelemetry-python-contrib that referenced this pull request Nov 13, 2022

Add grpc.aio support (open-telemetry#1245)

1b65a3d

cookiefission mentioned this pull request Nov 15, 2022

Add grpc.aio instrumentation to be auto-instrumented #1442

Merged

10 tasks

girishc13 mentioned this pull request Nov 18, 2022

feat: add startupProbe and replace readiness probe with liveness probe jina-ai/serve#5407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add grpc.aio support #1245

Add grpc.aio support #1245

cookiefission commented Aug 25, 2022 •

edited

Loading

linux-foundation-easycla bot commented Aug 25, 2022 •

edited

Loading

cookiefission commented Aug 25, 2022 •

edited

Loading

cookiefission commented Sep 27, 2022

cookiefission commented Oct 13, 2022

cookiefission commented Oct 24, 2022

aabmass left a comment •

edited

Loading

aabmass Oct 29, 2022

aabmass Oct 29, 2022

aabmass Oct 29, 2022

aabmass Oct 31, 2022

cookiefission commented Oct 31, 2022

aabmass left a comment

srikanthccv commented Oct 31, 2022

		from opentelemetry.trace.status import Status, StatusCode


		def _unary_done_callback(span, code, details):

		return await continuation(client_call_details, request)


		@pytest.mark.asyncio

Add grpc.aio support #1245

Add grpc.aio support #1245

Conversation

cookiefission commented Aug 25, 2022 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

linux-foundation-easycla bot commented Aug 25, 2022 • edited Loading

cookiefission commented Aug 25, 2022 • edited Loading

cookiefission commented Sep 27, 2022

cookiefission commented Oct 13, 2022

cookiefission commented Oct 24, 2022

aabmass left a comment • edited Loading

Choose a reason for hiding this comment

aabmass Oct 29, 2022

Choose a reason for hiding this comment

aabmass Oct 29, 2022

Choose a reason for hiding this comment

aabmass Oct 29, 2022

Choose a reason for hiding this comment

aabmass Oct 31, 2022

Choose a reason for hiding this comment

cookiefission commented Oct 31, 2022

aabmass left a comment

Choose a reason for hiding this comment

srikanthccv commented Oct 31, 2022

cookiefission commented Aug 25, 2022 •

edited

Loading

linux-foundation-easycla bot commented Aug 25, 2022 •

edited

Loading

cookiefission commented Aug 25, 2022 •

edited

Loading

aabmass left a comment •

edited

Loading