From 160f52168a9d78c1ed61b5b7bb4a71b59566988b Mon Sep 17 00:00:00 2001 From: "Tahir H. Butt" Date: Thu, 29 Apr 2021 20:31:14 -0400 Subject: [PATCH 1/5] fix(grpc): add snapshot test for method service metadata --- .circleci/config.yml | 3 +- tests/contrib/grpc/test_grpc.py | 39 +++++++++ ...ib.grpc.test_grpc.test_method_service.snap | 84 +++++++++++++++++++ 3 files changed, 125 insertions(+), 1 deletion(-) create mode 100644 tests/snapshots/tests.contrib.grpc.test_grpc.test_method_service.snap diff --git a/.circleci/config.yml b/.circleci/config.yml index 7a71f405a70..936a9d85951 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -622,11 +622,12 @@ jobs: pattern: '^httplib_contrib' grpc: - <<: *contrib_job + <<: *machine_executor parallelism: 6 steps: - run_test: pattern: "grpc" + snapshot: true molten: <<: *contrib_job diff --git a/tests/contrib/grpc/test_grpc.py b/tests/contrib/grpc/test_grpc.py index ccb77a72fcc..9fb991c2b07 100644 --- a/tests/contrib/grpc/test_grpc.py +++ b/tests/contrib/grpc/test_grpc.py @@ -3,6 +3,7 @@ import grpc from grpc._grpcio_metadata import __version__ as _GRPC_VERSION from grpc.framework.foundation import logging_pool +import pytest from ddtrace import Pin from ddtrace.constants import ANALYTICS_SAMPLE_RATE_KEY @@ -12,6 +13,7 @@ from ddtrace.contrib.grpc.patch import _unpatch_server from ddtrace.ext import errors from tests.utils import TracerTestCase +from tests.utils import snapshot from .hello_pb2 import HelloReply from .hello_pb2 import HelloRequest @@ -572,3 +574,40 @@ class NotFutureLike(object): assert span.duration is None _handle_response(span, FutureLike()) assert span.duration is not None + + +@pytest.fixture() +def patch_grpc(): + patch() + try: + yield + finally: + unpatch() + + +class _UnaryUnaryRpcHandler(grpc.GenericRpcHandler): + def __init__(self, handler): + self._handler = handler + + def service(self, handler_call_details): + return grpc.unary_unary_rpc_method_handler(self._handler) + + +@snapshot(ignores=["meta.grpc.port"], async_mode=False) +def test_method_service(patch_grpc): + def handler(request, context): + return b"" + + server = grpc.server( + logging_pool.pool(1), + options=(("grpc.so_reuseport", 0),), + ) + port = server.add_insecure_port("[::]:0") + channel = grpc.insecure_channel("[::]:{}".format(port)) + server.add_generic_rpc_handlers((_UnaryUnaryRpcHandler(handler),)) + try: + server.start() + channel.unary_unary("/Servicer/Handler")(b"request") + channel.unary_unary("/pkg.Servicer/Handler")(b"request") + finally: + server.stop(None) diff --git a/tests/snapshots/tests.contrib.grpc.test_grpc.test_method_service.snap b/tests/snapshots/tests.contrib.grpc.test_grpc.test_method_service.snap new file mode 100644 index 00000000000..34d8796b09a --- /dev/null +++ b/tests/snapshots/tests.contrib.grpc.test_grpc.test_method_service.snap @@ -0,0 +1,84 @@ +[[{"name" "grpc" + "service" "grpc-client" + "resource" "/Servicer/Handler" + "type" "grpc" + "error" 0 + "span_id" 0 + "trace_id" 0 + "parent_id" nil + "start" 1619743374223086900 + "duration" 3639600 + "meta" {"runtime-id" "4f28f3520875432db74bf28ceb0e8c07" + "grpc.port" "56469" + "grpc.host" "[::]" + "grpc.method.kind" "unary" + "span.kind" "client" + "grpc.method.path" "/Servicer/Handler" + "grpc.status.code" "StatusCode.OK" + "grpc.method.service" "Servicer" + "grpc.method.name" "Handler"} + "metrics" {"_dd.agent_psr" 1.0 + "_dd.measured" 1 + "_sampling_priority_v1" 1 + "system.pid" 516 + "_dd.tracer_kr" 1.0}} + {"name" "grpc" + "service" "grpc-server" + "resource" "/Servicer/Handler" + "type" "grpc" + "error" 0 + "span_id" 1 + "trace_id" 0 + "parent_id" 0 + "start" 1619743374225115700 + "duration" 332200 + "meta" {"grpc.method.path" "/Servicer/Handler" + "grpc.method.service" "Servicer" + "grpc.method.name" "Handler" + "grpc.method.kind" "unary" + "span.kind" "server"} + "metrics" {"_dd.measured" 1 + "_sampling_priority_v1" 1}}] + [{"name" "grpc" + "service" "grpc-client" + "resource" "/pkg.Servicer/Handler" + "type" "grpc" + "error" 0 + "span_id" 0 + "trace_id" 1 + "parent_id" nil + "start" 1619743374227415300 + "duration" 2830000 + "meta" {"runtime-id" "4f28f3520875432db74bf28ceb0e8c07" + "grpc.port" "56469" + "grpc.method.package" "pkg" + "grpc.host" "[::]" + "grpc.method.kind" "unary" + "span.kind" "client" + "grpc.method.path" "/pkg.Servicer/Handler" + "grpc.status.code" "StatusCode.OK" + "grpc.method.service" "Servicer" + "grpc.method.name" "Handler"} + "metrics" {"_dd.agent_psr" 1.0 + "_dd.measured" 1 + "_sampling_priority_v1" 1 + "system.pid" 516 + "_dd.tracer_kr" 1.0}} + {"name" "grpc" + "service" "grpc-server" + "resource" "/pkg.Servicer/Handler" + "type" "grpc" + "error" 0 + "span_id" 1 + "trace_id" 1 + "parent_id" 0 + "start" 1619743374229595900 + "duration" 117800 + "meta" {"grpc.method.path" "/pkg.Servicer/Handler" + "grpc.method.package" "pkg" + "grpc.method.service" "Servicer" + "grpc.method.name" "Handler" + "grpc.method.kind" "unary" + "span.kind" "server"} + "metrics" {"_dd.measured" 1 + "_sampling_priority_v1" 1}}]] From a1395f4d22c554bb567550ab330e2b46649b50ea Mon Sep 17 00:00:00 2001 From: "Tahir H. Butt" Date: Thu, 29 Apr 2021 21:01:55 -0400 Subject: [PATCH 2/5] handle empty method package --- ddtrace/contrib/grpc/utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ddtrace/contrib/grpc/utils.py b/ddtrace/contrib/grpc/utils.py index 5c88f3dfb28..0f39d1963ba 100644 --- a/ddtrace/contrib/grpc/utils.py +++ b/ddtrace/contrib/grpc/utils.py @@ -19,7 +19,8 @@ def set_grpc_method_meta(span, method, method_kind): method_path = method method_package, method_service, method_name = parse_method_path(method_path) span._set_str_tag(constants.GRPC_METHOD_PATH_KEY, method_path) - span._set_str_tag(constants.GRPC_METHOD_PACKAGE_KEY, method_package) + if method_package is not None: + span._set_str_tag(constants.GRPC_METHOD_PACKAGE_KEY, method_package) span._set_str_tag(constants.GRPC_METHOD_SERVICE_KEY, method_service) span._set_str_tag(constants.GRPC_METHOD_NAME_KEY, method_name) span._set_str_tag(constants.GRPC_METHOD_KIND_KEY, method_kind) From cc4c348b4a3c5084769499fc7670004ba9ff066f Mon Sep 17 00:00:00 2001 From: "Tahir H. Butt" Date: Thu, 29 Apr 2021 21:07:14 -0400 Subject: [PATCH 3/5] revert change to host parsing --- tests/contrib/grpc/test_grpc.py | 2 +- ...ib.grpc.test_grpc.test_method_service.snap | 32 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/contrib/grpc/test_grpc.py b/tests/contrib/grpc/test_grpc.py index 9fb991c2b07..9d87c40552b 100644 --- a/tests/contrib/grpc/test_grpc.py +++ b/tests/contrib/grpc/test_grpc.py @@ -603,7 +603,7 @@ def handler(request, context): options=(("grpc.so_reuseport", 0),), ) port = server.add_insecure_port("[::]:0") - channel = grpc.insecure_channel("[::]:{}".format(port)) + channel = grpc.insecure_channel("localhost:{}".format(port)) server.add_generic_rpc_handlers((_UnaryUnaryRpcHandler(handler),)) try: server.start() diff --git a/tests/snapshots/tests.contrib.grpc.test_grpc.test_method_service.snap b/tests/snapshots/tests.contrib.grpc.test_grpc.test_method_service.snap index 34d8796b09a..2a29c442629 100644 --- a/tests/snapshots/tests.contrib.grpc.test_grpc.test_method_service.snap +++ b/tests/snapshots/tests.contrib.grpc.test_grpc.test_method_service.snap @@ -6,11 +6,11 @@ "span_id" 0 "trace_id" 0 "parent_id" nil - "start" 1619743374223086900 - "duration" 3639600 - "meta" {"runtime-id" "4f28f3520875432db74bf28ceb0e8c07" - "grpc.port" "56469" - "grpc.host" "[::]" + "start" 1619744633550844100 + "duration" 1911900 + "meta" {"runtime-id" "04d67a3bd550461295cfd98eedc69d99" + "grpc.port" "55945" + "grpc.host" "localhost" "grpc.method.kind" "unary" "span.kind" "client" "grpc.method.path" "/Servicer/Handler" @@ -20,7 +20,7 @@ "metrics" {"_dd.agent_psr" 1.0 "_dd.measured" 1 "_sampling_priority_v1" 1 - "system.pid" 516 + "system.pid" 714 "_dd.tracer_kr" 1.0}} {"name" "grpc" "service" "grpc-server" @@ -30,8 +30,8 @@ "span_id" 1 "trace_id" 0 "parent_id" 0 - "start" 1619743374225115700 - "duration" 332200 + "start" 1619744633552296900 + "duration" 48200 "meta" {"grpc.method.path" "/Servicer/Handler" "grpc.method.service" "Servicer" "grpc.method.name" "Handler" @@ -47,12 +47,12 @@ "span_id" 0 "trace_id" 1 "parent_id" nil - "start" 1619743374227415300 - "duration" 2830000 - "meta" {"runtime-id" "4f28f3520875432db74bf28ceb0e8c07" - "grpc.port" "56469" + "start" 1619744633553123900 + "duration" 1079700 + "meta" {"runtime-id" "04d67a3bd550461295cfd98eedc69d99" + "grpc.port" "55945" "grpc.method.package" "pkg" - "grpc.host" "[::]" + "grpc.host" "localhost" "grpc.method.kind" "unary" "span.kind" "client" "grpc.method.path" "/pkg.Servicer/Handler" @@ -62,7 +62,7 @@ "metrics" {"_dd.agent_psr" 1.0 "_dd.measured" 1 "_sampling_priority_v1" 1 - "system.pid" 516 + "system.pid" 714 "_dd.tracer_kr" 1.0}} {"name" "grpc" "service" "grpc-server" @@ -72,8 +72,8 @@ "span_id" 1 "trace_id" 1 "parent_id" 0 - "start" 1619743374229595900 - "duration" 117800 + "start" 1619744633553812700 + "duration" 42900 "meta" {"grpc.method.path" "/pkg.Servicer/Handler" "grpc.method.package" "pkg" "grpc.method.service" "Servicer" From 5fe2dd22f0b7860068a1c0a398a71933d4a2eef6 Mon Sep 17 00:00:00 2001 From: "Tahir H. Butt" Date: Thu, 29 Apr 2021 21:37:11 -0400 Subject: [PATCH 4/5] add release note --- releasenotes/notes/fix-grpc-method-93038a476c9e95aa.yaml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 releasenotes/notes/fix-grpc-method-93038a476c9e95aa.yaml diff --git a/releasenotes/notes/fix-grpc-method-93038a476c9e95aa.yaml b/releasenotes/notes/fix-grpc-method-93038a476c9e95aa.yaml new file mode 100644 index 00000000000..d14291ecea0 --- /dev/null +++ b/releasenotes/notes/fix-grpc-method-93038a476c9e95aa.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + grpc: handle no package in fully qualified method From 680a4c6ef6381d60f449474b9b265841c9105ab0 Mon Sep 17 00:00:00 2001 From: "Tahir H. Butt" Date: Thu, 29 Apr 2021 23:54:16 -0400 Subject: [PATCH 5/5] use async_mode --- tests/contrib/grpc/test_grpc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/contrib/grpc/test_grpc.py b/tests/contrib/grpc/test_grpc.py index 9d87c40552b..41922223e28 100644 --- a/tests/contrib/grpc/test_grpc.py +++ b/tests/contrib/grpc/test_grpc.py @@ -593,7 +593,7 @@ def service(self, handler_call_details): return grpc.unary_unary_rpc_method_handler(self._handler) -@snapshot(ignores=["meta.grpc.port"], async_mode=False) +@snapshot(ignores=["meta.grpc.port"]) def test_method_service(patch_grpc): def handler(request, context): return b""