[contrib:requests] propagate distributed sampling context #372
Conversation
ddtrace/propagation/http.py
Outdated
| @@ -30,7 +30,9 @@ def parent_call(): | |||
| """ | |||
| headers[HTTP_HEADER_TRACE_ID] = str(span_context.trace_id) | |||
| headers[HTTP_HEADER_PARENT_ID] = str(span_context.span_id) | |||
| headers[HTTP_HEADER_SAMPLING_PRIORITY] = str(span_context.sampling_priority) | |||
| sampling_priority = span_context.sampling_priority | |||
| if sampling_priority is not None: # avoid converting None to 'None' | |||
There was a problem hiding this comment.
Nasty bug I found when writing tests, this is currently broken, any server implementation not handling a random string ('None' here) could end up with an exception.
There was a problem hiding this comment.
Do we need a patch to other libraries to ensure that we never int('None')?
There was a problem hiding this comment.
I think we should not patch any other library but instead ensure all libraries use this shared piece of code. As of today, any library using this features the bug, we should fix them all by releasing this. A quick grep shows x-datadog-sampling-priority is only set by this call so we should be fine by fixing just this piece of code.
There was a problem hiding this comment.
The patch is clear. I'm wondering if we can simplify the test since it's not required that we run a real server when we can Mock and capture arguments. I agree that integration tests are good, but we're doing lot of things manually in the test (so we're implementing the server logic in the test itself), so capturing the arguments with Mock is enough.
| _server_tracer = get_dummy_tracer() | ||
| _httpd = None | ||
|
|
||
| class DummyServer(BaseHTTPRequestHandler): |
There was a problem hiding this comment.
Why do we need this complexity? We can simply Mock the request call and ensures that sent headers are the one we are expecting (so the set values / types).
There was a problem hiding this comment.
OK, will rework the test to downgrade it to a simple mock, NP.
ddtrace/propagation/http.py
Outdated
| @@ -30,7 +30,9 @@ def parent_call(): | |||
| """ | |||
| headers[HTTP_HEADER_TRACE_ID] = str(span_context.trace_id) | |||
| headers[HTTP_HEADER_PARENT_ID] = str(span_context.span_id) | |||
| headers[HTTP_HEADER_SAMPLING_PRIORITY] = str(span_context.sampling_priority) | |||
| sampling_priority = span_context.sampling_priority | |||
| if sampling_priority is not None: # avoid converting None to 'None' | |||
There was a problem hiding this comment.
Do we need a patch to other libraries to ensure that we never int('None')?
ddtrace/contrib/requests/patch.py
Outdated
| @@ -33,14 +34,22 @@ def _traced_request_func(func, instance, args, kwargs): | |||
| # sessions to have their own (with the standard global fallback) | |||
| tracer = getattr(instance, 'datadog_tracer', ddtrace.tracer) | |||
|
|
|||
| distributed_tracing_enabled = getattr(instance, 'distributed_tracing_enabled', None) | |||
There was a problem hiding this comment.
Since we don't have a configuration system in the Python client (something I want to design like we did for Ruby), can you provide more details about how people will use our distributed_tracing_enabled? Writing the example in the module docstring is enough.
ufoot
force-pushed
the
christian/distributed-requests
branch
3 times, most recently
from
ecfa3a1
to
538f8dd
Compare
ufoot
force-pushed
the
christian/distributed-requests
branch
from
538f8dd
to
66df5dc
Compare
ddtrace/contrib/requests/__init__.py
Outdated
| from ddtrace.contrib.requests import TracedSession | ||
|
|
||
| session = TracedSession() | ||
| session.distributed_tracing_enabled = True |
There was a problem hiding this comment.
All our other contribs are using simply distributed_tracing as the option key.
Should we just do the same?
There was a problem hiding this comment.
Oh indeed, good catch. aiohttp https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/contrib/aiohttp/middlewares.py#L29 is the only one to use the _enabled flavor (and as it shows up 1st in alphabetical order, might explain why I used this...). Yes, I can rename it to distributed_tracing alone, we might fix aiohttp later if needed.
This is a rework on top of #356
It adds:
distributed_tracing_enabled)Nonetype would be converted in'Node'string when not defined