Add ScopeManager implementation for in-process propagation #24
Conversation
| - "2.7" | ||
|
|
||
| install: | ||
| - make bootstrap | ||
|
|
||
| script: | ||
| - make test | ||
|
|
||
| - make test lint |
There was a problem hiding this comment.
This could be another PR, but I needed that to ensure the CI runs the lint step.
There was a problem hiding this comment.
Actually, you could issue a PR right now with this small change and probably get it merged rather soon ;)
| def __init__(self): | ||
| self._tls_scope = threading.local() | ||
|
|
||
| def activate(self, span, finish_on_close=True): |
There was a problem hiding this comment.
finish_on_close is injected in underlying objects; I think it's a reasonable approach since it should be propagated till we reach a Scope.
| @@ -11,7 +12,7 @@ | |||
|
|
|||
| class BasicTracer(Tracer): | |||
|
|
|||
| def __init__(self, recorder=None, sampler=None): | |||
| def __init__(self, recorder=None, sampler=None, scope_manager=None): | |||
There was a problem hiding this comment.
It's reasonable defining a scope_manager in the constructor, since a Tracer should have one and only one ScopeManager for the rest of the execution. Switching it at runtime could lead to unexpected behaviors.
There was a problem hiding this comment.
I agree here with you. My only question is whether we should provide, on default, a noop ScopeManager by default or not. EDIT - oh, never mind, I saw you are setting it to ThreadLocalScopeManager by default right inside __init__(). Maybe set it right as scope_manager=ThreadLocalScopeManager() in the parameter list? That would be more explicit, I'd say...
| @@ -22,7 +22,8 @@ | |||
| platforms='any', | |||
| install_requires=[ | |||
| 'protobuf>=3.0.0b2.post2', | |||
| 'opentracing>=1.2.1,<1.3', | |||
| # TODO: pin the right version after the proposal has been merged on master | |||
| # 'opentracing>=1.2.1,<1.3', | |||
There was a problem hiding this comment.
This is a blocker before merging the PR.
| @@ -64,6 +90,12 @@ def start_span( | |||
| # TODO only the first reference is currently used | |||
| parent_ctx = references[0].referenced_context | |||
|
|
|||
| # retrieve the active SpanContext | |||
There was a problem hiding this comment.
if we pass child_of= kwarg, we ignore the ScopeManager, correct?
There was a problem hiding this comment.
In theory multiple parents will be supported - but currently, only one is used. So, yes, if we pass child_of, we ignore the current Scope.
| def is_parent(self, parent, span): | ||
| # use `Span` ids to check parenting | ||
| if parent is None: | ||
| return span.parent_id is None |
There was a problem hiding this comment.
Covers the case where is_parent(None, span) returns True if span is a root span.
|
@carlosalberto @tedsuo it would be great having a pass on this PR too to better understand the implementation details with the API defined in the other repo. |
| """Return the `Span` that's been scoped by this `Scope`.""" | ||
| return self._span | ||
|
|
||
| def close(self): |
There was a problem hiding this comment.
Hey - while porting our python-examples repository, I went through this code and wondered if there's a reason to have Scope.close() call ScopeManager.deactivate() (or similar) instead? That way we would reuse BasicScope lots more, I think - by putting specific stuff in ScopeManager.
That being said, I could prepare a PR if/as needed on this ;)
There was a problem hiding this comment.
I.e. specially since it would be nice (if possible), to separate BasicScope from the actual ThreadLocalScopeManager implementation ;)
|
An additional note: could we, in any case, expose |
|
Shall we close this PR, @palazzem ? |
|
@carlosalberto sure thing! Thanks for your efforts! |
Overview
This PR adds in-process context propagation to the reference implementation. It pairs with opentracing/opentracing-python#63 that is required to make it work. All implementation details should be discussed here, while API-related discussions should follow the other PR.
Details
ScopeandScopeManagerare implemented for in-process propagationScopeManager(and the only one implemented) uses a thread-local storage for in-process propagationstart_active()andstart_manual()are the new tracing API; both make use of the in-process propagation unlessignore_active_scopeflag is setScopeis automatically finished as default behavior, unlessfinish_on_closeflag is set toFalseTracerAPI compatibility is tested using the test suite (api_check.py) available in theopentracing-pythonpackage. No further tests are required in this package since theScopeManagerparenting is already tested.References