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

Refactor execution_context to use RuntimeContext #573

Merged
merged 3 commits into from
Mar 22, 2019
Merged

Refactor execution_context to use RuntimeContext #573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4344007
migrate trace/execution_context to RuntimeContext
reyang Mar 21, 2019
2966fff
migrate stats and tags execution_context
reyang Mar 21, 2019
035c520
fix missing return
reyang Mar 22, 2019
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
13 changes: 8 additions & 5 deletions opencensus/stats/execution_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,21 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import threading
from opencensus.common.runtime_context import RuntimeContext

_thread_local = threading.local()
_measure_to_view_map_slot = RuntimeContext.register_slot(
'measure_to_view_map',
Copy link
Member

@c24t c24t Mar 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think it would only make sense to store measure_to_view_map in the context if we wanted to e.g. register views in one thread (or async coroutine) and not another. If we can stop storing this in the context we can lose this module completely. FWIW other language clients don't have a stats execution context, and java uses a singleton for the measure to view map.

Definitely out of scope for this PR, just something to keep in mind as we're formalizing the context API.

lambda: {})


def get_measure_to_view_map():
return getattr(_thread_local, 'measure_to_view_map', {})
return RuntimeContext.measure_to_view_map


def set_measure_to_view_map(measure_to_view_map):
setattr(_thread_local, 'measure_to_view_map', measure_to_view_map)
RuntimeContext.measure_to_view_map = measure_to_view_map


def clear():
_thread_local.__dict__.clear()
"""Clear the context, used in test."""
_measure_to_view_map_slot.clear()
12 changes: 6 additions & 6 deletions opencensus/tags/execution_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,19 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import threading
from opencensus.common.runtime_context import RuntimeContext

_thread_local = threading.local()
_current_tag_map_slot = RuntimeContext.register_slot('current_tag_map', None)


def get_current_tag_map():
return getattr(_thread_local, 'current_tag_map', None)
return RuntimeContext.current_tag_map


def set_current_tag_map(current_tag_map):
setattr(_thread_local, 'current_tag_map', current_tag_map)
RuntimeContext.current_tag_map = current_tag_map
Copy link
Member

Choose a reason for hiding this comment

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

Eventually get/set needs to be replaced with a context manager that restores the old tag map on exit. This would also let us lose clear.



def clear():
"""Clear the thread local, used in test."""
_thread_local.__dict__.clear()
"""Clear the context, used in test."""
_current_tag_map_slot.clear()
62 changes: 25 additions & 37 deletions opencensus/trace/execution_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,81 +12,69 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import threading

from opencensus.common.runtime_context import RuntimeContext
from opencensus.trace.tracers import noop_tracer

_thread_local = threading.local()
_attrs_slot = RuntimeContext.register_slot('attrs', lambda: {})
_current_span_slot = RuntimeContext.register_slot('current_span', None)
_tracer_slot = RuntimeContext.register_slot('tracer', noop_tracer.NoopTracer())


def get_opencensus_tracer():
"""Get the opencensus tracer from thread local."""
return getattr(_thread_local, 'tracer', noop_tracer.NoopTracer())
"""Get the opencensus tracer from runtime context."""
return RuntimeContext.tracer


def set_opencensus_tracer(tracer):
"""Add the tracer to thread local."""
setattr(_thread_local, 'tracer', tracer)
"""Add the tracer to runtime context."""
RuntimeContext.tracer = tracer


def set_opencensus_attr(attr_key, attr_value):
# If there is no attrs, initialize it to empty dict.
attrs = getattr(_thread_local, 'attrs', {})

attrs = RuntimeContext.attrs.copy()
Copy link
Member

Choose a reason for hiding this comment

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

Setting attrs on the context directly instead of the current span might be unique to the python client. It looks like this is used to:

  1. store the host blacklist in the flask and django integrations
  2. store the current span ID in the httplib integration
  3. store the request object in the django integration

(1) seems like it could be static config instead, (2) seems like it could use the current span, and (3) seems to be used to get the trace headers from the request object, which we might be able to do at request time instead.

Hopefully we can remove this too, and only set attrs on the current context.

attrs[attr_key] = attr_value

setattr(_thread_local, 'attrs', attrs)
RuntimeContext.attrs = attrs


def set_opencensus_attrs(attrs):
setattr(_thread_local, 'attrs', attrs)
RuntimeContext.attrs = attrs


def get_opencensus_attr(attr_key):
attrs = getattr(_thread_local, 'attrs', None)

if attrs is not None:
return attrs.get(attr_key)

return None
return RuntimeContext.attrs.get(attr_key)


def get_opencensus_attrs():
return getattr(_thread_local, 'attrs', None)
return RuntimeContext.attrs


def get_current_span():
return getattr(_thread_local, 'current_span', None)
return RuntimeContext.current_span


def set_current_span(current_span):
setattr(_thread_local, 'current_span', current_span)
RuntimeContext.current_span = current_span


def get_opencensus_full_context():
_tracer = get_opencensus_tracer()
_span = get_current_span()
_attrs = get_opencensus_attrs()
return _tracer, _span, _attrs
attrs = RuntimeContext.attrs
current_span = RuntimeContext.current_span
tracer = RuntimeContext.tracer
return tracer, current_span, attrs


def set_opencensus_full_context(tracer, span, attrs):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This set_opencensus_full_context probably should be removed (in another PR):

  1. It is not the "full context" as it doesn't cover tags.
  2. RuntimeContext.apply provides the intended functionality, which can be used by propagating context to explicit threads.

set_opencensus_tracer(tracer)
set_current_span(span)
if not attrs:
set_opencensus_attrs({})
else:
set_opencensus_attrs(attrs)
set_opencensus_attrs(attrs or {})


def clean():
setattr(_thread_local, 'attrs', {})
if hasattr(_thread_local, 'current_span'):
delattr(_thread_local, 'current_span')
if hasattr(_thread_local, 'tracer'):
delattr(_thread_local, 'tracer')
_attrs_slot.clear()
_current_span_slot.clear()
_tracer_slot.clear()


def clear():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@c24t I think having a test-only method exposed publicly is evil. I plan to remove it or make it "private". Please let me know your thoughts.

BTW, _thread_local.__dict__.clear() will remove Thread-local storage introduced by other components (e.g. stats), not sure if this is intended. @liyanhui1228 might have some background on this?

Copy link
Member

Choose a reason for hiding this comment

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

I agree this is a cardinal sin. We may be able to remove this entirely soon -- if we stop storing the tracer and measure to view map in the context and control span and tag lifetimes with context managers then hopefully tests won't pollute the context (and should be able to run in parallel!).

"""Clear the thread local, used in test."""
_thread_local.__dict__.clear()
"""Clear the context, used in test."""
clean()
1 change: 1 addition & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
include_package_data=True,
long_description=open('README.rst').read(),
install_requires=[
'opencensus-context == 0.2.dev0',
'google-api-core >= 1.0.0, < 2.0.0',
],
extras_require={},
Expand Down