-
Notifications
You must be signed in to change notification settings - Fork 624
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
Moving contextvars and threadlocal context implementations to the API #419
Moving contextvars and threadlocal context implementations to the API #419
Conversation
While keeping the flexibility of the interface to support additional implementations of the RuntimeContext, moving the ContextVarsRuntimeContext and ThreadLocalRuntimeContext into the API ensures a useful context implementation for users without requiring additional configuration. This changes checks the version number to determine which implementation to use, unless it is overridden by the OPENTELEMETRY_CONTEXT environment variable. Removing the DefaultRuntimeContext as part of this change. Signed-off-by: Alex Boten <[email protected]>
I think we should follow what Java does in open-telemetry/opentelemetry-java#720 and partially has already done (https://github.com/open-telemetry/opentelemetry-java/tree/master/context_prop): Move the context to its own distribution that depends on nothing and on which API depends. |
@Oberon00 I like this, were you thinking another top level directory for this? |
Signed-off-by: Alex Boten <[email protected]>
@Oberon00 are you talking about moving the abstract |
Signed-off-by: Alex Boten <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #419 +/- ##
==========================================
- Coverage 88.3% 88.19% -0.11%
==========================================
Files 42 41 -1
Lines 2060 2042 -18
Branches 236 234 -2
==========================================
- Hits 1819 1801 -18
Misses 170 170
Partials 71 71
Continue to review full report at Codecov.
|
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.
Looks great!
install_requires=["typing; python_version<'3.5'"], | ||
install_requires=[ | ||
"typing; python_version<'3.5'", | ||
"aiocontextvars; python_version<'3.7' and python_version>='3.5'", |
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.
Good call on the conditional requirements.
opentelemetry-api/src/opentelemetry/context/threadlocal_context.py
Outdated
Show resolved
Hide resolved
context.set_current(self.previous_context) | ||
|
||
@patch( | ||
"opentelemetry.context._RUNTIME_CONTEXT", ContextVarsRuntimeContext() # type: ignore |
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'm not sure why the CLI black
isn't wrapping these. FWIW black.format_file_contents
via the vim plugin does.
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.
It could be line length isn't being honored. I see it in the pyproject.toml, but IIRC my editor wasn't always honoring that configuration either.
I'd assume all three. This is what we did with the opencensus-context package too. |
Exactly. It is illuminating to read OTEP 66, in which the context API was introduced, especially the motivation:
So one could install |
|
||
if (3, 5, 3) <= version_info < (3, 7): | ||
import aiocontextvars # type: ignore # pylint:disable=unused-import | ||
import aiocontextvars # type: ignore # pylint:disable=unused-import,import-error |
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.
Please note that without this patch in aiocontextvars fantix/aiocontextvars#66 behavior of async code in python<3.7 differs from original contextvars in python 3.7.
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.
Hello @condorcet
What specific behavior difference are you referring to?
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.
Hi @ocelotl !
Even with aiocontextvars call_soon
and similar methods of loop don't create their own context. Hence such scheduled callbacks are "stealing" outer context.
For example:
import asyncio
from contextvars import ContextVar
var = ContextVar('var')
def callback(name):
print(f'{name} var before: {var.get()}')
var.set(name)
print(f'{name} var after: {var.get()}')
if __name__ == '__main__':
var.set('root')
loop = asyncio.get_event_loop()
loop.call_soon(callback, 'callback_1')
loop.call_soon(callback, 'callback_2')
asyncio.get_event_loop().run_forever()
In python 3.7 (proper behavior):
callback_1 var before: root
callback_1 var after: callback_1
callback_2 var before: root
callback_2 var after: callback_2
In python 3.6 (with aiocontextvars):
callback_1 var before: root
callback_1 var after: callback_1
callback_2 var before: callback_1
callback_2 var after: callback_2
Here you can see that first callback changed context var, and next callback used modified value rather than initial.
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.
Thanks for clarifying @condorcet, I'll open an issue to track this.
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.
Opened #436
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.
nice!
@@ -84,9 +85,14 @@ def get_current() -> Context: | |||
if _RUNTIME_CONTEXT is None: | |||
# FIXME use a better implementation of a configuration manager to avoid having | |||
# to get configuration values straight from environment variables | |||
if version_info < (3, 5): | |||
# contextvars are not supported in 3.4, use thread-local storage | |||
default_context = "threadlocal_context" |
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.
does this mean that async context won't work for python3.4?
context.set_current(self.previous_context) | ||
|
||
@patch( | ||
"opentelemetry.context._RUNTIME_CONTEXT", ContextVarsRuntimeContext() # type: ignore |
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.
It could be line length isn't being honored. I see it in the pyproject.toml, but IIRC my editor wasn't always honoring that configuration either.
…t.py Co-Authored-By: Chris Kleinknecht <[email protected]>
There was a good conversation in the Gitter around breaking this out into it's own package now. @c24t points that there will be complications around version mismatches. E.g. today, if someone hard pins the API / SDK, they will also have to be aware that there is a context package that must also be hard pinned. so consumers will probably hard pin API / SDK, pick up a backwards-incompatible context change (since they're unaware of it's existence or role), then have to go figure out what went wrong. Probably good enough motivation to not split the context package out until it becomes more stable. |
@toumorokoshi, @c24t: With the same argument, we shouldn't split the SDK from the API. |
Yes, I think ideally, we should have kept it together as long as we could have, until we had a strong need to (e.g. someone started implementing their own SDK). But doing it from the start I think isn't the end of the world, it also sets the precedence that both SDK and API packages should be called out specifically. I could be convinced either way, hopefully context changes are few and far between after the first cycle or two. But I don't intend to be a voice of authority on this one, I'll defer to those who are more directly contributing, and also have a need for a separate context object. |
While keeping the flexibility of the interface to support additional implementations of the RuntimeContext, moving the
ContextVarsRuntimeContext
andThreadLocalRuntimeContext
into the API ensures a useful context implementation for users without requiring additional configuration. This changes checks the version number to determine which implementation to use, unless it is overridden by theOPENTELEMETRY_CONTEXT
environment variable. Removing theDefaultRuntimeContext
as part of this change.The discussion suggesting this change can be found here