-
Notifications
You must be signed in to change notification settings - Fork 250
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
Introduce a generic context #566
Conversation
@@ -0,0 +1,26 @@ | |||
from opencensus.common.runtime_context import RuntimeContext | |||
|
|||
RuntimeContext.register_slot('current_span', None) |
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.
This is to demonstrate how we can leverage RuntimeContext to build trace/execution_context (tracer, current_span, attrs), tags/execution_context, W3C CorrelationContext and TraceContext. So all of them will benefit from the generic context in async and explicit threading (customer explicitly created threads and propagate the context manually) scenario.
In this way Dynatrace could also introduce an extension/exporter which registers its own context, without having to pollute span_context (e.g. if we want to propagate something within the process, but don't want to serialize/send across the wire).
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.
Would you expect e.g. the correlation context package to have another single module-level context object? Like:
correlation_context = RuntimeContext.register_slot('correlation_context', dict)
This makes sense to me, but thinking about the API: as a user, why use RuntimeContext
instead of contextvars.Context
directly?
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 idea, I will definitely try this approach.
Regarding letting user to use contextvars.Context
, that is our direction - we don't want to introduce another context if the runtime already has it.
For now, given we need to support Python 2.7/3.4-3.6, probably we need to backport? I checked contextvars and didn't see a good way to backport as the logic was implemented as a native module (in C rather than Python code).
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.
https://pypi.org/project/contextvars/ for 3.6 we have contextvars looks like.
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.
The direction and names look great to me.
Some questions about the future of this work: Should we have a common context API that we include in the spec, and that all language clients implement? If so, what does the API look like, and what do we need that isn't offered in existing implementations (contextvars
, grpc.Context
, go's context.Context
, etc.)?
@@ -0,0 +1,26 @@ | |||
from opencensus.common.runtime_context import RuntimeContext | |||
|
|||
RuntimeContext.register_slot('current_span', None) |
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.
Would you expect e.g. the correlation context package to have another single module-level context object? Like:
correlation_context = RuntimeContext.register_slot('correlation_context', dict)
This makes sense to me, but thinking about the API: as a user, why use RuntimeContext
instead of contextvars.Context
directly?
contrib/opencensus-context/opencensus/common/runtime_context/__init__.py
Outdated
Show resolved
Hide resolved
contrib/opencensus-context/opencensus/common/runtime_context/__init__.py
Outdated
Show resolved
Hide resolved
contrib/opencensus-context/opencensus/common/runtime_context/__init__.py
Outdated
Show resolved
Hide resolved
contrib/opencensus-context/opencensus/common/runtime_context/__init__.py
Outdated
Show resolved
Hide resolved
cc @bogdandrutu, who championed using the gRPC context in java and may have opinions about the API. Other reviewers: see the python We currently use threadlocals to store the execution context, which fails predictably with asyncio (and other event loop frameworks like tornado). Moving to |
contextvars are not available everywhere? Can we just use this? |
contextvars was introduced in the latest version of Python (3.7). For old versions of Python, our best bet is Thread Local (and it doesn't seem that we can backport such feature, since contextvars was implemented as a native pyd module). |
contrib/opencensus-context/opencensus/common/runtime_context/__init__.py
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,26 @@ | |||
from opencensus.common.runtime_context import RuntimeContext | |||
|
|||
RuntimeContext.register_slot('current_span', None) |
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.
https://pypi.org/project/contextvars/ for 3.6 we have contextvars looks like.
Introduced the generic RuntimeContext.
Introduced the generic RuntimeContext.
Introduced the generic RuntimeContext.
This is part of the #564 effort.
The
RuntimeContext
class provides a consistent way of manipulating/debugging context for both TLS and asyncio environment.Please take a look at the examples first. I will start to add documents and test cases once we're okay on the general direction/naming/interface.
@c24t