Initial implementation of context #57
Conversation
|
Some questions about the context. These might have easy answers, I just don't know them: Do we need a context API instead of using Do we expect implementations to override |
|
IMHO the point of adding type annotations to API code is to make implementers' lives easy and give them the option to do type checking. The annotations here don't do much to describe the behavior of the class, add a lot of boilerplate code, and (at least in my experience) add significant friction while writing the code. |
| slot = self._slots[name] | ||
| slot.set(value) | ||
|
|
||
| def with_current_context( |
There was a problem hiding this comment.
would there be value in exposing a contextmanager like this as well?
There was a problem hiding this comment.
I don't know at this moment. I'm open to add it if we find it helpful.
Yes for now, although our direction is to use contextvars in the future.
There will be cases like this, for example https://github.com/census-instrumentation/opencensus-python/tree/master/contrib/opencensus-ext-gevent. |
|
|
||
| # user can propagate context explicitly | ||
| thread = Thread( | ||
| target=Context.with_current_context(work), |
There was a problem hiding this comment.
this helps clear up the value of with_current_context. Nice!
I feel like the need to add with_current_context will probably be a gotcha in many cases. It's too bad there isn't a way to make this something that is implicitly shared.
There was a problem hiding this comment.
When the day comes that contextvars or some built-in Python lib support such scenario, we can say goodbye to this Context class and never need to reinvent wheels again :)
|
|
||
| from opentelemetry.context import Context | ||
|
|
||
| class Span(object): |
There was a problem hiding this comment.
@c24t this should unblock your work on the actual span creation and tracer context update.
| self.parent, | ||
| )) | ||
|
|
||
| async def __aenter__(self): |
There was a problem hiding this comment.
This example only works on Python3.7+ due to the new async/await syntax.
We can explore more in #62.
| def call_with_current_context( | ||
| *args: 'object', | ||
| **kwargs: 'object', | ||
| ) -> 'object': |
There was a problem hiding this comment.
Since we don't expect users to bring their own context impl (as we do with trace and metrics) I think it's fine to lose the type annotations in this module, especially when they're this useless.
|
|
||
| if __name__ == '__main__': | ||
| asyncio.run(main()) | ||
| """ |
There was a problem hiding this comment.
How about adding this module to the sphinx docs?
There was a problem hiding this comment.
Sure, I'll send a follow up PR to add test cases and docs.
* Add TraceState * Fix review comments * Minor Fix, add TODOs * Use Map to preserve ordering * Add underscore for private methods & rename variable - Give more descriptive name than `s` to variable. - Private properties prefixed with an underscore for JavaScript users. - Private methods prefixed with an underscore for JavaScript users. * Remove TEST_ONLY methods * Add comment about TraceState class * Add unset method
This is based on the work in
opencensus-context.