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.

Sign up for GitHub

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

Jump to bottom

Moving contextvars and threadlocal context implementations to the API #419

Merged
merged 4 commits into from
Feb 18, 2020
Merged

Moving contextvars and threadlocal context implementations to the API #419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
90378f4
Moving context implementations to the API
Feb 14, 2020
253a969
adding python_version
Feb 14, 2020
39e0a19
fix lint, update required version
Feb 14, 2020
57b214e
Update opentelemetry-api/src/opentelemetry/context/threadlocal_contex…
codeboten Feb 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions opentelemetry-api/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@
include_package_data=True,
long_description=open("README.rst").read(),
long_description_content_type="text/x-rst",
install_requires=["typing; python_version<'3.5'"],
install_requires=[
"typing; python_version<'3.5'",
"aiocontextvars; python_version<'3.7' and python_version>='3.5'",
Copy link
Member

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.

],
extras_require={},
license="Apache-2.0",
package_dir={"": "src"},
Expand All @@ -58,9 +61,12 @@
zip_safe=False,
entry_points={
"opentelemetry_context": [
"default_context = "
"opentelemetry.context.default_context:"
"DefaultRuntimeContext",
"contextvars_context = "
"opentelemetry.context.contextvars_context:"
"ContextVarsRuntimeContext",
"threadlocal_context = "
"opentelemetry.context.threadlocal_context:"
"ThreadLocalRuntimeContext",
]
},
)
8 changes: 7 additions & 1 deletion opentelemetry-api/src/opentelemetry/context/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import logging
import typing
from os import environ
from sys import version_info

from pkg_resources import iter_entry_points

Expand Down Expand Up @@ -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"
Copy link
Member

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?

else:
default_context = "contextvars_context"

configured_context = environ.get(
"OPENTELEMETRY_CONTEXT", "default_context"
"OPENTELEMETRY_CONTEXT", default_context
) # type: str
try:
_RUNTIME_CONTEXT = next(
Expand Down
2 changes: 1 addition & 1 deletion ...elemetry/sdk/context/aiocontextvarsfix.py → ...pentelemetry/context/aiocontextvarsfix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def _get_event_loop():
asyncio._set_running_loop = asyncio.events._set_running_loop

# noinspection PyUnresolvedReferences
import aiocontextvars # pylint: disable=unused-import,wrong-import-position # noqa # isort:skip
import aiocontextvars # pylint: disable=import-error,unused-import,wrong-import-position # noqa # isort:skip


def _run_coroutine_threadsafe(coro, loop):
Expand Down
7 changes: 3 additions & 4 deletions ...emetry/sdk/context/contextvars_context.py → ...ntelemetry/context/contextvars_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,13 @@
from contextvars import ContextVar
from sys import version_info

from opentelemetry.context import Context
from opentelemetry.context.context import RuntimeContext
from opentelemetry.context.context import Context, RuntimeContext

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
Copy link

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.

Copy link
Contributor

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?

Copy link

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #436


elif (3, 4) < version_info <= (3, 5, 2):
import opentelemetry.sdk.context.aiocontextvarsfix # pylint:disable=unused-import
import opentelemetry.context.aiocontextvarsfix # pylint:disable=unused-import


class ContextVarsRuntimeContext(RuntimeContext):
Expand Down
34 changes: 0 additions & 34 deletions opentelemetry-api/src/opentelemetry/context/default_context.py
View file
Open in desktop

This file was deleted.

7 changes: 5 additions & 2 deletions ...emetry/sdk/context/threadlocal_context.py → ...ntelemetry/context/threadlocal_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

import threading

from opentelemetry.context import Context, RuntimeContext
from opentelemetry.context.context import Context, RuntimeContext


class ThreadLocalRuntimeContext(RuntimeContext):
Expand All @@ -38,7 +38,10 @@ def get_current(self) -> Context:
setattr(
self._current_context, self._CONTEXT_KEY, Context(),
)
return getattr(self._current_context, self._CONTEXT_KEY)
context = getattr(
self._current_context, self._CONTEXT_KEY
) # type: Context
return context


__all__ = ["ThreadLocalRuntimeContext"]
68 changes: 68 additions & 0 deletions opentelemetry-api/tests/context/test_contextvars_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# Copyright 2020, OpenTelemetry Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import unittest
from unittest.mock import patch

from opentelemetry import context

try:
import contextvars # pylint: disable=unused-import
from opentelemetry.context.contextvars_context import (
ContextVarsRuntimeContext,
)
except ImportError:
raise unittest.SkipTest("contextvars not available")


def do_work() -> None:
context.set_current(context.set_value("say", "bar"))


class TestContextVarsContext(unittest.TestCase):
def setUp(self):
self.previous_context = context.get_current()

def tearDown(self):
context.set_current(self.previous_context)

@patch(
"opentelemetry.context._RUNTIME_CONTEXT", ContextVarsRuntimeContext() # type: ignore
Copy link
Member

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.

Copy link
Member

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.

)
def test_context(self):
self.assertIsNone(context.get_value("say"))
empty = context.get_current()
second = context.set_value("say", "foo")

self.assertEqual(context.get_value("say", context=second), "foo")

do_work()
self.assertEqual(context.get_value("say"), "bar")
third = context.get_current()

self.assertIsNone(context.get_value("say", context=empty))
self.assertEqual(context.get_value("say", context=second), "foo")
self.assertEqual(context.get_value("say", context=third), "bar")

@patch(
"opentelemetry.context._RUNTIME_CONTEXT", ContextVarsRuntimeContext() # type: ignore
)
def test_set_value(self):
first = context.set_value("a", "yyy")
second = context.set_value("a", "zzz")
third = context.set_value("a", "---", first)
self.assertEqual("yyy", context.get_value("a", context=first))
self.assertEqual("zzz", context.get_value("a", context=second))
self.assertEqual("---", context.get_value("a", context=third))
self.assertEqual(None, context.get_value("a"))
8 changes: 3 additions & 5 deletions ...lemetry-sdk/tests/context/test_context.py → ...tests/context/test_threadlocal_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@
from unittest.mock import patch

from opentelemetry import context
from opentelemetry.sdk.context.threadlocal_context import (
ThreadLocalRuntimeContext,
)
from opentelemetry.context.threadlocal_context import ThreadLocalRuntimeContext


def do_work() -> None:
Expand All @@ -33,7 +31,7 @@ def tearDown(self):
context.set_current(self.previous_context)

@patch(
"opentelemetry.context._RUNTIME_CONTEXT", ThreadLocalRuntimeContext()
"opentelemetry.context._RUNTIME_CONTEXT", ThreadLocalRuntimeContext() # type: ignore
)
def test_context(self):
self.assertIsNone(context.get_value("say"))
Expand All @@ -51,7 +49,7 @@ def test_context(self):
self.assertEqual(context.get_value("say", context=third), "bar")

@patch(
"opentelemetry.context._RUNTIME_CONTEXT", ThreadLocalRuntimeContext()
"opentelemetry.context._RUNTIME_CONTEXT", ThreadLocalRuntimeContext() # type: ignore
)
def test_set_value(self):
first = context.set_value("a", "yyy")
Expand Down
12 changes: 1 addition & 11 deletions opentelemetry-sdk/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
include_package_data=True,
long_description=open("README.rst").read(),
long_description_content_type="text/x-rst",
install_requires=["opentelemetry-api==0.4.dev0", "aiocontextvars"],
install_requires=["opentelemetry-api==0.4.dev0"],
extras_require={},
license="Apache-2.0",
package_dir={"": "src"},
Expand All @@ -56,14 +56,4 @@
"/tree/master/opentelemetry-sdk"
),
zip_safe=False,
entry_points={
"opentelemetry_context": [
"contextvars_context = "
"opentelemetry.sdk.context.contextvars_context:"
"ContextVarsRuntimeContext",
"threadlocal_context = "
"opentelemetry.sdk.context.threadlocal_context:"
"ThreadLocalRuntimeContext",
]
},
)
44 changes: 1 addition & 43 deletions opentelemetry-sdk/tests/context/test_asyncio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

