Skip to content
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

Merged
merged 8 commits into from
Jan 30, 2018

Conversation

palazzem
Copy link

@palazzem palazzem commented Oct 24, 2017

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 specific Executor implementation handles the thread pool. This results in:

class MyHandler(TornadoHandler):
    executor = ThreadPoolExecutor(NUM_THREADS)

     def query(self):
        # do something

    @gen.coroutine
    def get(self, endpoint):
        response = yield self.executor.submit(query)
        return response

Unfortunately, if the ThreadPoolExecutor is used directly without the tornado.concurrent.run_on_executor decorator, we miss instrumentation to propagate the Context from the MainThread 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 general contrib module (i.e. patch(futures=True)), though it requires more time to test it in all asynchronous and synchronous frameworks.

@palazzem palazzem added this to the 0.9.3 milestone Oct 24, 2017
@palazzem palazzem requested a review from LotharSee October 24, 2017 13:31
# 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
Copy link
Contributor

@LotharSee LotharSee Oct 24, 2017

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?

Copy link
Author

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.

LotharSee
LotharSee previously approved these changes Oct 24, 2017
Copy link
Contributor

@LotharSee LotharSee left a 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!

@palazzem palazzem modified the milestones: 0.9.3, 0.10.0 Oct 26, 2017
@palazzem palazzem force-pushed the palazzem/python-futures branch 2 times, most recently from c47974e to aa3916f Compare October 31, 2017 14:09
ctx = Context()
current_ctx = tracer.context_provider.active()
if current_ctx is not None:
ctx._current_span = current_ctx.get_current_span()
Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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.

@palazzem palazzem removed this from the 0.10.0 milestone Nov 7, 2017
@palazzem palazzem added this to the 0.10.1 milestone Dec 11, 2017
@palazzem palazzem force-pushed the palazzem/python-futures branch from 7d31c34 to f5f337b Compare December 15, 2017 15:54
@palazzem palazzem added the wip label Dec 15, 2017
@palazzem palazzem modified the milestones: 0.10.1, 0.11.0 Jan 29, 2018
@palazzem palazzem force-pushed the palazzem/python-futures branch from f5f337b to adf5915 Compare January 29, 2018 16:38
@palazzem palazzem modified the milestones: 0.11.0, 0.10.1 Jan 29, 2018
@classmethod
def activate(cls, ctx):
io_loop = getattr(IOLoop._current, 'instance', None)
if io_loop is None:
Copy link
Contributor

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?

Copy link
Author

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)

Copy link
Contributor

@LotharSee LotharSee left a 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

@palazzem palazzem merged commit 06b744f into master Jan 30, 2018
@palazzem palazzem deleted the palazzem/python-futures branch January 30, 2018 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants