Refactor execution_context to use RuntimeContext #573
Conversation
| delattr(_thread_local, 'tracer') | ||
| _attrs_slot.clear() | ||
| _current_span_slot.clear() | ||
| _tracer_slot.clear() | ||
|
|
||
|
|
||
| def clear(): |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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!).
reyang
changed the title
| attrs = RuntimeContext.attrs | ||
| current_span = RuntimeContext.current_span | ||
| tracer = RuntimeContext.tracer | ||
| return tracer, current_span, attrs | ||
|
|
||
|
|
||
| def set_opencensus_full_context(tracer, span, attrs): |
There was a problem hiding this comment.
This set_opencensus_full_context probably should be removed (in another PR):
- It is not the "full context" as it doesn't cover tags.
RuntimeContext.applyprovides the intended functionality, which can be used by propagating context to explicit threads.
| delattr(_thread_local, 'tracer') | ||
| _attrs_slot.clear() | ||
| _current_span_slot.clear() | ||
| _tracer_slot.clear() | ||
|
|
||
|
|
||
| def clear(): |
There was a problem hiding this comment.
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!).
|
|
||
| _thread_local = threading.local() | ||
| _measure_to_view_map_slot = RuntimeContext.register_slot( | ||
| 'measure_to_view_map', |
There was a problem hiding this comment.
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.
|
|
||
|
|
||
| def set_current_tag_map(current_tag_map): | ||
| setattr(_thread_local, 'current_tag_map', current_tag_map) | ||
| RuntimeContext.current_tag_map = current_tag_map |
There was a problem hiding this comment.
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.
| # If there is no attrs, initialize it to empty dict. | ||
| attrs = getattr(_thread_local, 'attrs', {}) | ||
|
|
||
| attrs = RuntimeContext.attrs.copy() |
There was a problem hiding this comment.
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:
- store the host blacklist in the flask and django integrations
- store the current span ID in the httplib integration
- 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.
Migrate execution_context to RuntimeContext
Migrate execution_context to RuntimeContext
Migrate execution_context to RuntimeContext
This is part of the #564 effort.
Today we have
trace/execution_context,tags/execution_contextandstats/executino_contextbuilt on top of Thread-local Storage, which I plan to refactor.This PR simply refactored execution_context from Thread-local Storage to RuntimeContext (introduced in #566).