Skip to content
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

Add ScopeManager implementation for in-process propagation #24

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .flake8
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[flake8]
exclude =
basictracer/wire_pb2.py
5 changes: 2 additions & 3 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
language: python
python:
python:
- "2.7"

install:
- make bootstrap

script:
- make test

- make test lint
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be another PR, but I needed that to ensure the CI runs the lint step.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, you could issue a PR right now with this small change and probably get it merged rather soon ;)

2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ clean-test:
rm -fr htmlcov/

lint:
flake8 $(project) tests
flake8 --config=.flake8 $(project) tests

test:
$(pytest) $(test_args)
Expand Down
55 changes: 55 additions & 0 deletions basictracer/scope.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# Copyright (c) 2017 The OpenTracing Authors.
#
# Permission is hereby granted, free of charge, to any person obtaining a copy
# of this software and associated documentation files (the "Software"), to deal
# in the Software without restriction, including without limitation the rights
# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
# copies of the Software, and to permit persons to whom the Software is
# furnished to do so, subject to the following conditions:
#
# The above copyright notice and this permission notice shall be included in
# all copies or substantial portions of the Software.
#
# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

from __future__ import absolute_import

from opentracing import Scope


class BasicScope(Scope):
"""BasicScope is the implementation of `opentracing.Scope`"""

def __init__(self, manager, span, finish_on_close=True):
"""Initialize a `Scope` for the given `Span` object

:param span: the `Span` used for this `Scope`
:param finish_on_close: whether span should automatically be
finished when `Scope#close()` is called
"""
self._manager = manager
self._span = span
self._finish_on_close = finish_on_close
self._to_restore = manager.active()

def span(self):
"""Return the `Span` that's been scoped by this `Scope`."""
return self._span

def close(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I.e. specially since it would be nice (if possible), to separate BasicScope from the actual ThreadLocalScopeManager implementation ;)

"""Finish the `Span` when the `Scope` context ends, unless
`finish_on_close` has been set.
"""
if self._manager.active() is not self:
return

if self._finish_on_close:
self._span.finish()

setattr(self._manager._tls_scope, 'active', self._to_restore)
63 changes: 63 additions & 0 deletions basictracer/scope_manager.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# Copyright (c) 2017 The OpenTracing Authors.
#
# Permission is hereby granted, free of charge, to any person obtaining a copy
# of this software and associated documentation files (the "Software"), to deal
# in the Software without restriction, including without limitation the rights
# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
# copies of the Software, and to permit persons to whom the Software is
# furnished to do so, subject to the following conditions:
#
# The above copyright notice and this permission notice shall be included in
# all copies or substantial portions of the Software.
#
# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

from __future__ import absolute_import

import threading

from opentracing import ScopeManager

from .scope import BasicScope


class ThreadLocalScopeManager(ScopeManager):
"""ScopeManager implementation that stores the current active `Scope`
in a thread-local storage
"""
def __init__(self):
self._tls_scope = threading.local()

def activate(self, span, finish_on_close=True):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Make a `Span` instance active.

:param span: the `Span` that should become active
:param finish_on_close: whether span should automatically be
finished when `Scope#close()` is called

:return: a `Scope` instance to control the end of the active period for
the `Span`. It is a programming error to neglect to call
`Scope#close()` on the returned instance. By default, `Span` will
automatically be finished when `Scope#close()` is called.
"""
scope = BasicScope(self, span, finish_on_close=finish_on_close)
setattr(self._tls_scope, 'active', scope)
return scope

def active(self):
"""Return the currently active `Scope` which can be used to access the
currently active `Scope#span()`.

If there is a non-null `Scope`, its wrapped `Span` becomes an implicit
parent of any newly-created `Span` at `Tracer#start_active()`
time.

:return: the `Scope` that is active, or `None` if not available.
"""
return getattr(self._tls_scope, 'active', None)
55 changes: 52 additions & 3 deletions basictracer/tracer.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import opentracing
from opentracing import Format, Tracer
from opentracing import UnsupportedFormatException
from .scope_manager import ThreadLocalScopeManager
from .context import SpanContext
from .recorder import SpanRecorder, DefaultSampler
from .span import BasicSpan
Expand All @@ -11,7 +12,7 @@

class BasicTracer(Tracer):

def __init__(self, recorder=None, sampler=None):
def __init__(self, recorder=None, sampler=None, scope_manager=None):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@carlosalberto carlosalberto Nov 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

"""Initialize a BasicTracer instance.

