diff --git a/ddtrace/contrib/grpc/client_interceptor.py b/ddtrace/contrib/grpc/client_interceptor.py index abbf8031b54..dc2c0890414 100644 --- a/ddtrace/contrib/grpc/client_interceptor.py +++ b/ddtrace/contrib/grpc/client_interceptor.py @@ -178,9 +178,10 @@ def _intercept_client_call(self, method_kind, client_call_details): span.set_tag(SPAN_MEASURED_KEY) set_grpc_method_meta(span, client_call_details.method, method_kind) - span._set_str_tag(constants.GRPC_SPAN_KIND_KEY, constants.GRPC_SPAN_KIND_VALUE_CLIENT) span._set_str_tag(constants.GRPC_HOST_KEY, self._host) - span._set_str_tag(constants.GRPC_PORT_KEY, self._port) + if self._port: + span._set_str_tag(constants.GRPC_PORT_KEY, str(self._port)) + span._set_str_tag(constants.GRPC_SPAN_KIND_KEY, constants.GRPC_SPAN_KIND_VALUE_CLIENT) sample_rate = config.grpc.get_analytics_sample_rate() if sample_rate is not None: 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 0f39d1963ba..b47a82b4749 100644 --- a/ddtrace/contrib/grpc/utils.py +++ b/ddtrace/contrib/grpc/utils.py @@ -1,6 +1,13 @@ +import logging + +from ddtrace.compat import parse + from . import constants +log = logging.getLogger(__name__) + + def parse_method_path(method_path): """ Returns (package, service, method) tuple from parsing method path """ # unpack method path based on "/{package}.{service}/{method}" @@ -24,3 +31,26 @@ def set_grpc_method_meta(span, method, method_kind): 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) 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 d66951b8f9c..9fd7c1bbbd3 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("localhost:{}".format(port)) + channel = grpc.insecure_channel("[::]:{}".format(port)) server.add_generic_rpc_handlers((_UnaryUnaryRpcHandler(handler),)) try: server.start() 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 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