try:
import contextvars # pylint: disable=unused-import
from opentelemetry.sdk.context.contextvars_context import (
from opentelemetry.context.contextvars_context import (
ContextVarsRuntimeContext,
)
except ImportError:
Expand Down Expand Up @@ -53,10 +53,6 @@ def stop_loop_when(loop, cond_func, timeout=5.0):
loop.call_later(0.1, stop_loop_when, loop, cond_func, timeout)


def do_work() -> None:
context.set_current(context.set_value("say", "bar"))


class TestAsyncio(unittest.TestCase):
@asyncio.coroutine
def task(self, name):
Expand Down Expand Up @@ -114,41 +110,3 @@ def test_with_asyncio(self):
if span is expected_parent:
continue
self.assertEqual(span.parent, expected_parent)


class TestContextVarsContext(unittest.TestCase):
def setUp(self):
self.previous_context = context.get_current()

def tearDown(self):
context.set_current(self.previous_context)

@patch(
"opentelemetry.context._RUNTIME_CONTEXT", ContextVarsRuntimeContext()
)
def test_context(self):
self.assertIsNone(context.get_value("say"))
empty = context.get_current()
second = context.set_value("say", "foo")

self.assertEqual(context.get_value("say", context=second), "foo")

do_work()
self.assertEqual(context.get_value("say"), "bar")
third = context.get_current()

self.assertIsNone(context.get_value("say", context=empty))
self.assertEqual(context.get_value("say", context=second), "foo")
self.assertEqual(context.get_value("say", context=third), "bar")

@patch(
"opentelemetry.context._RUNTIME_CONTEXT", ContextVarsRuntimeContext()
)
def test_set_value(self):
first = context.set_value("a", "yyy")
second = context.set_value("a", "zzz")
third = context.set_value("a", "---", first)
self.assertEqual("yyy", context.get_value("a", context=first))
self.assertEqual("zzz", context.get_value("a", context=second))
self.assertEqual("---", context.get_value("a", context=third))
self.assertEqual(None, context.get_value("a"))
Loading