AsyncioScopeManager based on contextvars and supporting Tornado 6 #118

condorcet · 2019-04-16T17:55:57Z

Python 3.7 provides contextvars that can be used for implicitly propagation of context from parent to child coroutines / tasks.
Implementation of AsyncioScopeManager in the PR based on this feature so no more need to make manual activation of parent span in child coroutine / tasks like this:

with tracer.scope_manager.activate(parent_span, finish_on_close=False):
                ...
                with tracer.start_active_span('child') as scope:
                    ...

For python 3.6 we have backport of contextvars with async / await supporting https://github.com/fantix/aiocontextvars that based on the "official" backport https://github.com/MagicStack/contextvars. So in python 3.6 "new" AsyncioScopeManager also works.

Testbed contains tests for "new" AsyncioScopeManager and compatibility tests for original scope manager.

On the other side Tornado >= 6 became fully asyncio-based framework (that why stack_context was deprecated) so we no need to support TornadoScopeManager for these versions. I think the better way is to use AsyncioScopeManager in new Tornado applications instead of backporting stack_context or something like this in TornadoScopeManager specially for opentracing needs.
Of course for Tornado < 6 we have to use TornadoScopeManager and it still here.

carlosalberto · 2019-04-24T22:45:17Z

Hey hey @condorcet awesome, looks pretty neat. I will get back for a full review in the next days. One thing to maybe do, is to rename the 'new' AsyncioScopeManager to ContextScopeManager or something, as it is actually more related to contextvars ;)

(also, this way it won't be weird to use it with Tornado apps ;) )

condorcet · 2019-04-30T11:06:36Z

@carlosalberto I thought about backward compatibility between new and old scope manager, that's why I made changes right into AsyncioScopeManager. As I understand you would prefer another one context manager (something like ContextScopeManager) that will be separated from existing AsyncioScopeManager. Also it should contain their own testbed. Am I right?

condorcet · 2019-05-29T13:37:09Z

@carlosalberto ping

carlosalberto · 2019-06-10T14:10:16Z

Hey @condorcet

Sorry for the delayed answer. So keeping backwards compatibility is nice - so what about having the new manager be ContextScopeManager, and have AsyncioContextManager be a child of it with, without any items? This way it would be the actual same implementation, but we would keep the old AsyncioScopeManager name around ;)

class AsyncioScopeManager(ContextScopeManager):
   """ Documentation mentioning this now inherits from ContextScopeManager, etc
   """
   pass

We can keep the old TornadoScopeManager for tornado < 6, and tell users to use the new ContextScopeManager for >= 6.

Also, we can keep a single testbed for ContextScopeManager/AsyncioScopeManager ;)

Let me know if this sounds reasonable for you - else, feel free to comment :)

Jamim · 2019-06-14T21:03:23Z

ping @condorcet

condorcet · 2019-06-15T18:19:27Z

Hello @carlosalberto thank you for answer.
Let me explain my doubt.
I have the following picture in my head:

Tornado 5 (with async/await style, not "old" gen.coroutine style) and Tornado 6 should using AsyncioScopeManager, because of using asyncio loop and async / await syntax. This scope manager common by design for any async/await code, particular new Tornado apps.
Tornado 4 and Tornado 5 (with old gen.coroutine style) should using special TornadoScopeManager with narrow scope of usage.

If we make special ContextScopeManager for Tornado >= 6 I'm afraid it can confuse users -- another one scope manager without any difference AsyncioScopeManager, but should be used only in Tornado apps. Because under the hood Tornado >=6 works as any other asyncio-based framework it will be better to use exactly AsyncioScopeManager. ContextScopeManager in this case will be redundant.

What do you think?

condorcet · 2019-06-24T20:52:36Z

ping @carlosalberto

carlosalberto · 2019-07-11T02:14:36Z

opentracing/__init__.py

@@ -14,6 +14,11 @@
 # limitations under the License.

 from __future__ import absolute_import
