Add gRPC filter #1241

ymotongpoo · 2022-08-24T08:12:09Z

Description

This pull request adds section to handle filter in client and server interceptors and provides pre-defined commonly used filters.

Fixes #373

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
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

tox -e py39-test-instrumentation-grpc (test_filters.py and test_xxx_filters.py)

Does This PR Require a Core Repo Change?

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

aabmass · 2022-08-25T13:11:25Z

Thanks for the PR! In the issue you mentioned environment variables, something like OTEL_PYTHON_FLASK_EXCLUDED_URLS but for gRPC. These are nice for auto instrumentation. Is the plan to implement similar environment variables on top of these predicates? Is there an easy way to specify the filters for auto instrumentation?

ymotongpoo · 2022-08-25T23:21:19Z

@aabmass thanks for the comment! though I tried to look up the implementation for OTEL_PYTHON_FLASK_EXCLUDED_URLS, I couldn't find the point where it is implemented. Could you tell the pointer where the environment variable is handled?

ymotongpoo · 2022-08-26T00:44:41Z

Ok, now I understand this part is referencing the environment variable via get_excluded_urls. I think this function introduces unnecessary complexity into the repo, tho.

ymotongpoo · 2022-08-26T09:02:17Z

@aabmass ok, I added the support in the global interceptor to exclude requests to services specified in OTEL_PYTHON_GRPC_EXCLUDED_SERVICES in comma separated format.

aabmass · 2022-09-08T16:58:39Z

Tox is the right way. They are probably flakes or maybe an already known issue. I'll review this PR :)

aabmass

Thanks for the PR, sorry for all the comments

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

...pentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/filters/__init__.py

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

…emetry/instrumentation/grpc/__init__.py Co-authored-by: Aaron Abbott <[email protected]>

…emetry/instrumentation/grpc/filters/__init__.py Co-authored-by: Aaron Abbott <[email protected]>

ymotongpoo

adopted the suggestions and replied to the questions

...pentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/filters/__init__.py

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

ymotongpoo · 2022-09-14T04:54:42Z

I updated the option name from filters to filter_. The tests kicked by GitHub Action are flaky and failing with arbitrary unexpected errors.

aabmass

LGTM!

...pentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/filters/__init__.py

aabmass · 2022-09-15T21:59:11Z

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

+class _HandlerCallDetails(
+    collections.namedtuple(
+        "_HanlderCallDetails",
+        (
+            "method",
+            "invocation_metadata",
+        ),
+    ),
+    grpc.HandlerCallDetails,
+):
+    pass
+
+
+class _UnaryClientInfo(
+    collections.namedtuple("_UnaryClientInfo", ("full_method", "timeout"))
+):
+    pass
+
+
+class _StreamClientInfo(
+    collections.namedtuple(
+        "_StreamClientInfo",
+        ("full_method", "is_client_stream", "is_server_stream", "timeout"),
+    )
+):
+    pass


nit I would prefer @dataclass over named tuple

Either collections.namedtuple or @dataclass is fine. I aligned the implementation here with the one in grpcext.

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/grpcext/_interceptor.py#L28-L40

Yoshi Yamaguchi added 5 commits August 24, 2022 09:46

fix typo in example codes

8bb3be5

add initial implementation of filters

c960d22

Merge branch 'main' into grpc-filter

b2e1546

add docstring to filter functions

380d63e

update changelog to mention this pull request

059b076

ymotongpoo mentioned this pull request Aug 24, 2022

Filter out health check request in grpc #373

Closed

Yoshi Yamaguchi added 14 commits August 25, 2022 09:24

revert unnecessary change and fix typos in changelog

ead1f61

Merge branch 'open-telemetry:main' into grpc-filter

9e20d21

ran tox -e generate

d1aee36

add filter option to auto instrumentors

782213f

add filter tests and fix filters

a66b21f

ran tox -e generate

d7a1588

remove unused package

48baf11

rename to meet lint conditions

840f152

fix variable names to meet lint criteria

9825288

Merge branch 'main' into grpc-filter

45c94a2

fix filter propagation and add tests

de75b24

fix lint

e23d77d

add composition filters and tests

98a84f9

add comments about default filter value and its behavior

7aa93c8

ymotongpoo marked this pull request as ready for review August 25, 2022 12:09

ymotongpoo requested a review from a team August 25, 2022 12:09

fix function names

97faf49

Merge branch 'main' into grpc-filter

bb6fc2f

