diff --git a/ddtrace/contrib/grpc/client_interceptor.py b/ddtrace/contrib/grpc/client_interceptor.py index 7e550c96923..1e0def08de9 100644 --- a/ddtrace/contrib/grpc/client_interceptor.py +++ b/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() diff --git a/ddtrace/contrib/grpc/utils.py b/ddtrace/contrib/grpc/utils.py index bbe551d649e..5864a67ba7c 100644 --- a/ddtrace/contrib/grpc/utils.py +++ b/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) diff --git a/releasenotes/notes/fix-grpc-client-meta-1145db1fec07255f.yaml b/releasenotes/notes/fix-grpc-client-meta-1145db1fec07255f.yaml new file mode 100644 index 00000000000..6b1e994d340 --- /dev/null +++ b/releasenotes/notes/fix-grpc-client-meta-1145db1fec07255f.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + grpc: handle None values for span tags. diff --git a/tests/contrib/grpc/test_grpc_utils.py b/tests/contrib/grpc/test_grpc_utils.py index 0e66ebb50c2..e828860a68a 100644 --- a/tests/contrib/grpc/test_grpc_utils.py +++ b/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,10 +22,12 @@ 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")), @@ -34,8 +35,59 @@ def test_parse_method_path_without_package(): ], ) 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)