+try:
+    # Contextvars backport with coroutine supporting (python 3.6).
+    import aiocontextvars  # noqa


Any reason to not try this import from opentracing.scope_managers.asyncio?

carlosalberto · 2019-07-11T02:18:36Z

.travis.yml


 install:
  - make bootstrap
+  - pip install -q "tornado$TORNADO"


Probably it's safer to do this before make bootstrap ;)

carlosalberto · 2019-07-11T02:25:34Z

opentracing/scope_managers/asyncio.py

-    Automatic :class:`~opentracing.Span` propagation from
-    parent coroutines to their children is not provided, which needs to be
-    done manually:
+    The scope manager provides automatic :class:`~opentracing.Span` propagation


This is one reason that makes me feel we should keep this new implementation under ContextVarScopeManager or similar - otherwise, we would be breaking the assumption that manual propagation is required (we could at the same time deprecate AsyncioScopeManager in favor of this new one, of course).

At the same time, does aiocontextvars work for Python 3.4/3.5? If not, we cannot use them with asyncio (I bring this up as I've talked to people and the need to still keep Python 3.4 backwards compatibility).

(I we are going to take this route, we would need, yeah, a section for contextvars in the testbed :( )

Hello @carlosalberto !

...otherwise, we would be breaking the assumption that manual propagation is required (we could at the same time deprecate AsyncioScopeManager in favor of this new one, of course).

That's what I meant about backward compatibility of new scope manager :) No need to activate parent span, but you can do it and it doesn't break behavior of existing user code. There are tests that prove it.

I bring this up as I've talked to people and the need to still keep Python 3.4 backwards compatibility

Wow! But this is really strong reason to separate new scope manager, because both libraries (contextvars and aiocontextvars) doesn't support python 3.4. So this brave new ContextVarScopeManager we have to use only with python >= 3.5

we could at the same time deprecate AsyncioScopeManager in favor of this new one, of course

Maybe we can release new scope manager in new major version of the library (3.0.0) with renamed existing AsyncioScopeManager -> OldAsyncioScopeManager? Yes, it will break backward compatibility for python 3.4 users, but it's ok in terms of semver.

carlosalberto · 2019-07-11T02:29:35Z

Hey @condorcet

Sorry for the delayed answer - a mix of hectic days (which included talking to people on how to manage things with the nice incoming contextvar module), along with holidays got in the way.

I left a pair of comments. Let me know what you think (as I won't be taking long holidays anytime soon, I should be able to be super responsive now - I can also help with the testbed part too ;) )

condorcet · 2019-07-20T09:34:32Z

@carlosalberto We definitely have to separate ContextVarScopeManager from AsyncioScopeManager: I didn't consider situation with callbacks / tasks that scheduling in event loop in some coroutine which already have parent context. And with contextvars this parent span will propagate to the scheduled callback / task. It's not expected behavior and may break tracing when someone use original AsyncioScopeManager, where auto-propagation doesn't work and new task will be created without parent.

Another feature we should remember when using ContextVarScopeManager is periodic callbacks / tasks. For example :

async def task():
            await asyncio.sleep(5)
            with self.tracer.start_active_span('periodic_task'):
                # next iteration of `task` takes parent context from previous iteration
                # and it will go on forever.
                await task()

Of course it's very synthetic and rude: you shouldn't await task() right into context or more preferred you should use ignore_active_span=True in context manager. But user should be aware about the feature when making tracing wrappers for existing code.

I have some draft and hope to commit it in next days.

condorcet · 2019-07-26T09:58:42Z

Hello @carlosalberto !
I made ContextVarScopeManager that separated from AsyncioScopeManager. Theoretically new scope manager is more general and can be used instead of ThreadLocalScopeManager, because of contextvars, but we should prove it by tests. In this PR I've only added tests and testbed for new scope manager with asyncio.

Also I removed contextvars backport for following reasons:

It doesn't work with python < 3.5.
It doesn't support context propagation for methods like call_at (but we can achieve this feature by the PR Patch call_soon and friends. fantix/aiocontextvars#66) so behavior in python 3.5/3.6 and 3.7 will be different.
I think this reasons can confuse users, so better to keep it simple and mention that ContextVarScopeManager can be used only in Python 3.7+.

Also I've added some new tests for case when parent context propagates to coroutines / callbacks that was scheduled in event loop by call_at and other methods. You can see tests here 376c91d but I don't think that it's the best place for them.

no_parent_scope is context manager that can "reset" current scope: 376c91d#diff-f45867ddcb1eafc26fc3a08eaa762ae3R110
It also has tests. I don't have strong opinion about useful of this helper :)

Jamim · 2019-08-28T12:51:56Z

Hello @condorcet,

You probably might help with the similar work for OpenTelemetry which is the next major version of the OpenTracing and OpenCensus projects.
If so, could you please take a look at open-telemetry/opentelemetry-python#92?

Thank you!

Jamim · 2019-08-31T22:11:48Z

Hello @carlosalberto,
Can you please give @condorcet some additional feedback?
Thanks!

opentracing/scope_managers/contextvars.py

carlosalberto · 2019-09-09T14:28:06Z

opentracing/scope_managers/contextvars.py

+
+
+@contextmanager
+def no_parent_scope():


Any reason to not simply provide default=None for _SCOPE?

I think it's not enough. I've added docstring to make the idea clear. Sometimes we can have a scope, that we don't want to propagate.

carlosalberto · 2019-09-09T14:29:15Z

opentracing/scope_managers/contextvars.py

+_SCOPE = ContextVar('scope')
+
+
+class ContextVarsScopeManager(ThreadLocalScopeManager):


We don't need to inherit from ThreadLocalScopeManager, I think (we are not calling into it, and contextvars itself works fine with thread-local data).

carlosalberto · 2019-09-09T14:33:48Z

testbed/README.md

@@ -28,6 +28,10 @@ No automatic `Span` propagation between parent and children tasks is provided, a

 `TornadoScopeManager` uses a variation of `tornado.stack_context.StackContext` to both store **and** automatically propagate the context from parent coroutines to their children. 

+### contextvars
+
+`ContextVarsScopeManager` uses [contextvars](https://docs.python.org/3/library/contextvars.html) module to both store **and** automatically propagate the context from parent coroutines / tasks / scheduled in event loop callbacks to their children.


IIRC contextvars automatically propagates context only when used with asyncio - else, the user needs to propagate himself the context (through contextvars.copy_context()). Maybe we could add a note here?

Do you mean explicit span propagation in multi-threaded code? Because you don't need to do this in single-threaded code, it works implicitly (like threading.local). Or I misunderstood your idea?

carlosalberto · 2019-09-09T14:34:43Z

testbed/__main__.py

@@ -47,4 +55,6 @@ def get_test_directories():
        suite = loader.loadTestsFromModule(test_module)
        main_suite.addTests(suite)

-unittest.TextTestRunner(verbosity=3).run(main_suite)
+result = unittest.TextTestRunner(verbosity=3).run(main_suite)


carlosalberto · 2019-09-09T14:36:09Z

testbed/test_late_span_finish/test_asyncio.py

@@ -44,7 +44,7 @@ def submit_subtasks(self, parent_span):
            logger.info('Running %s' % name)
            with self.tracer.scope_manager.activate(parent_span, False):
                with self.tracer.start_active_span(name):
-                    asyncio.sleep(0.1)
+                    await asyncio.sleep(0.1)


carlosalberto · 2019-09-09T14:40:56Z

testbed/testcase.py

    def assertNamesEqual(self, spans, names):
        self.assertEqual(list(map(lambda x: x.operation_name, spans)), names)
+
+    def assertEmptySpan(self, span, name):


I'd leave the name assertion out of this test method ;)

It's super convenient to assert span with name, because every span must has name :) I can make name optional in this helper.

carlosalberto · 2019-09-09T14:43:13Z

Hey @condorcet

Sorry for the delay, had to look for some cycles to review (and play) with your code ;) it all looks good, thanks a lot!

I left a few comments. Once that is resolved (along your own TODO notes), we should be ready to merge ;)

…ornado to travis.yml.

…cstrings and README

condorcet · 2019-09-09T20:20:55Z

Hello @carlosalberto thank you for your feedback!
I've made some fixes and rebase this branch on master.

I need your help with this bunch of contextvar-specific tests https://github.com/opentracing/opentracing-python/pull/118/files#diff-bd336347a31f644e377b29aa0b028962R64 Where should I move them? Or just leave them in this file?

condorcet · 2019-09-23T14:48:24Z

Hi @carlosalberto ! Please could you take a look at fixes?
Thanks in advance!

Jamim · 2019-10-10T21:28:06Z

ping @carlosalberto

rhilfers · 2019-11-11T20:42:20Z

excited for this one.. bump @carlosalberto . Thanks @condorcet for the implementation!

condorcet · 2019-11-26T08:11:30Z

@carlosalberto @yurishkuro Hi! What about the PR? While we don't have stable release of Open Telemetry which includes contextvars out of the box, it would be great to have this feature in OT. For example this feature was added by https://github.com/DataDog/dd-trace-py (which compatible with OT) on September.
What should we do next to merge it to master?

yurishkuro · 2019-11-27T07:12:24Z

I have no experience with asyncio or contextvars, so cannot provide a review. However, it appears that this change is purely additive, so appears to be safe to merge.

Testbed contains tests for "new" AsyncioScopeManager and compatibility tests for original scope manager.

Do you mean that the test bed runs all the existing scope manager implementations as well as the new one?

condorcet · 2019-11-27T19:36:54Z

Testbed contains tests for "new" AsyncioScopeManager and compatibility tests for original scope manager.

Do you mean that the test bed runs all the existing scope manager implementations as well as the new one?

@yurishkuro Yes, you're right, finally we have the new one scope manager ContextVarsScopeManager and test bed for it.

sheregeda · 2019-12-03T10:05:15Z

ping @carlosalberto

nicholasamorim · 2019-12-10T11:14:59Z

This would be massive for those using AsyncIO in Python3.7+

yurishkuro

I find the testbed design kind of weird, I was expecting some common code in each of the use cases, and each test for a specific context manager just providing some customizations. But it's not in scope of this PR to fix.

setup.py

yurishkuro · 2019-12-10T12:15:33Z

testbed/test_common_request_handler/README.md

@@ -5,7 +5,8 @@ This example shows a `Span` used with `RequestHandler`, which is used as a middl
 Implementation details:
 - For `threading`, no active `Span` is consumed as the tasks may be run concurrently on different threads, and an explicit `SpanContext` has to be saved to be used as parent.
 - For `gevent` and `asyncio`, as no automatic `Span` propagation is done, an explicit `Span` has to be saved to be used as parent (observe an instrumentation library could help to do that implicitly - we stick to the simplest case, though).
- For `tornado`, as the `StackContext` automatically propapates the context (even is the tasks are called through different coroutines), we **do** leverage the active `Span`.
+- For `tornado` (as the `StackContext`) and `contextvars` automatically propagates the context (even is the tasks are called through different coroutines), we **do** leverage the active `Span`.


this sentence seems broken, eg. what is "(as the StackContext)"?

Oh, thank you! I'll fix the typo.

testbed/test_multiple_callbacks/README.md

testbed/test_nested_callbacks/README.md

testbed/test_subtask_span_propagation/README.md

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com>

testbed/test_common_request_handler/README.md

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com>

condorcet mentioned this pull request Apr 16, 2019

Tests failing with Tornado 6.x version due to missing stack_context #115

Closed

condorcet force-pushed the asyncio-context-with-context-vars branch from 2824a21 to a3bc006 Compare May 6, 2019 16:21

This was referenced May 24, 2019

Add support to Tornado 6 uber-common/opentracing-python-instrumentation#83

Open

@traced_function now works with AsyncIO's coroutines. uber-common/opentracing-python-instrumentation#84

Open

carlosalberto reviewed Jul 11, 2019

View reviewed changes

condorcet force-pushed the asyncio-context-with-context-vars branch from a3bc006 to 376c91d Compare July 26, 2019 09:42

condorcet mentioned this pull request Aug 28, 2019

asyncio context propagation in Python 3.5/3.6 open-telemetry/opentelemetry-python#71

Closed

carlosalberto reviewed Sep 9, 2019

View reviewed changes

opentracing/scope_managers/contextvars.py Show resolved Hide resolved

carlosalberto reviewed Sep 9, 2019

View reviewed changes

Asyncio context manager with contextvars. Add different versions of t…

640557c

…ornado to travis.yml.

condorcet added 2 commits September 9, 2019 22:59

Make new context manager based on python 3.7 contextvars.

784d815

Inherit ContextVarsScopeManagerFix directly from ScopeManager, fix do…

c4468e7

…cstrings and README

condorcet force-pushed the asyncio-context-with-context-vars branch from 376c91d to c4468e7 Compare September 9, 2019 20:07

condorcet mentioned this pull request Oct 10, 2019

Remove Tornado upper boundary jaegertracing/jaeger-client-python#262

Merged

yurishkuro reviewed Dec 10, 2019

View reviewed changes

yurishkuro changed the title ~~Proof of concept: AsyncioScopeManager based on contextvars and supporting Tornado 6~~ AsyncioScopeManager based on contextvars and supporting Tornado 6 Dec 10, 2019

condorcet and others added 5 commits December 10, 2019 19:35

Update testbed/test_multiple_callbacks/README.md

474f4c6

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com>

Update testbed/test_nested_callbacks/README.md

c95c126

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com>

Update testbed/test_subtask_span_propagation/README.md

ccb2a5f

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com>

Fix typo in testbed docs

75f136c

Remove obsolete description from testbed docs

bd67157

yurishkuro approved these changes Dec 17, 2019

View reviewed changes

testbed/test_common_request_handler/README.md Outdated Show resolved Hide resolved

Update testbed/test_common_request_handler/README.md

a3913e4

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com>

yurishkuro merged commit 170f927 into opentracing:master Dec 17, 2019

alisonatwork mentioned this pull request May 21, 2020

Add support for Tornado 6 opentracing-contrib/python-tornado#10

Closed

libnoon mentioned this pull request Feb 25, 2021

ImportError: No module named 'tornado.stack_context #136

Open

		_SCOPE = ContextVar('scope')


		class ContextVarsScopeManager(ThreadLocalScopeManager):

AsyncioScopeManager based on contextvars and supporting Tornado 6 #118

AsyncioScopeManager based on contextvars and supporting Tornado 6 #118

Conversation

condorcet commented Apr 16, 2019

carlosalberto commented Apr 24, 2019

condorcet commented Apr 30, 2019

condorcet commented May 29, 2019

carlosalberto commented Jun 10, 2019

Jamim commented Jun 14, 2019

condorcet commented Jun 15, 2019

condorcet commented Jun 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Jul 11, 2019 • edited Loading

condorcet commented Jul 20, 2019

condorcet commented Jul 26, 2019

Jamim commented Aug 28, 2019

Jamim commented Aug 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Sep 9, 2019

condorcet commented Sep 9, 2019

condorcet commented Sep 23, 2019

Jamim commented Oct 10, 2019

rhilfers commented Nov 11, 2019

condorcet commented Nov 26, 2019

yurishkuro commented Nov 27, 2019

condorcet commented Nov 27, 2019

sheregeda commented Dec 3, 2019

nicholasamorim commented Dec 10, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto Jul 11, 2019 •

edited

Loading

carlosalberto commented Jul 11, 2019 •

edited

Loading

carlosalberto Sep 9, 2019 •

edited

Loading