fix(grpc): parse target for ipv6 and missing port (backport #2298)

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:

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)

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)

docs/spelling_wordlist.txt

@@ -1,5 +1,6 @@
 CPython
 INfo
+IPv
 MySQL
 OpenTracing
 aiobotocore

releasenotes/notes/fix-grpc-target-0a1daf38516be79f.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    grpc: handle IPv6 addresses and no port in target.

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()

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()

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