From 2fd49557fc83513e0d6e6a5ce71c094f0a5a6616 Mon Sep 17 00:00:00 2001 From: "Tahir H. Butt" Date: Mon, 3 May 2021 13:34:00 -0400 Subject: [PATCH 1/4] fix(grpc): parse target for ipv6 and missing port (#2298) * fix(grpc): add snapshot test for method service metadata * revert change to host parsing * fix(grpc): parse target for ipv6 and missing port Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit de4064098b3344ea781acfe6ee5c9498be69b571) # Conflicts: # ddtrace/contrib/grpc/client_interceptor.py # ddtrace/contrib/grpc/utils.py # tests/contrib/grpc/test_grpc.py # tests/snapshots/tests.contrib.grpc.test_grpc.test_method_service.snap --- ddtrace/contrib/grpc/client_interceptor.py | 7 ++ ddtrace/contrib/grpc/patch.py | 14 +--- ddtrace/contrib/grpc/utils.py | 50 +++++++++++ docs/spelling_wordlist.txt | 1 + .../fix-grpc-target-0a1daf38516be79f.yaml | 4 + tests/contrib/grpc/test_grpc.py | 62 ++++++++++++++ tests/contrib/grpc/test_grpc_utils.py | 28 +++++++ ...ib.grpc.test_grpc.test_method_service.snap | 84 +++++++++++++++++++ 8 files changed, 238 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/fix-grpc-target-0a1daf38516be79f.yaml create mode 100644 tests/snapshots/tests.contrib.grpc.test_grpc.test_method_service.snap diff --git a/ddtrace/contrib/grpc/client_interceptor.py b/ddtrace/contrib/grpc/client_interceptor.py index 2b9a6112402..b9f707bc639 100644 --- a/ddtrace/contrib/grpc/client_interceptor.py +++ b/ddtrace/contrib/grpc/client_interceptor.py @@ -172,6 +172,7 @@ def _intercept_client_call(self, method_kind, client_call_details): resource=client_call_details.method, ) +<<<<<<< HEAD # tags for method details method_path = client_call_details.method method_package, method_service, method_name = parse_method_path(method_path) @@ -182,6 +183,12 @@ def _intercept_client_call(self, method_kind, client_call_details): span._set_str_tag(constants.GRPC_METHOD_KIND_KEY, method_kind) span._set_str_tag(constants.GRPC_HOST_KEY, self._host) span._set_str_tag(constants.GRPC_PORT_KEY, self._port) +======= + set_grpc_method_meta(span, client_call_details.method, method_kind) + span._set_str_tag(constants.GRPC_HOST_KEY, self._host) + if self._port: + span._set_str_tag(constants.GRPC_PORT_KEY, str(self._port)) +>>>>>>> de406409... fix(grpc): parse target for ipv6 and missing port (#2298) span._set_str_tag(constants.GRPC_SPAN_KIND_KEY, constants.GRPC_SPAN_KIND_VALUE_CLIENT) sample_rate = config.grpc.get_analytics_sample_rate() diff --git a/ddtrace/contrib/grpc/patch.py b/ddtrace/contrib/grpc/patch.py index bae88fcc230..d39519a4f64 100644 --- a/ddtrace/contrib/grpc/patch.py +++ b/ddtrace/contrib/grpc/patch.py @@ -5,6 +5,7 @@ from ddtrace.vendor.wrapt import wrap_function_wrapper as _w from . import constants +from . import utils from ...utils.wrappers import unwrap as _u from .client_interceptor import create_client_interceptor from .client_interceptor import intercept_channel @@ -95,7 +96,7 @@ def _client_channel_interceptor(wrapped, instance, args, kwargs): if not pin or not pin.enabled(): return channel - (host, port) = _parse_target_from_arguments(args, kwargs) + (host, port) = utils._parse_target_from_args(args, kwargs) interceptor_function = create_client_interceptor(pin, host, port) return grpc.intercept_channel(channel, interceptor_function) @@ -118,14 +119,3 @@ def _server_constructor_interceptor(wrapped, instance, args, kwargs): kwargs["interceptors"] = (interceptor,) return wrapped(*args, **kwargs) - - -def _parse_target_from_arguments(args, kwargs): - if "target" in kwargs: - target = kwargs["target"] - else: - target = args[0] - - split = target.rsplit(":", 2) - - return (split[0], split[1] if len(split) > 1 else None) diff --git a/ddtrace/contrib/grpc/utils.py b/ddtrace/contrib/grpc/utils.py index 1804dce25ef..288af553856 100644 --- a/ddtrace/contrib/grpc/utils.py +++ b/ddtrace/contrib/grpc/utils.py @@ -1,3 +1,16 @@ +<<<<<<< HEAD +======= +import logging + +from ddtrace.internal.compat import parse + +from . import constants + + +log = logging.getLogger(__name__) + + +>>>>>>> de406409... fix(grpc): parse target for ipv6 and missing port (#2298) def parse_method_path(method_path): """ Returns (package, service, method) tuple from parsing method path """ # unpack method path based on "/{package}.{service}/{method}" @@ -10,3 +23,40 @@ def parse_method_path(method_path): return package_service[0], package_service[1], method_name return None, package_service[0], method_name +<<<<<<< HEAD +======= + + +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) + 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) + + +def _parse_target_from_args(args, kwargs): + if "target" in kwargs: + target = kwargs["target"] + else: + target = args[0] + + try: + if target is None: + return + + # ensure URI follows RFC 3986 and is preceeded by double slash + # https://tools.ietf.org/html/rfc3986#section-3.2 + parsed = parse.urlsplit("//" + target if not target.startswith("//") else target) + port = None + try: + port = parsed.port + except ValueError: + log.warning("Non-integer port in target '%s'", target) + return parsed.hostname, port + except ValueError: + log.warning("Malformed target '%s'.", target) +>>>>>>> de406409... fix(grpc): parse target for ipv6 and missing port (#2298) diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index 776caf12925..883d64bd836 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -1,5 +1,6 @@ CPython INfo +IPv MySQL OpenTracing aiobotocore diff --git a/releasenotes/notes/fix-grpc-target-0a1daf38516be79f.yaml b/releasenotes/notes/fix-grpc-target-0a1daf38516be79f.yaml new file mode 100644 index 00000000000..c3a5c1558ba --- /dev/null +++ b/releasenotes/notes/fix-grpc-target-0a1daf38516be79f.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + grpc: handle IPv6 addresses and no port in target. diff --git a/tests/contrib/grpc/test_grpc.py b/tests/contrib/grpc/test_grpc.py index fd96aeeba33..e9c5d6650fc 100644 --- a/tests/contrib/grpc/test_grpc.py +++ b/tests/contrib/grpc/test_grpc.py @@ -550,3 +550,65 @@ def _intercept_call(self, continuation, client_call_details, request_or_iterator def intercept_unary_unary(self, continuation, client_call_details, request): return self._intercept_call(continuation, client_call_details, request) +<<<<<<< HEAD +======= + + +def test_handle_response_future_like(): + from ddtrace.contrib.grpc.client_interceptor import _handle_response + from ddtrace.span import Span + + span = Span(None, None) + + def finish_span(): + span.finish() + + class FutureLike(object): + def add_done_callback(self, fn): + finish_span() + + class NotFutureLike(object): + pass + + _handle_response(span, NotFutureLike()) + 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"]) +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) +>>>>>>> de406409... fix(grpc): parse target for ipv6 and missing port (#2298) diff --git a/tests/contrib/grpc/test_grpc_utils.py b/tests/contrib/grpc/test_grpc_utils.py index e9b606a6803..0e66ebb50c2 100644 --- a/tests/contrib/grpc/test_grpc_utils.py +++ b/tests/contrib/grpc/test_grpc_utils.py @@ -1,3 +1,7 @@ +import mock +import pytest + +from ddtrace.contrib.grpc.utils import _parse_target_from_args from ddtrace.contrib.grpc.utils import parse_method_path @@ -11,3 +15,27 @@ def test_parse_method_path_without_package(): method_path = "/service/method" parsed = parse_method_path(method_path) assert parsed == (None, "service", "method") + + +@mock.patch("ddtrace.contrib.grpc.utils.log") +@pytest.mark.parametrize( + "args, kwargs, result, log_warning_call", + [ + (("localhost:1234",), dict(), ("localhost", 1234), None), + (("localhost",), dict(), ("localhost", None), None), + (("[::]:1234",), dict(), ("::", 1234), None), + (("[::]",), dict(), ("::", None), None), + (None, dict(target="localhost:1234"), ("localhost", 1234), None), + (None, dict(target="localhost"), ("localhost", None), None), + (None, dict(target="[::]:1234"), ("::", 1234), None), + (None, dict(target="[::]"), ("::", None), None), + (("localhost:foo",), dict(), ("localhost", None), ("Non-integer port in target '%s'", "localhost:foo")), + (("",), dict(), (None, None), None), + ], +) +def test_parse_target_from_args(mock_log, args, kwargs, result, log_warning_call): + assert _parse_target_from_args(args, kwargs) == result + if log_warning_call: + mock_log.warning.assert_called_once_with(*log_warning_call) + else: + mock_log.warning.assert_not_called() 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..4739826d6de --- /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" 1619744633550844100 + "duration" 1911900 + "meta" {"runtime-id" "04d67a3bd550461295cfd98eedc69d99" + "grpc.port" "55945" + "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" 714 + "_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" 1619744633552296900 + "duration" 48200 + "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" 1619744633553123900 + "duration" 1079700 + "meta" {"runtime-id" "04d67a3bd550461295cfd98eedc69d99" + "grpc.port" "55945" + "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" 714 + "_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" 1619744633553812700 + "duration" 42900 + "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 27a1c33abb3924a925372dd8c2bad1f1b5fbefaf Mon Sep 17 00:00:00 2001 From: Kyle Verhoog Date: Thu, 13 May 2021 11:25:55 -0400 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Julien Danjou --- ddtrace/contrib/grpc/client_interceptor.py | 1 - ddtrace/contrib/grpc/utils.py | 6 ------ tests/contrib/grpc/test_grpc.py | 3 --- 3 files changed, 10 deletions(-) diff --git a/ddtrace/contrib/grpc/client_interceptor.py b/ddtrace/contrib/grpc/client_interceptor.py index b9f707bc639..22b9833e4a3 100644 --- a/ddtrace/contrib/grpc/client_interceptor.py +++ b/ddtrace/contrib/grpc/client_interceptor.py @@ -172,7 +172,6 @@ def _intercept_client_call(self, method_kind, client_call_details): resource=client_call_details.method, ) -<<<<<<< HEAD # tags for method details method_path = client_call_details.method method_package, method_service, method_name = parse_method_path(method_path) diff --git a/ddtrace/contrib/grpc/utils.py b/ddtrace/contrib/grpc/utils.py index 288af553856..ed12b0fffec 100644 --- a/ddtrace/contrib/grpc/utils.py +++ b/ddtrace/contrib/grpc/utils.py @@ -1,5 +1,3 @@ -<<<<<<< HEAD -======= import logging from ddtrace.internal.compat import parse @@ -10,7 +8,6 @@ log = logging.getLogger(__name__) ->>>>>>> de406409... fix(grpc): parse target for ipv6 and missing port (#2298) def parse_method_path(method_path): """ Returns (package, service, method) tuple from parsing method path """ # unpack method path based on "/{package}.{service}/{method}" @@ -23,8 +20,6 @@ def parse_method_path(method_path): return package_service[0], package_service[1], method_name return None, package_service[0], method_name -<<<<<<< HEAD -======= def set_grpc_method_meta(span, method, method_kind): @@ -59,4 +54,3 @@ def _parse_target_from_args(args, kwargs): return parsed.hostname, port except ValueError: log.warning("Malformed target '%s'.", target) ->>>>>>> de406409... fix(grpc): parse target for ipv6 and missing port (#2298) diff --git a/tests/contrib/grpc/test_grpc.py b/tests/contrib/grpc/test_grpc.py index e9c5d6650fc..679fb3ab4d1 100644 --- a/tests/contrib/grpc/test_grpc.py +++ b/tests/contrib/grpc/test_grpc.py @@ -550,8 +550,6 @@ def _intercept_call(self, continuation, client_call_details, request_or_iterator def intercept_unary_unary(self, continuation, client_call_details, request): return self._intercept_call(continuation, client_call_details, request) -<<<<<<< HEAD -======= def test_handle_response_future_like(): @@ -611,4 +609,3 @@ def handler(request, context): channel.unary_unary("/pkg.Servicer/Handler")(b"request") finally: server.stop(None) ->>>>>>> de406409... fix(grpc): parse target for ipv6 and missing port (#2298) From 98d591935cbc8c04aa8737b5923b0217921f6d4b Mon Sep 17 00:00:00 2001 From: Kyle Verhoog Date: Thu, 13 May 2021 12:57:32 -0400 Subject: [PATCH 3/4] Use public compat module --- ddtrace/contrib/grpc/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddtrace/contrib/grpc/utils.py b/ddtrace/contrib/grpc/utils.py index ed12b0fffec..b47a82b4749 100644 --- a/ddtrace/contrib/grpc/utils.py +++ b/ddtrace/contrib/grpc/utils.py @@ -1,6 +1,6 @@ import logging -from ddtrace.internal.compat import parse +from ddtrace.compat import parse from . import constants From 8eabcd8727c52b0b0187452845ba2ac25184a6f1 Mon Sep 17 00:00:00 2001 From: "Tahir H. Butt" Date: Thu, 13 May 2021 14:52:01 -0400 Subject: [PATCH 4/4] fix snapshot --- ...ib.grpc.test_grpc.test_method_service.snap | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) 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 f932ac1db15..3543d86cfc3 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" 1620852382523305000 - "duration" 2042000 - "meta" {"runtime-id" "e77b2d3fc0304fc7bc9b7520f69177fc" - "grpc.port" "58317" - "grpc.host" "localhost" + "start" 1620933912384429100 + "duration" 2398200 + "meta" {"runtime-id" "b5316a45603f47b2abb9bfd4ef204449" + "grpc.port" "56173" + "grpc.host" "::" "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" 25491 + "system.pid" 625 "_dd.tracer_kr" 1.0}} {"name" "grpc" "service" "grpc-server" @@ -30,13 +30,13 @@ "span_id" 1 "trace_id" 0 "parent_id" 0 - "start" 1620852382524760000 - "duration" 59000 + "start" 1620933912385949600 + "duration" 73300 "meta" {"grpc.method.path" "/Servicer/Handler" - "span.kind" "server" - "grpc.method.name" "Handler" "grpc.method.service" "Servicer" - "grpc.method.kind" "unary"} + "grpc.method.name" "Handler" + "grpc.method.kind" "unary" + "span.kind" "server"} "metrics" {"_dd.measured" 1 "_sampling_priority_v1" 1 "_dd.tracer_kr" 1.0}}] @@ -48,12 +48,12 @@ "span_id" 0 "trace_id" 1 "parent_id" nil - "start" 1620852382525600000 - "duration" 1335000 - "meta" {"runtime-id" "e77b2d3fc0304fc7bc9b7520f69177fc" - "grpc.port" "58317" + "start" 1620933912387141700 + "duration" 1941500 + "meta" {"runtime-id" "b5316a45603f47b2abb9bfd4ef204449" + "grpc.port" "56173" "grpc.method.package" "pkg" - "grpc.host" "localhost" + "grpc.host" "::" "grpc.method.kind" "unary" "span.kind" "client" "grpc.method.path" "/pkg.Servicer/Handler" @@ -63,7 +63,7 @@ "metrics" {"_dd.agent_psr" 1.0 "_dd.measured" 1 "_sampling_priority_v1" 1 - "system.pid" 25491 + "system.pid" 625 "_dd.tracer_kr" 1.0}} {"name" "grpc" "service" "grpc-server" @@ -73,13 +73,13 @@ "span_id" 1 "trace_id" 1 "parent_id" 0 - "start" 1620852382526578000 - "duration" 50000 - "meta" {"grpc.method.package" "pkg" + "start" 1620933912388411400 + "duration" 137300 + "meta" {"grpc.method.path" "/pkg.Servicer/Handler" + "grpc.method.package" "pkg" "grpc.method.service" "Servicer" - "grpc.method.path" "/pkg.Servicer/Handler" - "grpc.method.kind" "unary" "grpc.method.name" "Handler" + "grpc.method.kind" "unary" "span.kind" "server"} "metrics" {"_dd.measured" 1 "_sampling_priority_v1" 1