Refactor context propagation to work with async #588
Conversation
Introduced the generic RuntimeContext.
Migrate execution_context to RuntimeContext
There was a problem hiding this comment.
The context changes LGTM, and this is a huge improvement over threadlocals. We still have some work to do to change how the context is used in the library (e.g. removing the tracer, measure to view map, etc.) and adding/using context managers as in your Span example, but this is a problem for another PR.
I think it's possible that this PR will introduce bugs in instrumented code because of the different behavior between 2 and 3, especially in code that touches thread-local storage or changes the default thread creation behavior (like we do in opencensus-ext-threading). We may want to figure out exactly how this is likely to break instrumented code and document it before releasing.
|
|
||
| from opencensus.common.runtime_context import RuntimeContext | ||
|
|
||
| RuntimeContext.register_slot('correlation_context', lambda: {}) |
There was a problem hiding this comment.
Nit: I think it's more idiomatic to use dict than lambda: {}, but your call.
There was a problem hiding this comment.
I picked explicitly lambda in order to tell ctor from a type name.
For example isinstance(x, Foo) and map(lambda x: Foo(x), range(10)).
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| from multiprocessing.dummy import Pool as ThreadPool |
There was a problem hiding this comment.
Why multiprocessing...ThreadPool instead of concurrent.futures.ThreadPoolExecutor?
There was a problem hiding this comment.
This former works with Python 2.7.
|
|
||
| class Slot(object): | ||
| def __init__(self, name, default): | ||
| import contextvars |
There was a problem hiding this comment.
I was trying to reduce the visibility of contextvars to avoid accidental use.
Alternatively we could use if/else and import at the top level (which seems harder to maintain?)
We need to take a closer look at opencensus-ext-threading and how we propagate across process (multi-processing). |
This is to merge #566 and #573 to master.