fix(grpc): handle None values for tags (#2443) (#2445)

* fix(grpc): handle None values for tags

* handle empty hostname in Python 2.7

* correct coercion of hostname

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit c08f02a)

Co-authored-by: Tahir H. Butt <[email protected]>
Co-authored-by: Brett Langdon <[email protected]>
Co-authored-by: Kyle Verhoog <[email protected]>

Author: 4 people

ddtrace/contrib/grpc/client_interceptor.py

@@ -10,12 +10,12 @@
 from ddtrace.vendor import wrapt
 
 from . import constants
+from . import utils
 from .. import trace_utils
 from ...constants import ANALYTICS_SAMPLE_RATE_KEY
 from ...constants import SPAN_MEASURED_KEY
 from ...internal.logger import get_logger
 from ...propagation.http import HTTPPropagator
-from .utils import set_grpc_method_meta
 
 
 log = get_logger(__name__)
@@ -179,10 +179,8 @@ 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_HOST_KEY, self._host)
-        if self._port:
-            span._set_str_tag(constants.GRPC_PORT_KEY, str(self._port))
+        utils.set_grpc_method_meta(span, client_call_details.method, method_kind)
+        utils.set_grpc_client_meta(span, self._host, 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()

ddtrace/contrib/grpc/utils.py

@@ -25,12 +25,24 @@ def parse_method_path(method_path):
 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_path is not None:
+        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)
+    if method_service is not None:
+        span._set_str_tag(constants.GRPC_METHOD_SERVICE_KEY, method_service)
+    if method_name is not None:
+        span._set_str_tag(constants.GRPC_METHOD_NAME_KEY, method_name)
+    if method_kind is not None:
+        span._set_str_tag(constants.GRPC_METHOD_KIND_KEY, method_kind)
+
+
+def set_grpc_client_meta(span, host, port):
+    if host:
+        span._set_str_tag(constants.GRPC_HOST_KEY, host)
+    if port:
+        span._set_str_tag(constants.GRPC_PORT_KEY, str(port))
+    span._set_str_tag(constants.GRPC_SPAN_KIND_KEY, constants.GRPC_SPAN_KIND_VALUE_CLIENT)
 
 
 def _parse_target_from_args(args, kwargs):
@@ -51,6 +63,11 @@ def _parse_target_from_args(args, kwargs):
             port = parsed.port
         except ValueError:
             log.warning("Non-integer port in target '%s'", target)
-        return parsed.hostname, port
+
+        # an empty hostname in Python 2.7 will be an empty string rather than
+        # None
+        hostname = parsed.hostname if parsed.hostname is not None and len(parsed.hostname) > 0 else None
+
+        return hostname, port
     except ValueError:
         log.warning("Malformed target '%s'.", target)

releasenotes/notes/fix-grpc-client-meta-1145db1fec07255f.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    grpc: handle None values for span tags.

tests/contrib/grpc/test_grpc_utils.py

@@ -1,19 +1,18 @@
 import mock
 import pytest
 
-from ddtrace.contrib.grpc.utils import _parse_target_from_args
-from ddtrace.contrib.grpc.utils import parse_method_path
+from ddtrace.contrib.grpc import utils
 
 
 def test_parse_method_path_with_package():
     method_path = "/package.service/method"
-    parsed = parse_method_path(method_path)
+    parsed = utils.parse_method_path(method_path)
     assert parsed == ("package", "service", "method")
 
 
 def test_parse_method_path_without_package():
     method_path = "/service/method"
-    parsed = parse_method_path(method_path)
+    parsed = utils.parse_method_path(method_path)
     assert parsed == (None, "service", "method")
 
 
@@ -23,19 +22,72 @@ def test_parse_method_path_without_package():
     [
         (("localhost:1234",), dict(), ("localhost", 1234), None),
         (("localhost",), dict(), ("localhost", None), None),
+        ((":1234",), dict(), (None, 1234), 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"), (None, 1234), 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
+    assert utils._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()
+
+
+@pytest.mark.parametrize(
+    "host, port, calls",
+    [
+        (
+            "localhost",
+            1234,
+            [mock.call("grpc.host", "localhost"), mock.call("grpc.port", "1234"), mock.call("span.kind", "client")],
+        ),
+        ("localhost", None, [mock.call("grpc.host", "localhost"), mock.call("span.kind", "client")]),
+        (None, 1234, [mock.call("grpc.port", "1234"), mock.call("span.kind", "client")]),
+        (None, None, [mock.call("span.kind", "client")]),
+    ],
+)
+def test_set_grpc_client_meta(host, port, calls):
+    span = mock.MagicMock()
+    utils.set_grpc_client_meta(span, host, port)
+    span._set_str_tag.assert_has_calls(calls)
+
+
+@pytest.mark.parametrize(
+    "method, method_kind, calls",
+    [
+        (
+            "/package.service/method",
+            "unary",
+            [
+                mock.call("grpc.method.path", "/package.service/method"),
+                mock.call("grpc.method.package", "package"),
+                mock.call("grpc.method.service", "service"),
+                mock.call("grpc.method.name", "method"),
+                mock.call("grpc.method.kind", "unary"),
+            ],
+        ),
+        (
+            "/service/method",
+            "unary",
+            [
+                mock.call("grpc.method.path", "/service/method"),
+                mock.call("grpc.method.service", "service"),
+                mock.call("grpc.method.name", "method"),
+                mock.call("grpc.method.kind", "unary"),
+            ],
+        ),
+    ],
+)
+def test_set_grpc_method_meta(method, method_kind, calls):
+    span = mock.MagicMock()
+    utils.set_grpc_method_meta(span, method, method_kind)
+    span._set_str_tag.assert_has_calls(calls)