-
Notifications
You must be signed in to change notification settings - Fork 426
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.
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
[tornado] patch concurrent.futures if available #362
Conversation
ddtrace/contrib/tornado/futures.py
Outdated
# singleton, so we should rely in our top-level import | ||
ctx = Context() | ||
current_ctx = tracer.get_call_context() | ||
ctx._current_span = current_ctx._current_span |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this pending PR #359 will we have to copy other attributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely yes, but I prefer to ship that change + release 0.9.3 and then handle per framework migration to the new API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very technical, but it looks good and tests seem to cover it properly.
GTM!
c47974e
to
aa3916f
Compare
ddtrace/contrib/tornado/futures.py
Outdated
ctx = Context() | ||
current_ctx = tracer.context_provider.active() | ||
if current_ctx is not None: | ||
ctx._current_span = current_ctx.get_current_span() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't use a shallow copy (current_ctx.copy()
) because we must not copy the trace
array or the number of opened spans, otherwise we have a race condition on the array + will never flush the trace.
We can isolate that behavior in a specific method of the Context
though, so we can use that generic method all around the code. Will do that later in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I was more thinking about a custom copy
method (forgot that Python had a default one). So we should probably find another name. And that one would only copy _current_span
/ traceID / spanID / priority / sampled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, gotcha. Yes that's the plan.
7d31c34
to
f5f337b
Compare
… propagation (fallback is required if futures are not available)
f5f337b
to
adf5915
Compare
@classmethod | ||
def activate(cls, ctx): | ||
io_loop = getattr(IOLoop._current, 'instance', None) | ||
if io_loop is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see no related test to that case ; maybe it is already covered somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this section (and the specular for activate(ctx)
) is tested under tests.contrib.tornado.test_executor_decorator
. Without this section, the context propagation between threads fails and no test will pass (you'll have 2 different traces with 2 different trace_id
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides a code coverage question, LGTM
Overview
In general, a new Thread is created every time synchronous work must be done. To achieve that, a common approach is to use
concurrent.futures.ThreadPoolExecutor
so that a specificExecutor
implementation handles the thread pool. This results in:Unfortunately, if the
ThreadPoolExecutor
is used directly without thetornado.concurrent.run_on_executor
decorator, we miss instrumentation to propagate theContext
from theMainThread
to the new thread.This implementation ensures that the
Context
is properly propagated.Compatibility
It works for the current test suite, though the test is skipped in Python 2.7 without
futures
because that class (and that way to submit the function) is not available.Notes on the module
This new instrumentation is related to
futures
and NOT to Tornado. We may think to move that as a generalcontrib
module (i.e.patch(futures=True)
), though it requires more time to test it in all asynchronous and synchronous frameworks.