Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 14, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d55f4ac
fix(grpc): handle None values for tags
majorgreys May 14, 2021
03685fb
handle empty hostname in Python 2.7
majorgreys May 14, 2021
52b8d20
correct coercion of hostname
majorgreys May 14, 2021
36d2ee2
Merge branch 'master' into fix-grpc-client-meta
mergify[bot] May 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions ddtrace/contrib/grpc/client_interceptor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__)
Expand Down Expand Up @@ -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()
Expand Down
27 changes: 22 additions & 5 deletions ddtrace/contrib/grpc/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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)
4 changes: 4 additions & 0 deletions releasenotes/notes/fix-grpc-client-meta-1145db1fec07255f.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
fixes:
- |
grpc: handle None values for span tags.
62 changes: 57 additions & 5 deletions tests/contrib/grpc/test_grpc_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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")


Expand All @@ -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):
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of this pattern because it's a bit removed from what could happen realistically in the integration - but I also understand that it's hard to replicate these scenarios

Copy link
Contributor Author

@majorgreys majorgreys May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I think in this case I opted to go the unit testing route rather than an integration test because it's unclear how we could reproduce such a scenario. We could spend some time moving forward with rethinking these tests though.

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)