add OTEL_PYTHON_GRPC_EXCLUDED_SERVICES support in global instrumentor

49ab02e

fix lint

3e058cb

aabmass self-assigned this Sep 8, 2022

aabmass reviewed Sep 8, 2022

View reviewed changes

Yoshi Yamaguchi and others added 13 commits September 9, 2022 09:07

Update instrumentation/opentelemetry-instrumentation-grpc/src/opentel…

f7de43f

…emetry/instrumentation/grpc/__init__.py Co-authored-by: Aaron Abbott <[email protected]>

Update instrumentation/opentelemetry-instrumentation-grpc/src/opentel…

8f80844

…emetry/instrumentation/grpc/__init__.py Co-authored-by: Aaron Abbott <[email protected]>

Update instrumentation/opentelemetry-instrumentation-grpc/src/opentel…

b064a13

…emetry/instrumentation/grpc/__init__.py Co-authored-by: Aaron Abbott <[email protected]>

Update instrumentation/opentelemetry-instrumentation-grpc/src/opentel…

c2b62d1

…emetry/instrumentation/grpc/__init__.py Co-authored-by: Aaron Abbott <[email protected]>

Update instrumentation/opentelemetry-instrumentation-grpc/src/opentel…

3a5466e

…emetry/instrumentation/grpc/filters/__init__.py Co-authored-by: Aaron Abbott <[email protected]>

Update instrumentation/opentelemetry-instrumentation-grpc/src/opentel…

5660969

…emetry/instrumentation/grpc/filters/__init__.py Co-authored-by: Aaron Abbott <[email protected]>

Update instrumentation/opentelemetry-instrumentation-grpc/src/opentel…

d72156b

…emetry/instrumentation/grpc/filters/__init__.py Co-authored-by: Aaron Abbott <[email protected]>

Merge branch 'main' into grpc-filter

c625012

fix comments, function name and add type annotations

39bc530

tox -e lint and generate

91649ba

fix filter condition to handle both environment variables and options

fc9b0e6

tox -e generate

21c287c

tox -e lint

55a17f9

ymotongpoo commented Sep 9, 2022

View reviewed changes

srikanthccv and others added 4 commits September 10, 2022 00:34

Merge branch 'main' into grpc-filter

c298fc1

Merge branch 'main' into grpc-filter

b4d6386

Merge branch 'main' into grpc-filter

fa42135

change option name from filters to filter_

4ad8547

Merge branch 'main' into grpc-filter

ff06ac3

ymotongpoo mentioned this pull request Sep 15, 2022

tox -e generate fails #1334

Closed

Merge branch 'main' into grpc-filter

ebbf6ef

aabmass approved these changes Sep 15, 2022

View reviewed changes

Yoshi Yamaguchi added 3 commits September 20, 2022 14:26

Merge branch 'main' into grpc-filter

9d89b75

fix filters' type annotations

3dd81ce

lint: remove unused class

ddbf0bd

lzchen merged commit 2ce0b69 into open-telemetry:main Sep 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gRPC filter #1241

Add gRPC filter #1241

ymotongpoo commented Aug 24, 2022 •

edited

Loading

aabmass commented Aug 25, 2022

ymotongpoo commented Aug 25, 2022 •

edited

Loading

ymotongpoo commented Aug 26, 2022

ymotongpoo commented Aug 26, 2022

aabmass commented Sep 8, 2022

aabmass left a comment

ymotongpoo left a comment

ymotongpoo commented Sep 14, 2022

aabmass left a comment

aabmass Sep 15, 2022

ymotongpoo Sep 20, 2022

Add gRPC filter #1241

Add gRPC filter #1241

Conversation

ymotongpoo commented Aug 24, 2022 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

aabmass commented Aug 25, 2022

ymotongpoo commented Aug 25, 2022 • edited Loading

ymotongpoo commented Aug 26, 2022

ymotongpoo commented Aug 26, 2022

aabmass commented Sep 8, 2022

aabmass left a comment

Choose a reason for hiding this comment

ymotongpoo left a comment

Choose a reason for hiding this comment

ymotongpoo commented Sep 14, 2022

aabmass left a comment

Choose a reason for hiding this comment

aabmass Sep 15, 2022

Choose a reason for hiding this comment

ymotongpoo Sep 20, 2022

Choose a reason for hiding this comment

ymotongpoo commented Aug 24, 2022 •

edited

Loading

ymotongpoo commented Aug 25, 2022 •

edited

Loading