Skip to content

Commit 58a7bab

Browse files
mergify[bot]majorgreysKyle-VerhoogJulien Danjou
authored
fix(grpc): parse target for ipv6 and missing port (backport #2298) (#2431)
* fix(grpc): parse target for ipv6 and missing port (#2298) * fix(grpc): add snapshot test for method service metadata * revert change to host parsing * fix(grpc): parse target for ipv6 and missing port Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit de40640) # Conflicts: # ddtrace/contrib/grpc/client_interceptor.py # ddtrace/contrib/grpc/utils.py # tests/contrib/grpc/test_grpc.py # tests/snapshots/tests.contrib.grpc.test_grpc.test_method_service.snap * Apply suggestions from code review Co-authored-by: Julien Danjou <[email protected]> * Use public compat module * fix snapshot Co-authored-by: Tahir H. Butt <[email protected]> Co-authored-by: Kyle Verhoog <[email protected]> Co-authored-by: Julien Danjou <[email protected]> Co-authored-by: Kyle Verhoog <[email protected]>
1 parent 03ab549 commit 58a7bab

File tree

8 files changed

+91
-37
lines changed

8 files changed

+91
-37
lines changed

‎ddtrace/contrib/grpc/client_interceptor.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,9 +178,10 @@ def _intercept_client_call(self, method_kind, client_call_details):
178178
span.set_tag(SPAN_MEASURED_KEY)
179179

180180
set_grpc_method_meta(span, client_call_details.method, method_kind)
181-
span._set_str_tag(constants.GRPC_SPAN_KIND_KEY, constants.GRPC_SPAN_KIND_VALUE_CLIENT)
182181
span._set_str_tag(constants.GRPC_HOST_KEY, self._host)
183-
span._set_str_tag(constants.GRPC_PORT_KEY, self._port)
182+
if self._port:
183+
span._set_str_tag(constants.GRPC_PORT_KEY, str(self._port))
184+
span._set_str_tag(constants.GRPC_SPAN_KIND_KEY, constants.GRPC_SPAN_KIND_VALUE_CLIENT)
184185

185186
sample_rate = config.grpc.get_analytics_sample_rate()
186187
if sample_rate is not None:

‎ddtrace/contrib/grpc/patch.py‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
from ddtrace.vendor.wrapt import wrap_function_wrapper as _w
66

77
from . import constants
8+
from . import utils
89
from ...utils.wrappers import unwrap as _u
910
from .client_interceptor import create_client_interceptor
1011
from .client_interceptor import intercept_channel
@@ -95,7 +96,7 @@ def _client_channel_interceptor(wrapped, instance, args, kwargs):
9596
if not pin or not pin.enabled():
9697
return channel
9798

98-
(host, port) = _parse_target_from_arguments(args, kwargs)
99+
(host, port) = utils._parse_target_from_args(args, kwargs)
99100

100101
interceptor_function = create_client_interceptor(pin, host, port)
101102
return grpc.intercept_channel(channel, interceptor_function)
@@ -118,14 +119,3 @@ def _server_constructor_interceptor(wrapped, instance, args, kwargs):
118119
kwargs["interceptors"] = (interceptor,)
119120

120121
return wrapped(*args, **kwargs)
121-
122-
123-
def _parse_target_from_arguments(args, kwargs):
124-
if "target" in kwargs:
125-
target = kwargs["target"]
126-
else:
127-
target = args[0]
128-
129-
split = target.rsplit(":", 2)
130-
131-
return (split[0], split[1] if len(split) > 1 else None)

‎ddtrace/contrib/grpc/utils.py‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,13 @@
1+
import logging
2+
3+
from ddtrace.compat import parse
4+
15
from . import constants
26

37

8+
log = logging.getLogger(__name__)
9+
10+
411
def parse_method_path(method_path):
512
""" Returns (package, service, method) tuple from parsing method path """
613
# unpack method path based on "/{package}.{service}/{method}"
@@ -24,3 +31,26 @@ def set_grpc_method_meta(span, method, method_kind):
2431
span._set_str_tag(constants.GRPC_METHOD_SERVICE_KEY, method_service)
2532
span._set_str_tag(constants.GRPC_METHOD_NAME_KEY, method_name)
2633
span._set_str_tag(constants.GRPC_METHOD_KIND_KEY, method_kind)
34+
35+
36+
def _parse_target_from_args(args, kwargs):
37+
if "target" in kwargs:
38+
target = kwargs["target"]
39+
else:
40+
target = args[0]
41+
42+
try:
43+
if target is None:
44+
return
45+
46+
# ensure URI follows RFC 3986 and is preceeded by double slash
47+
# https://tools.ietf.org/html/rfc3986#section-3.2
48+
parsed = parse.urlsplit("//" + target if not target.startswith("//") else target)
49+
port = None
50+
try:
51+
port = parsed.port
52+
except ValueError:
53+
log.warning("Non-integer port in target '%s'", target)
54+
return parsed.hostname, port
55+
except ValueError:
56+
log.warning("Malformed target '%s'.", target)

‎docs/spelling_wordlist.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
CPython
22
INfo
3+
IPv
34
MySQL
45
OpenTracing
56
aiobotocore

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
fixes:
3+
- |
4+
grpc: handle IPv6 addresses and no port in target.

‎tests/contrib/grpc/test_grpc.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -603,7 +603,7 @@ def handler(request, context):
603603
options=(("grpc.so_reuseport", 0),),
604604
)
605605
port = server.add_insecure_port("[::]:0")
606-
channel = grpc.insecure_channel("localhost:{}".format(port))
606+
channel = grpc.insecure_channel("[::]:{}".format(port))
607607
server.add_generic_rpc_handlers((_UnaryUnaryRpcHandler(handler),))
608608
try:
609609
server.start()

‎tests/contrib/grpc/test_grpc_utils.py‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
import mock
2+
import pytest
3+
4+
from ddtrace.contrib.grpc.utils import _parse_target_from_args
15
from ddtrace.contrib.grpc.utils import parse_method_path
26

37

@@ -11,3 +15,27 @@ def test_parse_method_path_without_package():
1115
method_path = "/service/method"
1216
parsed = parse_method_path(method_path)
1317
assert parsed == (None, "service", "method")
18+
19+
20+
@mock.patch("ddtrace.contrib.grpc.utils.log")
21+
@pytest.mark.parametrize(
22+
"args, kwargs, result, log_warning_call",
23+
[
24+
(("localhost:1234",), dict(), ("localhost", 1234), None),
25+
(("localhost",), dict(), ("localhost", None), None),
26+
(("[::]:1234",), dict(), ("::", 1234), None),
27+
(("[::]",), dict(), ("::", None), None),
28+
(None, dict(target="localhost:1234"), ("localhost", 1234), None),
29+
(None, dict(target="localhost"), ("localhost", None), None),
30+
(None, dict(target="[::]:1234"), ("::", 1234), None),
31+
(None, dict(target="[::]"), ("::", None), None),
32+
(("localhost:foo",), dict(), ("localhost", None), ("Non-integer port in target '%s'", "localhost:foo")),
33+
(("",), dict(), (None, None), None),
34+
],
35+
)
36+
def test_parse_target_from_args(mock_log, args, kwargs, result, log_warning_call):
37+
assert _parse_target_from_args(args, kwargs) == result
38+
if log_warning_call:
39+
mock_log.warning.assert_called_once_with(*log_warning_call)
40+
else:
41+
mock_log.warning.assert_not_called()

‎tests/snapshots/tests.contrib.grpc.test_grpc.test_method_service.snap‎

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@
66
"span_id" 0
77
"trace_id" 0
88
"parent_id" nil
9-
"start" 1620852382523305000
10-
"duration" 2042000
11-
"meta" {"runtime-id" "e77b2d3fc0304fc7bc9b7520f69177fc"
12-
"grpc.port" "58317"
13-
"grpc.host" "localhost"
9+
"start" 1620933912384429100
10+
"duration" 2398200
11+
"meta" {"runtime-id" "b5316a45603f47b2abb9bfd4ef204449"
12+
"grpc.port" "56173"
13+
"grpc.host" "::"
1414
"grpc.method.kind" "unary"
1515
"span.kind" "client"
1616
"grpc.method.path" "/Servicer/Handler"
@@ -20,7 +20,7 @@
2020
"metrics" {"_dd.agent_psr" 1.0
2121
"_dd.measured" 1
2222
"_sampling_priority_v1" 1
23-
"system.pid" 25491
23+
"system.pid" 625
2424
"_dd.tracer_kr" 1.0}}
2525
{"name" "grpc"
2626
"service" "grpc-server"
@@ -30,13 +30,13 @@
3030
"span_id" 1
3131
"trace_id" 0
3232
"parent_id" 0
33-
"start" 1620852382524760000
34-
"duration" 59000
33+
"start" 1620933912385949600
34+
"duration" 73300
3535
"meta" {"grpc.method.path" "/Servicer/Handler"
36-
"span.kind" "server"
37-
"grpc.method.name" "Handler"
3836
"grpc.method.service" "Servicer"
39-
"grpc.method.kind" "unary"}
37+
"grpc.method.name" "Handler"
38+
"grpc.method.kind" "unary"
39+
"span.kind" "server"}
4040
"metrics" {"_dd.measured" 1
4141
"_sampling_priority_v1" 1
4242
"_dd.tracer_kr" 1.0}}]
@@ -48,12 +48,12 @@
4848
"span_id" 0
4949
"trace_id" 1
5050
"parent_id" nil
51-
"start" 1620852382525600000
52-
"duration" 1335000
53-
"meta" {"runtime-id" "e77b2d3fc0304fc7bc9b7520f69177fc"
54-
"grpc.port" "58317"
51+
"start" 1620933912387141700
52+
"duration" 1941500
53+
"meta" {"runtime-id" "b5316a45603f47b2abb9bfd4ef204449"
54+
"grpc.port" "56173"
5555
"grpc.method.package" "pkg"
56-
"grpc.host" "localhost"
56+
"grpc.host" "::"
5757
"grpc.method.kind" "unary"
5858
"span.kind" "client"
5959
"grpc.method.path" "/pkg.Servicer/Handler"
@@ -63,7 +63,7 @@
6363
"metrics" {"_dd.agent_psr" 1.0
6464
"_dd.measured" 1
6565
"_sampling_priority_v1" 1
66-
"system.pid" 25491
66+
"system.pid" 625
6767
"_dd.tracer_kr" 1.0}}
6868
{"name" "grpc"
6969
"service" "grpc-server"
@@ -73,13 +73,13 @@
7373
"span_id" 1
7474
"trace_id" 1
7575
"parent_id" 0
76-
"start" 1620852382526578000
77-
"duration" 50000
78-
"meta" {"grpc.method.package" "pkg"
76+
"start" 1620933912388411400
77+
"duration" 137300
78+
"meta" {"grpc.method.path" "/pkg.Servicer/Handler"
79+
"grpc.method.package" "pkg"
7980
"grpc.method.service" "Servicer"
80-
"grpc.method.path" "/pkg.Servicer/Handler"
81-
"grpc.method.kind" "unary"
8281
"grpc.method.name" "Handler"
82+
"grpc.method.kind" "unary"
8383
"span.kind" "server"}
8484
"metrics" {"_dd.measured" 1
8585
"_sampling_priority_v1" 1

0 commit comments

Comments
 (0)