Note that the returned BasicTracer has *no* propagators registered. The
Expand All @@ -26,6 +27,8 @@ def __init__(self, recorder=None, sampler=None):
super(BasicTracer, self).__init__()
self.recorder = NoopRecorder() if recorder is None else recorder
self.sampler = DefaultSampler(1) if sampler is None else sampler
self._scope_manager = ThreadLocalScopeManager() \
if scope_manager is None else scope_manager
self._propagators = {}

def register_propagator(self, format, propagator):
Expand All @@ -44,13 +47,36 @@ def register_required_propagators(self):
self.register_propagator(Format.HTTP_HEADERS, TextPropagator())
self.register_propagator(Format.BINARY, BinaryPropagator())

def start_span(
def start_active(self,
operation_name=None,
child_of=None,
references=None,
tags=None,
start_time=None,
ignore_active_scope=False,
finish_on_close=True):

# create a new Span
span = self.start_manual(
operation_name=operation_name,
child_of=child_of,
references=references,
tags=tags,
start_time=start_time,
ignore_active_scope=ignore_active_scope,
)

return self.scope_manager.activate(span,
finish_on_close=finish_on_close)

def start_manual(
self,
operation_name=None,
child_of=None,
references=None,
tags=None,
start_time=None):
start_time=None,
ignore_active_scope=False):

start_time = time.time() if start_time is None else start_time

Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we pass child_of= kwarg, we ignore the ScopeManager, correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if not ignore_active_scope and parent_ctx is None:
scope = self.scope_manager.active()
if scope is not None:
parent_ctx = scope.span().context

# Assemble the child ctx
ctx = SpanContext(span_id=generate_id())
if parent_ctx is not None:
Expand All @@ -84,6 +116,23 @@ def start_span(
tags=tags,
start_time=start_time)

def start_span(
self,
operation_name=None,
child_of=None,
references=None,
tags=None,
start_time=None):
"""Deprecated: use `start_manual()` or `start_active()` instead."""
return self.start_manual(
operation_name=operation_name,
child_of=child_of,
references=references,
tags=tags,
start_time=start_time,
ignore_active_scope=True,
)

def inject(self, span_context, format, carrier):
if format in self._propagators:
self._propagators[format].inject(span_context, carrier)
Expand Down
3 changes: 3 additions & 0 deletions requirements-test.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,6 @@
-r requirements.txt

-e .[tests]

# TODO: using the latest proposal version; remove that after it has been merged upstream
-e git+https://github.com/palazzem/opentracing-python.git@palazzem/scope-manager#egg=opentracing-1.3.0
3 changes: 2 additions & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a blocker before merging the PR.

'six>=1.10.0,<2.0',
],
extras_require={
Expand Down
7 changes: 7 additions & 0 deletions tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,10 @@ def tracer(self):

def check_baggage_values(self):
return True

def is_parent(self, parent, span):
# use `Span` ids to check parenting
if parent is None:
return span.parent_id is None
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covers the case where is_parent(None, span) returns True if span is a root span.


return parent.context.span_id == span.parent_id
35 changes: 35 additions & 0 deletions tests/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Copyright (c) 2017 The OpenTracing Authors.
#
# Permission is hereby granted, free of charge, to any person obtaining a copy
# of this software and associated documentation files (the "Software"), to deal
# in the Software without restriction, including without limitation the rights
# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
# copies of the Software, and to permit persons to whom the Software is
# furnished to do so, subject to the following conditions:
#
# The above copyright notice and this permission notice shall be included in
# all copies or substantial portions of the Software.
#
# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

from __future__ import absolute_import

from unittest import TestCase

from basictracer import BasicTracer
from basictracer.recorder import InMemoryRecorder


class TracerTestCase(TestCase):
"""Common TestCase to avoid duplication"""

def setUp(self):
# initialize an in-memory tracer
self.recorder = InMemoryRecorder()
self.tracer = BasicTracer(recorder=self.recorder)