From 90226839e2c919c96e3d8ff0f254b636879cb738 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Mon, 13 Nov 2017 09:53:57 +0100 Subject: [PATCH 01/17] add Scope interface --- opentracing/scope.py | 73 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_scope.py | 53 ++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+) create mode 100644 opentracing/scope.py create mode 100644 tests/test_scope.py diff --git a/opentracing/scope.py b/opentracing/scope.py new file mode 100644 index 0000000..a9c1b41 --- /dev/null +++ b/opentracing/scope.py @@ -0,0 +1,73 @@ +# 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 + + +class Scope(object): + """ + A `Scope` formalizes the activation and deactivation of a `Span`, usually + from a CPU standpoint. Many times a `Span` will be extant (in that + `Span#finish()` has not been called) despite being in a non-runnable state + from a CPU/scheduler standpoint. For instance, a `Span` representing the + client side of an RPC will be unfinished but blocked on IO while the RPC is + still outstanding. A `Scope` defines when a given `Span` is scheduled + and on the path. + """ + def __init__(self, span): + """ + Initialize a `Scope` for the given `Span` object + + :param span: the `Span` used for this `Scope` + """ + # TODO: maybe it should always be the NoopSpan? + # something similar to: + # self._noop_span_context = SpanContext() + # self._noop_span = Span(tracer=self, context=self._noop_span_context) + self._span = span + + def span(self): + """ + Return the `Span` that's been scoped by this `Scope`. + """ + return self._span + + def close(self): + """ + Mark the end of the active period for the current thread and `Scope`, + updating the `ScopeManager#active()` in the process. + + NOTE: Calling `close()` more than once on a single `Scope` instance + leads to undefined behavior. + """ + pass + + def __enter__(self): + """ + Allow `Scope` to be used inside a Python Context Manager. + """ + return self + + def __exit__(self, exc_type, exc_val, exc_tb): + """ + Call `close()` when the execution is outside the Python + Context Manager. + """ + self.close() diff --git a/tests/test_scope.py b/tests/test_scope.py new file mode 100644 index 0000000..c7160ed --- /dev/null +++ b/tests/test_scope.py @@ -0,0 +1,53 @@ +# 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 mock + +from opentracing.tracer import Tracer +from opentracing.scope import Scope +from opentracing.span import Span, SpanContext + + +def test_scope_wrapper(): + # ensure `Scope` wraps the `Span` argument + span = Span(tracer=Tracer(), context=SpanContext()) + scope = Scope(span) + assert scope._span == span + assert scope.span() == span + + +def test_scope_context_manager(): + # ensure `Scope` can be used in a Context Manager that + # calls the `close()` method + span = Span(tracer=Tracer(), context=SpanContext()) + scope = Scope(span) + with mock.patch.object(scope, 'close') as close: + with scope: + pass + assert close.call_count == 1 + + +def test_scope_close(): + # ensure `Scope` can be closed + span = Span(tracer=Tracer(), context=SpanContext()) + scope = Scope(span) + scope.close() From 9ac4e41cc3cf86caedf7174c1b6938646fe29ff1 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Mon, 13 Nov 2017 09:54:18 +0100 Subject: [PATCH 02/17] add ScopeManager interface --- opentracing/scope_manager.py | 63 ++++++++++++++++++++++++++++++++++++ tests/test_scope_manager.py | 35 ++++++++++++++++++++ 2 files changed, 98 insertions(+) create mode 100644 opentracing/scope_manager.py create mode 100644 tests/test_scope_manager.py diff --git a/opentracing/scope_manager.py b/opentracing/scope_manager.py new file mode 100644 index 0000000..ddd77a5 --- /dev/null +++ b/opentracing/scope_manager.py @@ -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 + +from opentracing.scope import Scope +from opentracing.span import Span, SpanContext + + +class ScopeManager(object): + """ + The `ScopeManager` interface abstracts both the activation of `Span` + instances (via `ScopeManager#activate(Span)`) and access to an active + `Span` / `Scope` (via `ScopeManager#active()`). + """ + def __init__(self): + # TODO: check that if it's required + self._noop_span = Span(tracer=None, context=SpanContext()) + self._noop_scope = Scope(self._noop_span) + + def activate(self, span, finish_span_on_close=True): + """ + Make a `Span` instance active. + + :param span: the `Span` that should become active + :param finish_span_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. + """ + return self._noop_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 self._noop_scope diff --git a/tests/test_scope_manager.py b/tests/test_scope_manager.py new file mode 100644 index 0000000..9c6889f --- /dev/null +++ b/tests/test_scope_manager.py @@ -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 opentracing.scope_manager import ScopeManager +from opentracing.tracer import Tracer +from opentracing.span import Span, SpanContext + + +def test_scope_manager(): + # ensure the activation returns the noop `Scope` + # that is always active + scope_manager = ScopeManager() + span = Span(tracer=Tracer(), context=SpanContext()) + scope = scope_manager.activate(span) + assert scope == scope_manager._noop_scope + assert scope == scope_manager.active() From 3dc7f8b0093204a486e2e4db93b155f9b48fde82 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Mon, 13 Nov 2017 09:55:00 +0100 Subject: [PATCH 03/17] start_span deprecation; provide start_active and start_manual with ScopeManager propagation --- opentracing/harness/api_check.py | 74 +++++++++++++++++------ opentracing/tracer.py | 100 ++++++++++++++++++++++++++----- 2 files changed, 142 insertions(+), 32 deletions(-) diff --git a/opentracing/harness/api_check.py b/opentracing/harness/api_check.py index 36c6fc4..13b9434 100644 --- a/opentracing/harness/api_check.py +++ b/opentracing/harness/api_check.py @@ -45,6 +45,7 @@ def check_baggage_values(self): return True def test_start_span(self): + # test for deprecated API tracer = self.tracer() span = tracer.start_span(operation_name='Fry') span.finish() @@ -54,15 +55,49 @@ def test_start_span(self): payload={'hospital': 'Brooklyn Pre-Med Hospital', 'city': 'Old New York'}) - def test_start_span_with_parent(self): + def test_start_active(self): tracer = self.tracer() - parent_span = tracer.start_span(operation_name='parent') + scope = tracer.start_active( + operation_name='Fry', + ignore_active_span=False, + finish_on_close=True, + ) + + assert scope.span() is not None + + def test_scope_as_context_manager(self): + tracer = self.tracer() + + with tracer.start_active(operation_name='antiquing') as scope: + assert scope.span() is not None + + def test_start_manual(self): + tracer = self.tracer() + span = tracer.start_manual(operation_name='Fry') + span.finish() + with tracer.start_manual(operation_name='Fry', + tags={'birthday': 'August 14 1974'}) as span: + span.log_event('birthplace', + payload={'hospital': 'Brooklyn Pre-Med Hospital', + 'city': 'Old New York'}) + + def test_start_manual_ignore_active(self): + tracer = self.tracer() + span = tracer.start_manual( + operation_name='Fry', + ignore_active_span=False, + ) + assert span is not None + + def test_start_manual_with_parent(self): + tracer = self.tracer() + parent_span = tracer.start_manual(operation_name='parent') assert parent_span is not None - span = tracer.start_span( + span = tracer.start_manual( operation_name='Leela', child_of=parent_span) span.finish() - span = tracer.start_span( + span = tracer.start_manual( operation_name='Leela', references=[opentracing.follows_from(parent_span.context)], tags={'birthplace': 'sewers'}) @@ -71,7 +106,7 @@ def test_start_span_with_parent(self): def test_start_child_span(self): tracer = self.tracer() - parent_span = tracer.start_span(operation_name='parent') + parent_span = tracer.start_manual(operation_name='parent') assert parent_span is not None child_span = opentracing.start_child_span( parent_span, operation_name='Leela') @@ -79,23 +114,24 @@ def test_start_child_span(self): parent_span.finish() def test_set_operation_name(self): - span = self.tracer().start_span().set_operation_name('Farnsworth') + span = self.tracer().start_manual().set_operation_name('Farnsworth') span.finish() def test_span_as_context_manager(self): + tracer = self.tracer() finish = {'called': False} def mock_finish(*_): finish['called'] = True - with self.tracer().start_span(operation_name='antiquing') as span: + with tracer.start_manual(operation_name='antiquing') as span: setattr(span, 'finish', mock_finish) assert finish['called'] is True # now try with exception finish['called'] = False try: - with self.tracer().start_span(operation_name='antiquing') as span: + with tracer.start_manual(operation_name='antiquing') as span: setattr(span, 'finish', mock_finish) raise ValueError() except ValueError: @@ -104,14 +140,14 @@ def mock_finish(*_): raise AssertionError('Expected ValueError') # pragma: no cover def test_span_tag_value_types(self): - with self.tracer().start_span(operation_name='ManyTypes') as span: + with self.tracer().start_manual(operation_name='ManyTypes') as span: span. \ set_tag('an_int', 9). \ set_tag('a_bool', True). \ set_tag('a_string', 'aoeuidhtns') def test_span_tags_with_chaining(self): - span = self.tracer().start_span(operation_name='Farnsworth') + span = self.tracer().start_manual(operation_name='Farnsworth') span. \ set_tag('birthday', '9 April, 2841'). \ set_tag('loves', 'different lengths of wires') @@ -121,7 +157,7 @@ def test_span_tags_with_chaining(self): span.finish() def test_span_logs(self): - span = self.tracer().start_span(operation_name='Fry') + span = self.tracer().start_manual(operation_name='Fry') # Newer API span.log_kv( @@ -146,7 +182,7 @@ def test_span_logs(self): payload={'year': 2999}) def test_span_baggage(self): - with self.tracer().start_span(operation_name='Fry') as span: + with self.tracer().start_manual(operation_name='Fry') as span: assert span.context.baggage == {} span_ref = span.set_baggage_item('Kiff-loves', 'Amy') assert span_ref is span @@ -156,7 +192,7 @@ def test_span_baggage(self): pass def test_context_baggage(self): - with self.tracer().start_span(operation_name='Fry') as span: + with self.tracer().start_manual(operation_name='Fry') as span: assert span.context.baggage == {} span.set_baggage_item('Kiff-loves', 'Amy') if self.check_baggage_values(): @@ -164,7 +200,7 @@ def test_context_baggage(self): pass def test_text_propagation(self): - with self.tracer().start_span(operation_name='Bender') as span: + with self.tracer().start_manual(operation_name='Bender') as span: text_carrier = {} self.tracer().inject( span_context=span.context, @@ -176,7 +212,7 @@ def test_text_propagation(self): assert extracted_ctx.baggage == {} def test_binary_propagation(self): - with self.tracer().start_span(operation_name='Bender') as span: + with self.tracer().start_manual(operation_name='Bender') as span: bin_carrier = bytearray() self.tracer().inject( span_context=span.context, @@ -193,7 +229,7 @@ def test_mandatory_formats(self): (Format.HTTP_HEADERS, {}), (Format.BINARY, bytearray()), ] - with self.tracer().start_span(operation_name='Bender') as span: + with self.tracer().start_manual(operation_name='Bender') as span: for fmt, carrier in formats: # expecting no exceptions span.tracer.inject(span.context, fmt, carrier) @@ -201,8 +237,12 @@ def test_mandatory_formats(self): def test_unknown_format(self): custom_format = 'kiss my shiny metal ...' - with self.tracer().start_span(operation_name='Bender') as span: + with self.tracer().start_manual(operation_name='Bender') as span: with pytest.raises(opentracing.UnsupportedFormatException): span.tracer.inject(span.context, custom_format, {}) with pytest.raises(opentracing.UnsupportedFormatException): span.tracer.extract(custom_format, {}) + + def test_tracer_scope_manager(self): + # a Tracer should always have a ScopeManager + assert self.tracer().scope_manager diff --git a/opentracing/tracer.py b/opentracing/tracer.py index 57709c3..a527659 100644 --- a/opentracing/tracer.py +++ b/opentracing/tracer.py @@ -24,7 +24,9 @@ from collections import namedtuple from .span import Span from .span import SpanContext +from .scope import Scope from .propagation import Format, UnsupportedFormatException +from .scope_manager import ScopeManager class Tracer(object): @@ -38,33 +40,90 @@ class Tracer(object): _supported_formats = [Format.TEXT_MAP, Format.BINARY, Format.HTTP_HEADERS] def __init__(self): + self._scope_manager = ScopeManager() self._noop_span_context = SpanContext() self._noop_span = Span(tracer=self, context=self._noop_span_context) + self._noop_scope = Scope(self._noop_span) + + @property + def scope_manager(self): + """Accessor for `ScopeManager`.""" + return self._scope_manager + + def start_active(self, + operation_name=None, + child_of=None, + references=None, + tags=None, + start_time=None, + ignore_active_span=False, + finish_on_close=True): + """ + Returns a newly started and activated `Scope`. - def start_span(self, - operation_name=None, - child_of=None, - references=None, - tags=None, - start_time=None): + The returned `Scope` supports with-statement contexts. For example: + + with tracer.start_active('...') as scope: + scope.span().set_tag('http.method', 'GET') + do_some_work() + # Span is finished automatically outside the `Scope` `with`. + + It's also possible to not finish the `Span` when the `Scope` context + expires: + + with tracer.start_active('...', finish_on_close=False) as scope: + scope.span().set_tag('http.method', 'GET') + do_some_work() + # Span does not finish automatically when the Scope is closed as + # `finish_on_close` is `False` + + :param operation_name: name of the operation represented by the new + span from the perspective of the current service. + :param child_of: (optional) a Span or SpanContext instance representing + the parent in a REFERENCE_CHILD_OF Reference. If specified, the + `references` parameter must be omitted. + :param references: (optional) a list of Reference objects that identify + one or more parent SpanContexts. (See the Reference documentation + for detail). + :param tags: an optional dictionary of Span Tags. The caller gives up + ownership of that dictionary, because the Tracer may use it as-is + to avoid extra data copying. + :param start_time: an explicit Span start time as a unix timestamp per + time.time(). + :param ignore_active_span: an explicit flag that ignores the current + active `Span` and creates a root `Span`. + :param finish_on_close: whether span should automatically be finished + when `Scope#close()` is called. + + :return: a `Scope`, already registered via the `ScopeManager`. + """ + return self._noop_scope + + def start_manual(self, + operation_name=None, + child_of=None, + references=None, + tags=None, + start_time=None, + ignore_active_span=False): """Starts and returns a new Span representing a unit of work. Starting a root Span (a Span with no causal references):: - tracer.start_span('...') + tracer.start_manual('...') Starting a child Span (see also start_child_span()):: - tracer.start_span( + tracer.start_manual( '...', child_of=parent_span) Starting a child Span in a more verbose way:: - tracer.start_span( + tracer.start_manual( '...', references=[opentracing.child_of(parent_span)]) @@ -82,11 +141,22 @@ def start_span(self, to avoid extra data copying. :param start_time: an explicit Span start time as a unix timestamp per time.time() + :param ignore_active_span: an explicit flag that ignores the current + active `Span` and creates a root `Span`. :return: Returns an already-started Span instance. """ return self._noop_span + 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._noop_span + def inject(self, span_context, format, carrier): """Injects `span_context` into `carrier`. @@ -150,7 +220,7 @@ class ReferenceType(object): class Reference(namedtuple('Reference', ['type', 'referenced_context'])): """A Reference pairs a reference type with a referenced SpanContext. - References are used by Tracer.start_span() to describe the relationships + References are used by Tracer.start_manual() to describe the relationships between Spans. Tracer implementations must ignore references where referenced_context is @@ -159,7 +229,7 @@ class Reference(namedtuple('Reference', ['type', 'referenced_context'])): None:: parent_ref = tracer.extract(opentracing.HTTP_HEADERS, request.headers) - span = tracer.start_span( + span = tracer.start_manual( 'operation', references=child_of(parent_ref) ) @@ -175,7 +245,7 @@ def child_of(referenced_context=None): If None is passed, this reference must be ignored by the tracer. :rtype: Reference - :return: A Reference suitable for Tracer.start_span(..., references=...) + :return: A Reference suitable for Tracer.start_manual(..., references=...) """ return Reference( type=ReferenceType.CHILD_OF, @@ -189,7 +259,7 @@ def follows_from(referenced_context=None): If None is passed, this reference must be ignored by the tracer. :rtype: Reference - :return: A Reference suitable for Tracer.start_span(..., references=...) + :return: A Reference suitable for Tracer.start_manual(..., references=...) """ return Reference( type=ReferenceType.FOLLOWS_FROM, @@ -201,7 +271,7 @@ def start_child_span(parent_span, operation_name, tags=None, start_time=None): Equivalent to calling - parent_span.tracer().start_span( + parent_span.tracer().start_manual( operation_name, references=opentracing.child_of(parent_span.context), tags=tags, @@ -218,7 +288,7 @@ def start_child_span(parent_span, operation_name, tags=None, start_time=None): :return: Returns an already-started Span instance. """ - return parent_span.tracer.start_span( + return parent_span.tracer.start_manual( operation_name=operation_name, child_of=parent_span, tags=tags, From cf7b1dcff984ee156bfbc2d33576e2384dfbf49b Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Mon, 13 Nov 2017 10:08:52 +0100 Subject: [PATCH 04/17] update Scope and ScopeManager imports --- opentracing/__init__.py | 2 ++ opentracing/scope_manager.py | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/opentracing/__init__.py b/opentracing/__init__.py index bc8bc26..4faf9de 100644 --- a/opentracing/__init__.py +++ b/opentracing/__init__.py @@ -22,6 +22,8 @@ from __future__ import absolute_import from .span import Span # noqa from .span import SpanContext # noqa +from .scope import Scope # noqa +from .scope_manager import ScopeManager # noqa from .tracer import child_of # noqa from .tracer import follows_from # noqa from .tracer import Reference # noqa diff --git a/opentracing/scope_manager.py b/opentracing/scope_manager.py index ddd77a5..b9d82bf 100644 --- a/opentracing/scope_manager.py +++ b/opentracing/scope_manager.py @@ -20,8 +20,8 @@ from __future__ import absolute_import -from opentracing.scope import Scope -from opentracing.span import Span, SpanContext +from .span import Span, SpanContext +from .scope import Scope class ScopeManager(object): From 42b794466126e839f2f5673c28f71de040bb8819 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Mon, 13 Nov 2017 14:33:04 +0100 Subject: [PATCH 05/17] more APICompatibility checks --- opentracing/harness/api_check.py | 123 ++++++++++++++++++++++++++++--- opentracing/scope.py | 8 +- opentracing/scope_manager.py | 4 +- tests/test_api.py | 3 + tests/test_scope.py | 2 +- 5 files changed, 123 insertions(+), 17 deletions(-) diff --git a/opentracing/harness/api_check.py b/opentracing/harness/api_check.py index 13b9434..ea276b5 100644 --- a/opentracing/harness/api_check.py +++ b/opentracing/harness/api_check.py @@ -18,8 +18,9 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN # THE SOFTWARE. from __future__ import absolute_import -import time +import mock +import time import pytest import opentracing @@ -39,11 +40,26 @@ def check_baggage_values(self): """If true, the test will validate Baggage items by storing and retrieving them from the trace context. If false, it will only attempt to store and retrieve the Baggage items to check the API compliance, - but not actually validate stored values. The latter mode is only + but not actually validate stored values. The latter mode is only useful for no-op tracer. """ return True + def check_scope_manager(self): + """If true, the test suite will validate the `ScopeManager` propagation + and storage to ensure the correct parenting and active `Scope` are + properly defined by the implementation. If false, it will only use + the API without any assert. The latter mode is only useful for no-op + tracer. + """ + return True + + def is_parent(self, parent, span): + """Utility method that must be defined by Tracer implementers to define + how the test suite can check when a `Span` is a parent of another one. + """ + return False + def test_start_span(self): # test for deprecated API tracer = self.tracer() @@ -56,14 +72,69 @@ def test_start_span(self): 'city': 'Old New York'}) def test_start_active(self): + # the first usage returns a `Scope` that wraps a root `Span` tracer = self.tracer() - scope = tracer.start_active( - operation_name='Fry', - ignore_active_span=False, - finish_on_close=True, - ) + scope = tracer.start_active(operation_name='Fry') assert scope.span() is not None + if self.check_scope_manager(): + assert self.is_parent(None, scope.span()) + + def test_start_active_parent(self): + # ensure the `ScopeManager` provides the right parenting + tracer = self.tracer() + with tracer.start_active(operation_name='Fry') as parent: + with tracer.start_active(operation_name='Farnsworth') as child: + if self.check_scope_manager(): + assert self.is_parent(parent.span(), child.span()) + + def test_start_active_ignore_active_scope(self): + # ensure the `ScopeManager` ignores the active `Scope` + # if the flag is set + tracer = self.tracer() + with tracer.start_active(operation_name='Fry') as parent: + with tracer.start_active(operation_name='Farnsworth', + ignore_active_span=True) as child: + if self.check_scope_manager(): + assert not self.is_parent(parent.span(), child.span()) + + def test_start_active_finish_on_close(self): + # ensure a `Span` is finished when the `Scope` close + tracer = self.tracer() + scope = tracer.start_active(operation_name='Fry') + with mock.patch.object(scope.span(), 'finish') as finish: + scope.close() + + if self.check_scope_manager(): + assert finish.call_count == 1 + + def test_start_active_not_finish_on_close(self): + # a `Span` is not finished when the flag is set + tracer = self.tracer() + scope = tracer.start_active(operation_name='Fry', + finish_on_close=False) + with mock.patch.object(scope.span(), 'finish') as finish: + scope.close() + + assert finish.call_count == 0 + + def test_start_manual_propagation(self): + # `start_manual` must inherit the current active `Scope` span + tracer = self.tracer() + with tracer.start_active(operation_name='Fry') as parent: + with tracer.start_manual(operation_name='Farnsworth') as child: + if self.check_scope_manager(): + assert self.is_parent(parent.span(), child) + + def test_start_manual_propagation_ignore_active_scope(self): + # `start_manual` doesn't inherit the current active `Scope` span + # if the flag is set + tracer = self.tracer() + with tracer.start_active(operation_name='Fry') as parent: + with tracer.start_manual(operation_name='Farnsworth', + ignore_active_span=True) as child: + if self.check_scope_manager(): + assert not self.is_parent(parent.span(), child) def test_scope_as_context_manager(self): tracer = self.tracer() @@ -243,6 +314,38 @@ def test_unknown_format(self): with pytest.raises(opentracing.UnsupportedFormatException): span.tracer.extract(custom_format, {}) - def test_tracer_scope_manager(self): - # a Tracer should always have a ScopeManager - assert self.tracer().scope_manager + def test_tracer_start_active_scope(self): + # the Tracer ScopeManager should store the active Scope + tracer = self.tracer() + scope = tracer.start_active(operation_name='Fry') + + if self.check_scope_manager(): + assert scope == tracer.scope_manager.active() + + scope.close() + + def test_tracer_start_manual_scope(self): + # the Tracer ScopeManager should not store the new Span + tracer = self.tracer() + span = tracer.start_manual(operation_name='Fry') + + if self.check_scope_manager(): + assert tracer.scope_manager.active() is None + + span.finish() + + def test_tracer_scope_manager_active(self): + # a `ScopeManager` has no scopes in its initial state + tracer = self.tracer() + + if self.check_scope_manager(): + assert tracer.scope_manager.active() is None + + def test_tracer_scope_manager_activate(self): + # a `ScopeManager` should activate any `Span` + tracer = self.tracer() + span = tracer.start_manual(operation_name='Fry') + tracer.scope_manager.activate(span) + + if self.check_scope_manager(): + assert tracer.scope_manager.active().span() == span diff --git a/opentracing/scope.py b/opentracing/scope.py index a9c1b41..688d941 100644 --- a/opentracing/scope.py +++ b/opentracing/scope.py @@ -31,16 +31,14 @@ class Scope(object): still outstanding. A `Scope` defines when a given `Span` is scheduled and on the path. """ - def __init__(self, span): + def __init__(self, span, finish_span_on_close=True): """ Initialize a `Scope` for the given `Span` object :param span: the `Span` used for this `Scope` + :param finish_span_on_close: whether span should automatically be + finished when `Scope#close()` is called """ - # TODO: maybe it should always be the NoopSpan? - # something similar to: - # self._noop_span_context = SpanContext() - # self._noop_span = Span(tracer=self, context=self._noop_span_context) self._span = span def span(self): diff --git a/opentracing/scope_manager.py b/opentracing/scope_manager.py index b9d82bf..a2e8f2f 100644 --- a/opentracing/scope_manager.py +++ b/opentracing/scope_manager.py @@ -31,7 +31,9 @@ class ScopeManager(object): `Span` / `Scope` (via `ScopeManager#active()`). """ def __init__(self): - # TODO: check that if it's required + # TODO: `tracer` should not be None, but we don't have a reference; + # should we move the NOOP SpanContext, Span, Scope to somewhere + # else so that they're globally reachable? self._noop_span = Span(tracer=None, context=SpanContext()) self._noop_scope = Scope(self._noop_span) diff --git a/tests/test_api.py b/tests/test_api.py index 7e800e2..f32df62 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -33,3 +33,6 @@ def tracer(self): def check_baggage_values(self): return False + + def check_scope_manager(self): + return False diff --git a/tests/test_scope.py b/tests/test_scope.py index c7160ed..c37e6de 100644 --- a/tests/test_scope.py +++ b/tests/test_scope.py @@ -30,7 +30,7 @@ def test_scope_wrapper(): # ensure `Scope` wraps the `Span` argument span = Span(tracer=Tracer(), context=SpanContext()) - scope = Scope(span) + scope = Scope(span, finish_span_on_close=False) assert scope._span == span assert scope.span() == span From 7f10c8f113c4ff0be0b5d8c4052797033eec74c4 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Mon, 13 Nov 2017 20:47:11 +0100 Subject: [PATCH 06/17] kwarg and arguments renaming; the Scope has the ScopeManager reference --- opentracing/harness/api_check.py | 82 ++++++++++++++++++++++---------- opentracing/scope.py | 5 +- opentracing/scope_manager.py | 6 +-- opentracing/tracer.py | 10 ++-- tests/test_api_check_mixin.py | 6 +++ tests/test_scope.py | 7 +-- 6 files changed, 77 insertions(+), 39 deletions(-) diff --git a/opentracing/harness/api_check.py b/opentracing/harness/api_check.py index ea276b5..0a9167a 100644 --- a/opentracing/harness/api_check.py +++ b/opentracing/harness/api_check.py @@ -94,7 +94,7 @@ def test_start_active_ignore_active_scope(self): tracer = self.tracer() with tracer.start_active(operation_name='Fry') as parent: with tracer.start_active(operation_name='Farnsworth', - ignore_active_span=True) as child: + ignore_active_scope=True) as child: if self.check_scope_manager(): assert not self.is_parent(parent.span(), child.span()) @@ -118,24 +118,6 @@ def test_start_active_not_finish_on_close(self): assert finish.call_count == 0 - def test_start_manual_propagation(self): - # `start_manual` must inherit the current active `Scope` span - tracer = self.tracer() - with tracer.start_active(operation_name='Fry') as parent: - with tracer.start_manual(operation_name='Farnsworth') as child: - if self.check_scope_manager(): - assert self.is_parent(parent.span(), child) - - def test_start_manual_propagation_ignore_active_scope(self): - # `start_manual` doesn't inherit the current active `Scope` span - # if the flag is set - tracer = self.tracer() - with tracer.start_active(operation_name='Fry') as parent: - with tracer.start_manual(operation_name='Farnsworth', - ignore_active_span=True) as child: - if self.check_scope_manager(): - assert not self.is_parent(parent.span(), child) - def test_scope_as_context_manager(self): tracer = self.tracer() @@ -152,13 +134,23 @@ def test_start_manual(self): payload={'hospital': 'Brooklyn Pre-Med Hospital', 'city': 'Old New York'}) - def test_start_manual_ignore_active(self): + def test_start_manual_propagation(self): + # `start_manual` must inherit the current active `Scope` span tracer = self.tracer() - span = tracer.start_manual( - operation_name='Fry', - ignore_active_span=False, - ) - assert span is not None + with tracer.start_active(operation_name='Fry') as parent: + with tracer.start_manual(operation_name='Farnsworth') as child: + if self.check_scope_manager(): + assert self.is_parent(parent.span(), child) + + def test_start_manual_propagation_ignore_active_scope(self): + # `start_manual` doesn't inherit the current active `Scope` span + # if the flag is set + tracer = self.tracer() + with tracer.start_active(operation_name='Fry') as parent: + with tracer.start_manual(operation_name='Farnsworth', + ignore_active_scope=True) as child: + if self.check_scope_manager(): + assert not self.is_parent(parent.span(), child) def test_start_manual_with_parent(self): tracer = self.tracer() @@ -320,10 +312,48 @@ def test_tracer_start_active_scope(self): scope = tracer.start_active(operation_name='Fry') if self.check_scope_manager(): - assert scope == tracer.scope_manager.active() + assert tracer.scope_manager.active() == scope scope.close() + def test_tracer_start_active_nesting(self): + # when a Scope is closed, the previous one must be activated + tracer = self.tracer() + with tracer.start_active(operation_name='Fry') as parent: + with tracer.start_active(operation_name='Fry'): + pass + + if self.check_scope_manager(): + assert tracer.scope_manager.active() == parent + + if self.check_scope_manager(): + assert tracer.scope_manager.active() is None + + def test_tracer_start_active_nesting_finish_on_close(self): + # finish_on_close must be correctly handled + tracer = self.tracer() + parent = tracer.start_active(operation_name='Fry', + finish_on_close=False) + with mock.patch.object(parent.span(), 'finish') as finish: + with tracer.start_active(operation_name='Fry'): + pass + parent.close() + + assert finish.call_count == 0 + + if self.check_scope_manager(): + assert tracer.scope_manager.active() is None + + def test_tracer_start_active_wrong_close_order(self): + # only the active `Scope` can be closed + tracer = self.tracer() + parent = tracer.start_active(operation_name='Fry') + child = tracer.start_active(operation_name='Fry') + parent.close() + + if self.check_scope_manager(): + assert tracer.scope_manager.active() == child + def test_tracer_start_manual_scope(self): # the Tracer ScopeManager should not store the new Span tracer = self.tracer() diff --git a/opentracing/scope.py b/opentracing/scope.py index 688d941..4776199 100644 --- a/opentracing/scope.py +++ b/opentracing/scope.py @@ -31,14 +31,15 @@ class Scope(object): still outstanding. A `Scope` defines when a given `Span` is scheduled and on the path. """ - def __init__(self, span, finish_span_on_close=True): + 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_span_on_close: whether span should automatically be + :param finish_on_close: whether span should automatically be finished when `Scope#close()` is called """ + self._manager = manager self._span = span def span(self): diff --git a/opentracing/scope_manager.py b/opentracing/scope_manager.py index a2e8f2f..3ba19bc 100644 --- a/opentracing/scope_manager.py +++ b/opentracing/scope_manager.py @@ -35,14 +35,14 @@ def __init__(self): # should we move the NOOP SpanContext, Span, Scope to somewhere # else so that they're globally reachable? self._noop_span = Span(tracer=None, context=SpanContext()) - self._noop_scope = Scope(self._noop_span) + self._noop_scope = Scope(self, self._noop_span) - def activate(self, span, finish_span_on_close=True): + def activate(self, span, finish_on_close=True): """ Make a `Span` instance active. :param span: the `Span` that should become active - :param finish_span_on_close: whether span should automatically be + :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 diff --git a/opentracing/tracer.py b/opentracing/tracer.py index a527659..bc1ecad 100644 --- a/opentracing/tracer.py +++ b/opentracing/tracer.py @@ -43,7 +43,7 @@ def __init__(self): self._scope_manager = ScopeManager() self._noop_span_context = SpanContext() self._noop_span = Span(tracer=self, context=self._noop_span_context) - self._noop_scope = Scope(self._noop_span) + self._noop_scope = Scope(self._scope_manager, self._noop_span) @property def scope_manager(self): @@ -56,7 +56,7 @@ def start_active(self, references=None, tags=None, start_time=None, - ignore_active_span=False, + ignore_active_scope=False, finish_on_close=True): """ Returns a newly started and activated `Scope`. @@ -90,7 +90,7 @@ def start_active(self, to avoid extra data copying. :param start_time: an explicit Span start time as a unix timestamp per time.time(). - :param ignore_active_span: an explicit flag that ignores the current + :param ignore_active_scope: an explicit flag that ignores the current active `Span` and creates a root `Span`. :param finish_on_close: whether span should automatically be finished when `Scope#close()` is called. @@ -105,7 +105,7 @@ def start_manual(self, references=None, tags=None, start_time=None, - ignore_active_span=False): + ignore_active_scope=False): """Starts and returns a new Span representing a unit of work. @@ -141,7 +141,7 @@ def start_manual(self, to avoid extra data copying. :param start_time: an explicit Span start time as a unix timestamp per time.time() - :param ignore_active_span: an explicit flag that ignores the current + :param ignore_active_scope: an explicit flag that ignores the current active `Span` and creates a root `Span`. :return: Returns an already-started Span instance. diff --git a/tests/test_api_check_mixin.py b/tests/test_api_check_mixin.py index a1c0cad..065eaa2 100644 --- a/tests/test_api_check_mixin.py +++ b/tests/test_api_check_mixin.py @@ -45,3 +45,9 @@ def test_baggage_check_works(self): # second check that assert on empty baggage will fail too with self.assertRaises(AssertionError): api_check.test_context_baggage() + + def test_scope_manager_check_works(self): + api_check = APICompatibilityCheckMixin() + setattr(api_check, 'tracer', lambda: Tracer()) + + # TODO: list here all scope_manager checks diff --git a/tests/test_scope.py b/tests/test_scope.py index c37e6de..64bad41 100644 --- a/tests/test_scope.py +++ b/tests/test_scope.py @@ -22,6 +22,7 @@ import mock +from opentracing.scope_manager import ScopeManager from opentracing.tracer import Tracer from opentracing.scope import Scope from opentracing.span import Span, SpanContext @@ -30,7 +31,7 @@ def test_scope_wrapper(): # ensure `Scope` wraps the `Span` argument span = Span(tracer=Tracer(), context=SpanContext()) - scope = Scope(span, finish_span_on_close=False) + scope = Scope(ScopeManager, span, finish_on_close=False) assert scope._span == span assert scope.span() == span @@ -39,7 +40,7 @@ def test_scope_context_manager(): # ensure `Scope` can be used in a Context Manager that # calls the `close()` method span = Span(tracer=Tracer(), context=SpanContext()) - scope = Scope(span) + scope = Scope(ScopeManager(), span) with mock.patch.object(scope, 'close') as close: with scope: pass @@ -49,5 +50,5 @@ def test_scope_context_manager(): def test_scope_close(): # ensure `Scope` can be closed span = Span(tracer=Tracer(), context=SpanContext()) - scope = Scope(span) + scope = Scope(ScopeManager(), span) scope.close() From 1dbd1fba99d878d36141d43d63437d4833029668 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Mon, 13 Nov 2017 21:05:49 +0100 Subject: [PATCH 07/17] increase test coverage calling ScopeManager checks --- tests/test_api_check_mixin.py | 43 ++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/tests/test_api_check_mixin.py b/tests/test_api_check_mixin.py index 065eaa2..60d24be 100644 --- a/tests/test_api_check_mixin.py +++ b/tests/test_api_check_mixin.py @@ -33,6 +33,10 @@ def test_default_baggage_check_mode(self): api_check = APICompatibilityCheckMixin() assert api_check.check_baggage_values() is True + def test_default_scope_manager_check_mode(self): + api_check = APICompatibilityCheckMixin() + assert api_check.check_scope_manager() is True + def test_baggage_check_works(self): api_check = APICompatibilityCheckMixin() setattr(api_check, 'tracer', lambda: Tracer()) @@ -50,4 +54,41 @@ def test_scope_manager_check_works(self): api_check = APICompatibilityCheckMixin() setattr(api_check, 'tracer', lambda: Tracer()) - # TODO: list here all scope_manager checks + # these tests are expected to succeed + api_check.test_start_active_ignore_active_scope() + api_check.test_start_manual_propagation_ignore_active_scope() + + # no-op tracer doesn't have a ScopeManager implementation + # so these tests are expected to work, but asserts to fail + with self.assertRaises(AssertionError): + api_check.test_start_active() + + with self.assertRaises(AssertionError): + api_check.test_start_active_parent() + + with self.assertRaises(AssertionError): + api_check.test_start_active_finish_on_close() + + with self.assertRaises(AssertionError): + api_check.test_start_manual_propagation() + + with self.assertRaises(AssertionError): + api_check.test_tracer_start_active_scope() + + with self.assertRaises(AssertionError): + api_check.test_tracer_start_active_nesting() + + with self.assertRaises(AssertionError): + api_check.test_tracer_start_active_nesting_finish_on_close() + + with self.assertRaises(AssertionError): + api_check.test_tracer_start_active_wrong_close_order() + + with self.assertRaises(AssertionError): + api_check.test_tracer_start_manual_scope() + + with self.assertRaises(AssertionError): + api_check.test_tracer_scope_manager_active() + + with self.assertRaises(AssertionError): + api_check.test_tracer_scope_manager_activate() From 9f406cfd03c196f45506dcb95835a8691aec9837 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Tue, 14 Nov 2017 11:46:59 +0100 Subject: [PATCH 08/17] updating docstrings and tests --- opentracing/harness/api_check.py | 16 ++++++++-------- opentracing/scope.py | 23 ++++++++--------------- opentracing/scope_manager.py | 9 +++------ opentracing/tracer.py | 12 +++++------- tests/test_scope.py | 8 -------- tests/test_scope_manager.py | 3 +-- 6 files changed, 25 insertions(+), 46 deletions(-) diff --git a/opentracing/harness/api_check.py b/opentracing/harness/api_check.py index 0a9167a..2976176 100644 --- a/opentracing/harness/api_check.py +++ b/opentracing/harness/api_check.py @@ -47,21 +47,21 @@ def check_baggage_values(self): def check_scope_manager(self): """If true, the test suite will validate the `ScopeManager` propagation - and storage to ensure the correct parenting and active `Scope` are - properly defined by the implementation. If false, it will only use - the API without any assert. The latter mode is only useful for no-op - tracer. + to ensure correct parenting. If false, it will only use the API without + asserting. The latter mode is only useful for no-op tracer. """ return True def is_parent(self, parent, span): """Utility method that must be defined by Tracer implementers to define how the test suite can check when a `Span` is a parent of another one. + It depends by the underlying implementation that is not part of the + OpenTracing API. """ return False def test_start_span(self): - # test for deprecated API + # test deprecated API for minor compatibility tracer = self.tracer() span = tracer.start_span(operation_name='Fry') span.finish() @@ -320,7 +320,7 @@ def test_tracer_start_active_nesting(self): # when a Scope is closed, the previous one must be activated tracer = self.tracer() with tracer.start_active(operation_name='Fry') as parent: - with tracer.start_active(operation_name='Fry'): + with tracer.start_active(operation_name='Farnsworth'): pass if self.check_scope_manager(): @@ -335,7 +335,7 @@ def test_tracer_start_active_nesting_finish_on_close(self): parent = tracer.start_active(operation_name='Fry', finish_on_close=False) with mock.patch.object(parent.span(), 'finish') as finish: - with tracer.start_active(operation_name='Fry'): + with tracer.start_active(operation_name='Farnsworth'): pass parent.close() @@ -348,7 +348,7 @@ def test_tracer_start_active_wrong_close_order(self): # only the active `Scope` can be closed tracer = self.tracer() parent = tracer.start_active(operation_name='Fry') - child = tracer.start_active(operation_name='Fry') + child = tracer.start_active(operation_name='Farnsworth') parent.close() if self.check_scope_manager(): diff --git a/opentracing/scope.py b/opentracing/scope.py index 4776199..95dde38 100644 --- a/opentracing/scope.py +++ b/opentracing/scope.py @@ -22,9 +22,8 @@ class Scope(object): - """ - A `Scope` formalizes the activation and deactivation of a `Span`, usually - from a CPU standpoint. Many times a `Span` will be extant (in that + """A `Scope` formalizes the activation and deactivation of a `Span`, + usually from a CPU standpoint. Many times a `Span` will be extant (in that `Span#finish()` has not been called) despite being in a non-runnable state from a CPU/scheduler standpoint. For instance, a `Span` representing the client side of an RPC will be unfinished but blocked on IO while the RPC is @@ -32,9 +31,9 @@ class Scope(object): and on the path. """ def __init__(self, manager, span, finish_on_close=True): - """ - Initialize a `Scope` for the given `Span` object + """Initialize a `Scope` for the given `Span` object + :param manager: the `ScopeManager` that created this `Scope` :param span: the `Span` used for this `Scope` :param finish_on_close: whether span should automatically be finished when `Scope#close()` is called @@ -43,14 +42,11 @@ def __init__(self, manager, span, finish_on_close=True): self._span = span def span(self): - """ - Return the `Span` that's been scoped by this `Scope`. - """ + """Return the `Span` that's been scoped by this `Scope`.""" return self._span def close(self): - """ - Mark the end of the active period for the current thread and `Scope`, + """Mark the end of the active period for the current thread and `Scope`, updating the `ScopeManager#active()` in the process. NOTE: Calling `close()` more than once on a single `Scope` instance @@ -59,14 +55,11 @@ def close(self): pass def __enter__(self): - """ - Allow `Scope` to be used inside a Python Context Manager. - """ + """Allow `Scope` to be used inside a Python Context Manager.""" return self def __exit__(self, exc_type, exc_val, exc_tb): - """ - Call `close()` when the execution is outside the Python + """Call `close()` when the execution is outside the Python Context Manager. """ self.close() diff --git a/opentracing/scope_manager.py b/opentracing/scope_manager.py index 3ba19bc..52e5028 100644 --- a/opentracing/scope_manager.py +++ b/opentracing/scope_manager.py @@ -25,8 +25,7 @@ class ScopeManager(object): - """ - The `ScopeManager` interface abstracts both the activation of `Span` + """The `ScopeManager` interface abstracts both the activation of `Span` instances (via `ScopeManager#activate(Span)`) and access to an active `Span` / `Scope` (via `ScopeManager#active()`). """ @@ -38,8 +37,7 @@ def __init__(self): self._noop_scope = Scope(self, self._noop_span) def activate(self, span, finish_on_close=True): - """ - Make a `Span` instance active. + """Make a `Span` instance active. :param span: the `Span` that should become active :param finish_on_close: whether span should automatically be @@ -52,8 +50,7 @@ def activate(self, span, finish_on_close=True): return self._noop_scope def active(self): - """ - Return the currently active `Scope` which can be used to access the + """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 diff --git a/opentracing/tracer.py b/opentracing/tracer.py index bc1ecad..7620b7b 100644 --- a/opentracing/tracer.py +++ b/opentracing/tracer.py @@ -47,7 +47,7 @@ def __init__(self): @property def scope_manager(self): - """Accessor for `ScopeManager`.""" + """ScopeManager accessor""" return self._scope_manager def start_active(self, @@ -58,10 +58,8 @@ def start_active(self, start_time=None, ignore_active_scope=False, finish_on_close=True): - """ - Returns a newly started and activated `Scope`. - - The returned `Scope` supports with-statement contexts. For example: + """Returns a newly started and activated `Scope`. + Returned `Scope` supports with-statement contexts. For example: with tracer.start_active('...') as scope: scope.span().set_tag('http.method', 'GET') @@ -91,7 +89,7 @@ def start_active(self, :param start_time: an explicit Span start time as a unix timestamp per time.time(). :param ignore_active_scope: an explicit flag that ignores the current - active `Span` and creates a root `Span`. + active `Scope` and creates a root `Span`. :param finish_on_close: whether span should automatically be finished when `Scope#close()` is called. @@ -142,7 +140,7 @@ def start_manual(self, :param start_time: an explicit Span start time as a unix timestamp per time.time() :param ignore_active_scope: an explicit flag that ignores the current - active `Span` and creates a root `Span`. + active `Scope` and creates a root `Span`. :return: Returns an already-started Span instance. """ diff --git a/tests/test_scope.py b/tests/test_scope.py index 64bad41..7d6f84b 100644 --- a/tests/test_scope.py +++ b/tests/test_scope.py @@ -32,7 +32,6 @@ def test_scope_wrapper(): # ensure `Scope` wraps the `Span` argument span = Span(tracer=Tracer(), context=SpanContext()) scope = Scope(ScopeManager, span, finish_on_close=False) - assert scope._span == span assert scope.span() == span @@ -45,10 +44,3 @@ def test_scope_context_manager(): with scope: pass assert close.call_count == 1 - - -def test_scope_close(): - # ensure `Scope` can be closed - span = Span(tracer=Tracer(), context=SpanContext()) - scope = Scope(ScopeManager(), span) - scope.close() diff --git a/tests/test_scope_manager.py b/tests/test_scope_manager.py index 9c6889f..18b8c43 100644 --- a/tests/test_scope_manager.py +++ b/tests/test_scope_manager.py @@ -26,8 +26,7 @@ def test_scope_manager(): - # ensure the activation returns the noop `Scope` - # that is always active + # ensure the activation returns the noop `Scope` that is always active scope_manager = ScopeManager() span = Span(tracer=Tracer(), context=SpanContext()) scope = scope_manager.activate(span) From 17cb696d5fe626d46a2a9c6bec2f587388cd9327 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Wed, 31 Jan 2018 19:18:46 +0100 Subject: [PATCH 09/17] Update Tracer's start_* calls. Make our start_* calls behave like the ones from the Java API: * start_span() stays untouched. * start_manual() is gone (no need to deprecate as master does not have it yet). * start_active() becomes start_active_span() (for better naming), and finish_on_close becomes 'False' by default. * ignore_active_scope parameter becomes ignore_active_span, just as the Java api has it. --- opentracing/harness/api_check.py | 125 ++++++++++++++----------------- opentracing/tracer.py | 69 ++++++++--------- tests/test_api_check_mixin.py | 22 +++--- 3 files changed, 98 insertions(+), 118 deletions(-) diff --git a/opentracing/harness/api_check.py b/opentracing/harness/api_check.py index 2976176..c0133c3 100644 --- a/opentracing/harness/api_check.py +++ b/opentracing/harness/api_check.py @@ -60,58 +60,47 @@ def is_parent(self, parent, span): """ return False - def test_start_span(self): - # test deprecated API for minor compatibility - tracer = self.tracer() - span = tracer.start_span(operation_name='Fry') - span.finish() - with tracer.start_span(operation_name='Fry', - tags={'birthday': 'August 14 1974'}) as span: - span.log_event('birthplace', - payload={'hospital': 'Brooklyn Pre-Med Hospital', - 'city': 'Old New York'}) - - def test_start_active(self): + def test_start_active_span(self): # the first usage returns a `Scope` that wraps a root `Span` tracer = self.tracer() - scope = tracer.start_active(operation_name='Fry') + scope = tracer.start_active_span(operation_name='Fry') assert scope.span() is not None if self.check_scope_manager(): assert self.is_parent(None, scope.span()) - def test_start_active_parent(self): + def test_start_active_span_parent(self): # ensure the `ScopeManager` provides the right parenting tracer = self.tracer() - with tracer.start_active(operation_name='Fry') as parent: - with tracer.start_active(operation_name='Farnsworth') as child: + with tracer.start_active_span(operation_name='Fry') as parent: + with tracer.start_active_span(operation_name='Farnsworth') as child: if self.check_scope_manager(): assert self.is_parent(parent.span(), child.span()) - def test_start_active_ignore_active_scope(self): + def test_start_active_span_ignore_active_span(self): # ensure the `ScopeManager` ignores the active `Scope` # if the flag is set tracer = self.tracer() - with tracer.start_active(operation_name='Fry') as parent: - with tracer.start_active(operation_name='Farnsworth', - ignore_active_scope=True) as child: + with tracer.start_active_span(operation_name='Fry') as parent: + with tracer.start_active_span(operation_name='Farnsworth', + ignore_active_span=True) as child: if self.check_scope_manager(): assert not self.is_parent(parent.span(), child.span()) - def test_start_active_finish_on_close(self): + def test_start_active_span_finish_on_close(self): # ensure a `Span` is finished when the `Scope` close tracer = self.tracer() - scope = tracer.start_active(operation_name='Fry') + scope = tracer.start_active_span(operation_name='Fry') with mock.patch.object(scope.span(), 'finish') as finish: scope.close() if self.check_scope_manager(): assert finish.call_count == 1 - def test_start_active_not_finish_on_close(self): + def test_start_active_span_not_finish_on_close(self): # a `Span` is not finished when the flag is set tracer = self.tracer() - scope = tracer.start_active(operation_name='Fry', + scope = tracer.start_active_span(operation_name='Fry', finish_on_close=False) with mock.patch.object(scope.span(), 'finish') as finish: scope.close() @@ -121,46 +110,46 @@ def test_start_active_not_finish_on_close(self): def test_scope_as_context_manager(self): tracer = self.tracer() - with tracer.start_active(operation_name='antiquing') as scope: + with tracer.start_active_span(operation_name='antiquing') as scope: assert scope.span() is not None - def test_start_manual(self): + def test_start_span(self): tracer = self.tracer() - span = tracer.start_manual(operation_name='Fry') + span = tracer.start_span(operation_name='Fry') span.finish() - with tracer.start_manual(operation_name='Fry', + with tracer.start_span(operation_name='Fry', tags={'birthday': 'August 14 1974'}) as span: span.log_event('birthplace', payload={'hospital': 'Brooklyn Pre-Med Hospital', 'city': 'Old New York'}) - def test_start_manual_propagation(self): - # `start_manual` must inherit the current active `Scope` span + def test_start_span_propagation(self): + # `start_span` must inherit the current active `Scope` span tracer = self.tracer() - with tracer.start_active(operation_name='Fry') as parent: - with tracer.start_manual(operation_name='Farnsworth') as child: + with tracer.start_active_span(operation_name='Fry') as parent: + with tracer.start_span(operation_name='Farnsworth') as child: if self.check_scope_manager(): assert self.is_parent(parent.span(), child) - def test_start_manual_propagation_ignore_active_scope(self): - # `start_manual` doesn't inherit the current active `Scope` span + def test_start_span_propagation_ignore_active_span(self): + # `start_span` doesn't inherit the current active `Scope` span # if the flag is set tracer = self.tracer() - with tracer.start_active(operation_name='Fry') as parent: - with tracer.start_manual(operation_name='Farnsworth', - ignore_active_scope=True) as child: + with tracer.start_active_span(operation_name='Fry') as parent: + with tracer.start_span(operation_name='Farnsworth', + ignore_active_span=True) as child: if self.check_scope_manager(): assert not self.is_parent(parent.span(), child) - def test_start_manual_with_parent(self): + def test_start_span_with_parent(self): tracer = self.tracer() - parent_span = tracer.start_manual(operation_name='parent') + parent_span = tracer.start_span(operation_name='parent') assert parent_span is not None - span = tracer.start_manual( + span = tracer.start_span( operation_name='Leela', child_of=parent_span) span.finish() - span = tracer.start_manual( + span = tracer.start_span( operation_name='Leela', references=[opentracing.follows_from(parent_span.context)], tags={'birthplace': 'sewers'}) @@ -169,7 +158,7 @@ def test_start_manual_with_parent(self): def test_start_child_span(self): tracer = self.tracer() - parent_span = tracer.start_manual(operation_name='parent') + parent_span = tracer.start_span(operation_name='parent') assert parent_span is not None child_span = opentracing.start_child_span( parent_span, operation_name='Leela') @@ -177,7 +166,7 @@ def test_start_child_span(self): parent_span.finish() def test_set_operation_name(self): - span = self.tracer().start_manual().set_operation_name('Farnsworth') + span = self.tracer().start_span().set_operation_name('Farnsworth') span.finish() def test_span_as_context_manager(self): @@ -187,14 +176,14 @@ def test_span_as_context_manager(self): def mock_finish(*_): finish['called'] = True - with tracer.start_manual(operation_name='antiquing') as span: + with tracer.start_span(operation_name='antiquing') as span: setattr(span, 'finish', mock_finish) assert finish['called'] is True # now try with exception finish['called'] = False try: - with tracer.start_manual(operation_name='antiquing') as span: + with tracer.start_span(operation_name='antiquing') as span: setattr(span, 'finish', mock_finish) raise ValueError() except ValueError: @@ -203,14 +192,14 @@ def mock_finish(*_): raise AssertionError('Expected ValueError') # pragma: no cover def test_span_tag_value_types(self): - with self.tracer().start_manual(operation_name='ManyTypes') as span: + with self.tracer().start_span(operation_name='ManyTypes') as span: span. \ set_tag('an_int', 9). \ set_tag('a_bool', True). \ set_tag('a_string', 'aoeuidhtns') def test_span_tags_with_chaining(self): - span = self.tracer().start_manual(operation_name='Farnsworth') + span = self.tracer().start_span(operation_name='Farnsworth') span. \ set_tag('birthday', '9 April, 2841'). \ set_tag('loves', 'different lengths of wires') @@ -220,7 +209,7 @@ def test_span_tags_with_chaining(self): span.finish() def test_span_logs(self): - span = self.tracer().start_manual(operation_name='Fry') + span = self.tracer().start_span(operation_name='Fry') # Newer API span.log_kv( @@ -245,7 +234,7 @@ def test_span_logs(self): payload={'year': 2999}) def test_span_baggage(self): - with self.tracer().start_manual(operation_name='Fry') as span: + with self.tracer().start_span(operation_name='Fry') as span: assert span.context.baggage == {} span_ref = span.set_baggage_item('Kiff-loves', 'Amy') assert span_ref is span @@ -255,7 +244,7 @@ def test_span_baggage(self): pass def test_context_baggage(self): - with self.tracer().start_manual(operation_name='Fry') as span: + with self.tracer().start_span(operation_name='Fry') as span: assert span.context.baggage == {} span.set_baggage_item('Kiff-loves', 'Amy') if self.check_baggage_values(): @@ -263,7 +252,7 @@ def test_context_baggage(self): pass def test_text_propagation(self): - with self.tracer().start_manual(operation_name='Bender') as span: + with self.tracer().start_span(operation_name='Bender') as span: text_carrier = {} self.tracer().inject( span_context=span.context, @@ -275,7 +264,7 @@ def test_text_propagation(self): assert extracted_ctx.baggage == {} def test_binary_propagation(self): - with self.tracer().start_manual(operation_name='Bender') as span: + with self.tracer().start_span(operation_name='Bender') as span: bin_carrier = bytearray() self.tracer().inject( span_context=span.context, @@ -292,7 +281,7 @@ def test_mandatory_formats(self): (Format.HTTP_HEADERS, {}), (Format.BINARY, bytearray()), ] - with self.tracer().start_manual(operation_name='Bender') as span: + with self.tracer().start_span(operation_name='Bender') as span: for fmt, carrier in formats: # expecting no exceptions span.tracer.inject(span.context, fmt, carrier) @@ -300,27 +289,27 @@ def test_mandatory_formats(self): def test_unknown_format(self): custom_format = 'kiss my shiny metal ...' - with self.tracer().start_manual(operation_name='Bender') as span: + with self.tracer().start_span(operation_name='Bender') as span: with pytest.raises(opentracing.UnsupportedFormatException): span.tracer.inject(span.context, custom_format, {}) with pytest.raises(opentracing.UnsupportedFormatException): span.tracer.extract(custom_format, {}) - def test_tracer_start_active_scope(self): + def test_tracer_start_active_span_scope(self): # the Tracer ScopeManager should store the active Scope tracer = self.tracer() - scope = tracer.start_active(operation_name='Fry') + scope = tracer.start_active_span(operation_name='Fry') if self.check_scope_manager(): assert tracer.scope_manager.active() == scope scope.close() - def test_tracer_start_active_nesting(self): + def test_tracer_start_active_span_nesting(self): # when a Scope is closed, the previous one must be activated tracer = self.tracer() - with tracer.start_active(operation_name='Fry') as parent: - with tracer.start_active(operation_name='Farnsworth'): + with tracer.start_active_span(operation_name='Fry') as parent: + with tracer.start_active_span(operation_name='Farnsworth'): pass if self.check_scope_manager(): @@ -329,13 +318,13 @@ def test_tracer_start_active_nesting(self): if self.check_scope_manager(): assert tracer.scope_manager.active() is None - def test_tracer_start_active_nesting_finish_on_close(self): + def test_tracer_start_active_span_nesting_finish_on_close(self): # finish_on_close must be correctly handled tracer = self.tracer() - parent = tracer.start_active(operation_name='Fry', + parent = tracer.start_active_span(operation_name='Fry', finish_on_close=False) with mock.patch.object(parent.span(), 'finish') as finish: - with tracer.start_active(operation_name='Farnsworth'): + with tracer.start_active_span(operation_name='Farnsworth'): pass parent.close() @@ -344,20 +333,20 @@ def test_tracer_start_active_nesting_finish_on_close(self): if self.check_scope_manager(): assert tracer.scope_manager.active() is None - def test_tracer_start_active_wrong_close_order(self): + def test_tracer_start_active_span_wrong_close_order(self): # only the active `Scope` can be closed tracer = self.tracer() - parent = tracer.start_active(operation_name='Fry') - child = tracer.start_active(operation_name='Farnsworth') + parent = tracer.start_active_span(operation_name='Fry') + child = tracer.start_active_span(operation_name='Farnsworth') parent.close() if self.check_scope_manager(): assert tracer.scope_manager.active() == child - def test_tracer_start_manual_scope(self): + def test_tracer_start_span_scope(self): # the Tracer ScopeManager should not store the new Span tracer = self.tracer() - span = tracer.start_manual(operation_name='Fry') + span = tracer.start_span(operation_name='Fry') if self.check_scope_manager(): assert tracer.scope_manager.active() is None @@ -374,7 +363,7 @@ def test_tracer_scope_manager_active(self): def test_tracer_scope_manager_activate(self): # a `ScopeManager` should activate any `Span` tracer = self.tracer() - span = tracer.start_manual(operation_name='Fry') + span = tracer.start_span(operation_name='Fry') tracer.scope_manager.activate(span) if self.check_scope_manager(): diff --git a/opentracing/tracer.py b/opentracing/tracer.py index 7620b7b..3074115 100644 --- a/opentracing/tracer.py +++ b/opentracing/tracer.py @@ -25,8 +25,8 @@ from .span import Span from .span import SpanContext from .scope import Scope -from .propagation import Format, UnsupportedFormatException from .scope_manager import ScopeManager +from .propagation import Format, UnsupportedFormatException class Tracer(object): @@ -50,30 +50,30 @@ def scope_manager(self): """ScopeManager accessor""" return self._scope_manager - def start_active(self, + def start_active_span(self, operation_name=None, child_of=None, references=None, tags=None, start_time=None, - ignore_active_scope=False, - finish_on_close=True): + ignore_active_span=False, + finish_on_close=False): """Returns a newly started and activated `Scope`. Returned `Scope` supports with-statement contexts. For example: - with tracer.start_active('...') as scope: - scope.span().set_tag('http.method', 'GET') + with tracer.start_active_span('...') as scope: + scope.span.set_tag('http.method', 'GET') do_some_work() - # Span is finished automatically outside the `Scope` `with`. + # Span is *not* finished automatically outside the `Scope` `with`. - It's also possible to not finish the `Span` when the `Scope` context + It's also possible to finish the `Span` when the `Scope` context expires: - with tracer.start_active('...', finish_on_close=False) as scope: - scope.span().set_tag('http.method', 'GET') + with tracer.start_active_span('...', finish_on_close=True) as scope: + scope.span.set_tag('http.method', 'GET') do_some_work() - # Span does not finish automatically when the Scope is closed as - # `finish_on_close` is `False` + # Span finishes automatically when the Scope is closed as + # `finish_on_close` is `True` :param operation_name: name of the operation represented by the new span from the perspective of the current service. @@ -88,7 +88,7 @@ def start_active(self, to avoid extra data copying. :param start_time: an explicit Span start time as a unix timestamp per time.time(). - :param ignore_active_scope: an explicit flag that ignores the current + :param ignore_active_span: an explicit flag that ignores the current active `Scope` and creates a root `Span`. :param finish_on_close: whether span should automatically be finished when `Scope#close()` is called. @@ -97,31 +97,31 @@ def start_active(self, """ return self._noop_scope - def start_manual(self, - operation_name=None, - child_of=None, - references=None, - tags=None, - start_time=None, - ignore_active_scope=False): + def start_span(self, + operation_name=None, + child_of=None, + references=None, + tags=None, + start_time=None, + ignore_active_span=False): """Starts and returns a new Span representing a unit of work. Starting a root Span (a Span with no causal references):: - tracer.start_manual('...') + tracer.start_span('...') Starting a child Span (see also start_child_span()):: - tracer.start_manual( + tracer.start_span( '...', child_of=parent_span) Starting a child Span in a more verbose way:: - tracer.start_manual( + tracer.start_span( '...', references=[opentracing.child_of(parent_span)]) @@ -139,22 +139,13 @@ def start_manual(self, to avoid extra data copying. :param start_time: an explicit Span start time as a unix timestamp per time.time() - :param ignore_active_scope: an explicit flag that ignores the current + :param ignore_active_span: an explicit flag that ignores the current active `Scope` and creates a root `Span`. :return: Returns an already-started Span instance. """ return self._noop_span - 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._noop_span - def inject(self, span_context, format, carrier): """Injects `span_context` into `carrier`. @@ -218,7 +209,7 @@ class ReferenceType(object): class Reference(namedtuple('Reference', ['type', 'referenced_context'])): """A Reference pairs a reference type with a referenced SpanContext. - References are used by Tracer.start_manual() to describe the relationships + References are used by Tracer.start_span() to describe the relationships between Spans. Tracer implementations must ignore references where referenced_context is @@ -227,7 +218,7 @@ class Reference(namedtuple('Reference', ['type', 'referenced_context'])): None:: parent_ref = tracer.extract(opentracing.HTTP_HEADERS, request.headers) - span = tracer.start_manual( + span = tracer.start_span( 'operation', references=child_of(parent_ref) ) @@ -243,7 +234,7 @@ def child_of(referenced_context=None): If None is passed, this reference must be ignored by the tracer. :rtype: Reference - :return: A Reference suitable for Tracer.start_manual(..., references=...) + :return: A Reference suitable for Tracer.start_span(..., references=...) """ return Reference( type=ReferenceType.CHILD_OF, @@ -257,7 +248,7 @@ def follows_from(referenced_context=None): If None is passed, this reference must be ignored by the tracer. :rtype: Reference - :return: A Reference suitable for Tracer.start_manual(..., references=...) + :return: A Reference suitable for Tracer.start_span(..., references=...) """ return Reference( type=ReferenceType.FOLLOWS_FROM, @@ -269,7 +260,7 @@ def start_child_span(parent_span, operation_name, tags=None, start_time=None): Equivalent to calling - parent_span.tracer().start_manual( + parent_span.tracer().start_span( operation_name, references=opentracing.child_of(parent_span.context), tags=tags, @@ -286,7 +277,7 @@ def start_child_span(parent_span, operation_name, tags=None, start_time=None): :return: Returns an already-started Span instance. """ - return parent_span.tracer.start_manual( + return parent_span.tracer.start_span( operation_name=operation_name, child_of=parent_span, tags=tags, diff --git a/tests/test_api_check_mixin.py b/tests/test_api_check_mixin.py index 60d24be..52067ac 100644 --- a/tests/test_api_check_mixin.py +++ b/tests/test_api_check_mixin.py @@ -55,37 +55,37 @@ def test_scope_manager_check_works(self): setattr(api_check, 'tracer', lambda: Tracer()) # these tests are expected to succeed - api_check.test_start_active_ignore_active_scope() - api_check.test_start_manual_propagation_ignore_active_scope() + api_check.test_start_active_span_ignore_active_span() + api_check.test_start_span_propagation_ignore_active_span() # no-op tracer doesn't have a ScopeManager implementation # so these tests are expected to work, but asserts to fail with self.assertRaises(AssertionError): - api_check.test_start_active() + api_check.test_start_active_span() with self.assertRaises(AssertionError): - api_check.test_start_active_parent() + api_check.test_start_active_span_parent() with self.assertRaises(AssertionError): - api_check.test_start_active_finish_on_close() + api_check.test_start_active_span_finish_on_close() with self.assertRaises(AssertionError): - api_check.test_start_manual_propagation() + api_check.test_start_span_propagation() with self.assertRaises(AssertionError): - api_check.test_tracer_start_active_scope() + api_check.test_tracer_start_active_span_scope() with self.assertRaises(AssertionError): - api_check.test_tracer_start_active_nesting() + api_check.test_tracer_start_active_span_nesting() with self.assertRaises(AssertionError): - api_check.test_tracer_start_active_nesting_finish_on_close() + api_check.test_tracer_start_active_span_nesting_finish_on_close() with self.assertRaises(AssertionError): - api_check.test_tracer_start_active_wrong_close_order() + api_check.test_tracer_start_active_span_wrong_close_order() with self.assertRaises(AssertionError): - api_check.test_tracer_start_manual_scope() + api_check.test_tracer_start_span_scope() with self.assertRaises(AssertionError): api_check.test_tracer_scope_manager_active() From 9adcd59407f32754e55d91b69bcdc9f43e53f03c Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Thu, 1 Feb 2018 01:58:51 +0100 Subject: [PATCH 10/17] Update Scope/ScopeManager. * ScopeManager and Scope have now properties instead of methods for exposing their members. * Scope now also exposes finish_on_close. * ScopeManager.activate() *requires* the finish_on_close parameter. --- opentracing/harness/api_check.py | 38 ++++++++++++++++---------------- opentracing/scope.py | 17 ++++++++++++-- opentracing/scope_manager.py | 14 ++++++------ opentracing/tracer.py | 2 +- tests/test_scope.py | 6 ++--- tests/test_scope_manager.py | 4 ++-- 6 files changed, 47 insertions(+), 34 deletions(-) diff --git a/opentracing/harness/api_check.py b/opentracing/harness/api_check.py index c0133c3..c6b5a0c 100644 --- a/opentracing/harness/api_check.py +++ b/opentracing/harness/api_check.py @@ -65,9 +65,9 @@ def test_start_active_span(self): tracer = self.tracer() scope = tracer.start_active_span(operation_name='Fry') - assert scope.span() is not None + assert scope.span is not None if self.check_scope_manager(): - assert self.is_parent(None, scope.span()) + assert self.is_parent(None, scope.span) def test_start_active_span_parent(self): # ensure the `ScopeManager` provides the right parenting @@ -75,7 +75,7 @@ def test_start_active_span_parent(self): with tracer.start_active_span(operation_name='Fry') as parent: with tracer.start_active_span(operation_name='Farnsworth') as child: if self.check_scope_manager(): - assert self.is_parent(parent.span(), child.span()) + assert self.is_parent(parent.span, child.span) def test_start_active_span_ignore_active_span(self): # ensure the `ScopeManager` ignores the active `Scope` @@ -85,13 +85,13 @@ def test_start_active_span_ignore_active_span(self): with tracer.start_active_span(operation_name='Farnsworth', ignore_active_span=True) as child: if self.check_scope_manager(): - assert not self.is_parent(parent.span(), child.span()) + assert not self.is_parent(parent.span, child.span) def test_start_active_span_finish_on_close(self): # ensure a `Span` is finished when the `Scope` close tracer = self.tracer() scope = tracer.start_active_span(operation_name='Fry') - with mock.patch.object(scope.span(), 'finish') as finish: + with mock.patch.object(scope.span, 'finish') as finish: scope.close() if self.check_scope_manager(): @@ -102,7 +102,7 @@ def test_start_active_span_not_finish_on_close(self): tracer = self.tracer() scope = tracer.start_active_span(operation_name='Fry', finish_on_close=False) - with mock.patch.object(scope.span(), 'finish') as finish: + with mock.patch.object(scope.span, 'finish') as finish: scope.close() assert finish.call_count == 0 @@ -111,7 +111,7 @@ def test_scope_as_context_manager(self): tracer = self.tracer() with tracer.start_active_span(operation_name='antiquing') as scope: - assert scope.span() is not None + assert scope.span is not None def test_start_span(self): tracer = self.tracer() @@ -129,7 +129,7 @@ def test_start_span_propagation(self): with tracer.start_active_span(operation_name='Fry') as parent: with tracer.start_span(operation_name='Farnsworth') as child: if self.check_scope_manager(): - assert self.is_parent(parent.span(), child) + assert self.is_parent(parent.span, child) def test_start_span_propagation_ignore_active_span(self): # `start_span` doesn't inherit the current active `Scope` span @@ -139,7 +139,7 @@ def test_start_span_propagation_ignore_active_span(self): with tracer.start_span(operation_name='Farnsworth', ignore_active_span=True) as child: if self.check_scope_manager(): - assert not self.is_parent(parent.span(), child) + assert not self.is_parent(parent.span, child) def test_start_span_with_parent(self): tracer = self.tracer() @@ -301,7 +301,7 @@ def test_tracer_start_active_span_scope(self): scope = tracer.start_active_span(operation_name='Fry') if self.check_scope_manager(): - assert tracer.scope_manager.active() == scope + assert tracer.scope_manager.active == scope scope.close() @@ -313,17 +313,17 @@ def test_tracer_start_active_span_nesting(self): pass if self.check_scope_manager(): - assert tracer.scope_manager.active() == parent + assert tracer.scope_manager.active == parent if self.check_scope_manager(): - assert tracer.scope_manager.active() is None + assert tracer.scope_manager.active is None def test_tracer_start_active_span_nesting_finish_on_close(self): # finish_on_close must be correctly handled tracer = self.tracer() parent = tracer.start_active_span(operation_name='Fry', finish_on_close=False) - with mock.patch.object(parent.span(), 'finish') as finish: + with mock.patch.object(parent.span, 'finish') as finish: with tracer.start_active_span(operation_name='Farnsworth'): pass parent.close() @@ -331,7 +331,7 @@ def test_tracer_start_active_span_nesting_finish_on_close(self): assert finish.call_count == 0 if self.check_scope_manager(): - assert tracer.scope_manager.active() is None + assert tracer.scope_manager.active is None def test_tracer_start_active_span_wrong_close_order(self): # only the active `Scope` can be closed @@ -341,7 +341,7 @@ def test_tracer_start_active_span_wrong_close_order(self): parent.close() if self.check_scope_manager(): - assert tracer.scope_manager.active() == child + assert tracer.scope_manager.active == child def test_tracer_start_span_scope(self): # the Tracer ScopeManager should not store the new Span @@ -349,7 +349,7 @@ def test_tracer_start_span_scope(self): span = tracer.start_span(operation_name='Fry') if self.check_scope_manager(): - assert tracer.scope_manager.active() is None + assert tracer.scope_manager.active is None span.finish() @@ -358,13 +358,13 @@ def test_tracer_scope_manager_active(self): tracer = self.tracer() if self.check_scope_manager(): - assert tracer.scope_manager.active() is None + assert tracer.scope_manager.active is None def test_tracer_scope_manager_activate(self): # a `ScopeManager` should activate any `Span` tracer = self.tracer() span = tracer.start_span(operation_name='Fry') - tracer.scope_manager.activate(span) + tracer.scope_manager.activate(span, False) if self.check_scope_manager(): - assert tracer.scope_manager.active().span() == span + assert tracer.scope_manager.active.span == span diff --git a/opentracing/scope.py b/opentracing/scope.py index 95dde38..c54acd2 100644 --- a/opentracing/scope.py +++ b/opentracing/scope.py @@ -30,7 +30,7 @@ class Scope(object): still outstanding. A `Scope` defines when a given `Span` is scheduled and on the path. """ - def __init__(self, manager, span, finish_on_close=True): + def __init__(self, manager, span, finish_on_close): """Initialize a `Scope` for the given `Span` object :param manager: the `ScopeManager` that created this `Scope` @@ -40,14 +40,27 @@ def __init__(self, manager, span, finish_on_close=True): """ self._manager = manager self._span = span + self._finish_on_close = finish_on_close + @property def span(self): """Return the `Span` that's been scoped by this `Scope`.""" return self._span + @property + def manager(self): + """Return the `ScopeManager` that created this `Scope`.""" + return self._manager + + @property + def finish_on_close(self): + """Return whether the span should automatically be finished + when `Scope#close()` is called.""" + return self._finish_on_close + def close(self): """Mark the end of the active period for the current thread and `Scope`, - updating the `ScopeManager#active()` in the process. + updating the `ScopeManager#active` in the process. NOTE: Calling `close()` more than once on a single `Scope` instance leads to undefined behavior. diff --git a/opentracing/scope_manager.py b/opentracing/scope_manager.py index 52e5028..b2a3ffb 100644 --- a/opentracing/scope_manager.py +++ b/opentracing/scope_manager.py @@ -27,16 +27,16 @@ class ScopeManager(object): """The `ScopeManager` interface abstracts both the activation of `Span` instances (via `ScopeManager#activate(Span)`) and access to an active - `Span` / `Scope` (via `ScopeManager#active()`). + `Span` / `Scope` (via `ScopeManager#active`). """ def __init__(self): # TODO: `tracer` should not be None, but we don't have a reference; # should we move the NOOP SpanContext, Span, Scope to somewhere # else so that they're globally reachable? self._noop_span = Span(tracer=None, context=SpanContext()) - self._noop_scope = Scope(self, self._noop_span) + self._noop_scope = Scope(self, self._noop_span, False) - def activate(self, span, finish_on_close=True): + def activate(self, span, finish_on_close): """Make a `Span` instance active. :param span: the `Span` that should become active @@ -44,17 +44,17 @@ def activate(self, span, finish_on_close=True): 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#close()` on the returned instance. """ return self._noop_scope + @property def active(self): """Return the currently active `Scope` which can be used to access the - currently active `Scope#span()`. + 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()` + parent of any newly-created `Span` at `Tracer#start_active_span()` time. :return: the `Scope` that is active, or `None` if not available. diff --git a/opentracing/tracer.py b/opentracing/tracer.py index 3074115..b7fe170 100644 --- a/opentracing/tracer.py +++ b/opentracing/tracer.py @@ -43,7 +43,7 @@ def __init__(self): self._scope_manager = ScopeManager() self._noop_span_context = SpanContext() self._noop_span = Span(tracer=self, context=self._noop_span_context) - self._noop_scope = Scope(self._scope_manager, self._noop_span) + self._noop_scope = Scope(self._scope_manager, self._noop_span, False) @property def scope_manager(self): diff --git a/tests/test_scope.py b/tests/test_scope.py index 7d6f84b..accbaa2 100644 --- a/tests/test_scope.py +++ b/tests/test_scope.py @@ -31,15 +31,15 @@ def test_scope_wrapper(): # ensure `Scope` wraps the `Span` argument span = Span(tracer=Tracer(), context=SpanContext()) - scope = Scope(ScopeManager, span, finish_on_close=False) - assert scope.span() == span + scope = Scope(ScopeManager, span, False) + assert scope.span == span def test_scope_context_manager(): # ensure `Scope` can be used in a Context Manager that # calls the `close()` method span = Span(tracer=Tracer(), context=SpanContext()) - scope = Scope(ScopeManager(), span) + scope = Scope(ScopeManager(), span, True) with mock.patch.object(scope, 'close') as close: with scope: pass diff --git a/tests/test_scope_manager.py b/tests/test_scope_manager.py index 18b8c43..01ae0cd 100644 --- a/tests/test_scope_manager.py +++ b/tests/test_scope_manager.py @@ -29,6 +29,6 @@ def test_scope_manager(): # ensure the activation returns the noop `Scope` that is always active scope_manager = ScopeManager() span = Span(tracer=Tracer(), context=SpanContext()) - scope = scope_manager.activate(span) + scope = scope_manager.activate(span, False) assert scope == scope_manager._noop_scope - assert scope == scope_manager.active() + assert scope == scope_manager.active From 8b1e59c82bdde5cc4f1b5369ca2e4652f73c3ca0 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Thu, 1 Feb 2018 17:59:25 +0100 Subject: [PATCH 11/17] Let Tracer.__init__() receive the scope_manager field. --- opentracing/tracer.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/opentracing/tracer.py b/opentracing/tracer.py index b7fe170..9acaa9b 100644 --- a/opentracing/tracer.py +++ b/opentracing/tracer.py @@ -39,8 +39,8 @@ class Tracer(object): _supported_formats = [Format.TEXT_MAP, Format.BINARY, Format.HTTP_HEADERS] - def __init__(self): - self._scope_manager = ScopeManager() + def __init__(self, scope_manager=ScopeManager()): + self._scope_manager = scope_manager self._noop_span_context = SpanContext() self._noop_span = Span(tracer=self, context=self._noop_span_context) self._noop_scope = Scope(self._scope_manager, self._noop_span, False) From f1ff7c33149d686b38d1ff7b23e1d56b5fb0d0e4 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Thu, 1 Feb 2018 21:34:36 +0100 Subject: [PATCH 12/17] Fix lint. --- opentracing/harness/api_check.py | 13 +++++++------ opentracing/tracer.py | 17 +++++++++-------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/opentracing/harness/api_check.py b/opentracing/harness/api_check.py index c6b5a0c..19a9e1e 100644 --- a/opentracing/harness/api_check.py +++ b/opentracing/harness/api_check.py @@ -73,7 +73,8 @@ def test_start_active_span_parent(self): # ensure the `ScopeManager` provides the right parenting tracer = self.tracer() with tracer.start_active_span(operation_name='Fry') as parent: - with tracer.start_active_span(operation_name='Farnsworth') as child: + with tracer.start_active_span( + operation_name='Farnsworth') as child: if self.check_scope_manager(): assert self.is_parent(parent.span, child.span) @@ -83,7 +84,7 @@ def test_start_active_span_ignore_active_span(self): tracer = self.tracer() with tracer.start_active_span(operation_name='Fry') as parent: with tracer.start_active_span(operation_name='Farnsworth', - ignore_active_span=True) as child: + ignore_active_span=True) as child: if self.check_scope_manager(): assert not self.is_parent(parent.span, child.span) @@ -101,7 +102,7 @@ def test_start_active_span_not_finish_on_close(self): # a `Span` is not finished when the flag is set tracer = self.tracer() scope = tracer.start_active_span(operation_name='Fry', - finish_on_close=False) + finish_on_close=False) with mock.patch.object(scope.span, 'finish') as finish: scope.close() @@ -118,7 +119,7 @@ def test_start_span(self): span = tracer.start_span(operation_name='Fry') span.finish() with tracer.start_span(operation_name='Fry', - tags={'birthday': 'August 14 1974'}) as span: + tags={'birthday': 'August 14 1974'}) as span: span.log_event('birthplace', payload={'hospital': 'Brooklyn Pre-Med Hospital', 'city': 'Old New York'}) @@ -137,7 +138,7 @@ def test_start_span_propagation_ignore_active_span(self): tracer = self.tracer() with tracer.start_active_span(operation_name='Fry') as parent: with tracer.start_span(operation_name='Farnsworth', - ignore_active_span=True) as child: + ignore_active_span=True) as child: if self.check_scope_manager(): assert not self.is_parent(parent.span, child) @@ -322,7 +323,7 @@ def test_tracer_start_active_span_nesting_finish_on_close(self): # finish_on_close must be correctly handled tracer = self.tracer() parent = tracer.start_active_span(operation_name='Fry', - finish_on_close=False) + finish_on_close=False) with mock.patch.object(parent.span, 'finish') as finish: with tracer.start_active_span(operation_name='Farnsworth'): pass diff --git a/opentracing/tracer.py b/opentracing/tracer.py index 9acaa9b..a2f174f 100644 --- a/opentracing/tracer.py +++ b/opentracing/tracer.py @@ -51,13 +51,13 @@ def scope_manager(self): return self._scope_manager def start_active_span(self, - operation_name=None, - child_of=None, - references=None, - tags=None, - start_time=None, - ignore_active_span=False, - finish_on_close=False): + operation_name=None, + child_of=None, + references=None, + tags=None, + start_time=None, + ignore_active_span=False, + finish_on_close=False): """Returns a newly started and activated `Scope`. Returned `Scope` supports with-statement contexts. For example: @@ -69,7 +69,8 @@ def start_active_span(self, It's also possible to finish the `Span` when the `Scope` context expires: - with tracer.start_active_span('...', finish_on_close=True) as scope: + with tracer.start_active_span('...', + finish_on_close=True) as scope: scope.span.set_tag('http.method', 'GET') do_some_work() # Span finishes automatically when the Scope is closed as From d0c7d257ae69d9667442572363840f777d19c002 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Thu, 1 Feb 2018 21:45:16 +0100 Subject: [PATCH 13/17] Update our api_check as we changed the default for finish_on_close. --- opentracing/harness/api_check.py | 8 ++++---- tests/test_api_check_mixin.py | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/opentracing/harness/api_check.py b/opentracing/harness/api_check.py index 19a9e1e..9d68f53 100644 --- a/opentracing/harness/api_check.py +++ b/opentracing/harness/api_check.py @@ -95,18 +95,18 @@ def test_start_active_span_finish_on_close(self): with mock.patch.object(scope.span, 'finish') as finish: scope.close() - if self.check_scope_manager(): - assert finish.call_count == 1 + assert finish.call_count == 0 def test_start_active_span_not_finish_on_close(self): # a `Span` is not finished when the flag is set tracer = self.tracer() scope = tracer.start_active_span(operation_name='Fry', - finish_on_close=False) + finish_on_close=True) with mock.patch.object(scope.span, 'finish') as finish: scope.close() - assert finish.call_count == 0 + if self.check_scope_manager(): + assert finish.call_count == 1 def test_scope_as_context_manager(self): tracer = self.tracer() diff --git a/tests/test_api_check_mixin.py b/tests/test_api_check_mixin.py index 52067ac..03463aa 100644 --- a/tests/test_api_check_mixin.py +++ b/tests/test_api_check_mixin.py @@ -66,9 +66,6 @@ def test_scope_manager_check_works(self): with self.assertRaises(AssertionError): api_check.test_start_active_span_parent() - with self.assertRaises(AssertionError): - api_check.test_start_active_span_finish_on_close() - with self.assertRaises(AssertionError): api_check.test_start_span_propagation() @@ -92,3 +89,6 @@ def test_scope_manager_check_works(self): with self.assertRaises(AssertionError): api_check.test_tracer_scope_manager_activate() + + with self.assertRaises(AssertionError): + api_check.test_start_active_span_not_finish_on_close() From 44eece45e06b0b1af0e8a266e976f11947af91be Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Fri, 2 Feb 2018 02:49:19 +0100 Subject: [PATCH 14/17] Remove the finish_on_close property from Scope. This is a little bit of an implementation attribute, so lets leave it out of the main API. --- opentracing/scope.py | 11 +---------- opentracing/scope_manager.py | 2 +- opentracing/tracer.py | 2 +- tests/test_scope.py | 4 ++-- 4 files changed, 5 insertions(+), 14 deletions(-) diff --git a/opentracing/scope.py b/opentracing/scope.py index c54acd2..23b1153 100644 --- a/opentracing/scope.py +++ b/opentracing/scope.py @@ -30,17 +30,14 @@ class Scope(object): still outstanding. A `Scope` defines when a given `Span` is scheduled and on the path. """ - def __init__(self, manager, span, finish_on_close): + def __init__(self, manager, span): """Initialize a `Scope` for the given `Span` object :param manager: the `ScopeManager` that created this `Scope` :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 @property def span(self): @@ -52,12 +49,6 @@ def manager(self): """Return the `ScopeManager` that created this `Scope`.""" return self._manager - @property - def finish_on_close(self): - """Return whether the span should automatically be finished - when `Scope#close()` is called.""" - return self._finish_on_close - def close(self): """Mark the end of the active period for the current thread and `Scope`, updating the `ScopeManager#active` in the process. diff --git a/opentracing/scope_manager.py b/opentracing/scope_manager.py index b2a3ffb..9c29944 100644 --- a/opentracing/scope_manager.py +++ b/opentracing/scope_manager.py @@ -34,7 +34,7 @@ def __init__(self): # should we move the NOOP SpanContext, Span, Scope to somewhere # else so that they're globally reachable? self._noop_span = Span(tracer=None, context=SpanContext()) - self._noop_scope = Scope(self, self._noop_span, False) + self._noop_scope = Scope(self, self._noop_span) def activate(self, span, finish_on_close): """Make a `Span` instance active. diff --git a/opentracing/tracer.py b/opentracing/tracer.py index a2f174f..99f0a59 100644 --- a/opentracing/tracer.py +++ b/opentracing/tracer.py @@ -43,7 +43,7 @@ def __init__(self, scope_manager=ScopeManager()): self._scope_manager = scope_manager self._noop_span_context = SpanContext() self._noop_span = Span(tracer=self, context=self._noop_span_context) - self._noop_scope = Scope(self._scope_manager, self._noop_span, False) + self._noop_scope = Scope(self._scope_manager, self._noop_span) @property def scope_manager(self): diff --git a/tests/test_scope.py b/tests/test_scope.py index accbaa2..f4a536b 100644 --- a/tests/test_scope.py +++ b/tests/test_scope.py @@ -31,7 +31,7 @@ def test_scope_wrapper(): # ensure `Scope` wraps the `Span` argument span = Span(tracer=Tracer(), context=SpanContext()) - scope = Scope(ScopeManager, span, False) + scope = Scope(ScopeManager, span) assert scope.span == span @@ -39,7 +39,7 @@ def test_scope_context_manager(): # ensure `Scope` can be used in a Context Manager that # calls the `close()` method span = Span(tracer=Tracer(), context=SpanContext()) - scope = Scope(ScopeManager(), span, True) + scope = Scope(ScopeManager(), span) with mock.patch.object(scope, 'close') as close: with scope: pass From 544c7c9141bcf85d1c67f5dd8f529ed3f159fa7d Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Tue, 6 Feb 2018 10:14:54 +0100 Subject: [PATCH 15/17] Tracer.start_active_span() requires operation_name and finish_on_close. --- opentracing/harness/api_check.py | 37 +++++++++++++++----------------- opentracing/tracer.py | 9 ++++---- 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/opentracing/harness/api_check.py b/opentracing/harness/api_check.py index 9d68f53..f46d011 100644 --- a/opentracing/harness/api_check.py +++ b/opentracing/harness/api_check.py @@ -63,7 +63,7 @@ def is_parent(self, parent, span): def test_start_active_span(self): # the first usage returns a `Scope` that wraps a root `Span` tracer = self.tracer() - scope = tracer.start_active_span(operation_name='Fry') + scope = tracer.start_active_span('Fry', False) assert scope.span is not None if self.check_scope_manager(): @@ -72,9 +72,8 @@ def test_start_active_span(self): def test_start_active_span_parent(self): # ensure the `ScopeManager` provides the right parenting tracer = self.tracer() - with tracer.start_active_span(operation_name='Fry') as parent: - with tracer.start_active_span( - operation_name='Farnsworth') as child: + with tracer.start_active_span('Fry', False) as parent: + with tracer.start_active_span('Farnsworth', False) as child: if self.check_scope_manager(): assert self.is_parent(parent.span, child.span) @@ -82,8 +81,8 @@ def test_start_active_span_ignore_active_span(self): # ensure the `ScopeManager` ignores the active `Scope` # if the flag is set tracer = self.tracer() - with tracer.start_active_span(operation_name='Fry') as parent: - with tracer.start_active_span(operation_name='Farnsworth', + with tracer.start_active_span('Fry', False) as parent: + with tracer.start_active_span('Farnsworth', False, ignore_active_span=True) as child: if self.check_scope_manager(): assert not self.is_parent(parent.span, child.span) @@ -91,7 +90,7 @@ def test_start_active_span_ignore_active_span(self): def test_start_active_span_finish_on_close(self): # ensure a `Span` is finished when the `Scope` close tracer = self.tracer() - scope = tracer.start_active_span(operation_name='Fry') + scope = tracer.start_active_span('Fry', False) with mock.patch.object(scope.span, 'finish') as finish: scope.close() @@ -100,8 +99,7 @@ def test_start_active_span_finish_on_close(self): def test_start_active_span_not_finish_on_close(self): # a `Span` is not finished when the flag is set tracer = self.tracer() - scope = tracer.start_active_span(operation_name='Fry', - finish_on_close=True) + scope = tracer.start_active_span('Fry', True) with mock.patch.object(scope.span, 'finish') as finish: scope.close() @@ -111,7 +109,7 @@ def test_start_active_span_not_finish_on_close(self): def test_scope_as_context_manager(self): tracer = self.tracer() - with tracer.start_active_span(operation_name='antiquing') as scope: + with tracer.start_active_span('antiquing', False) as scope: assert scope.span is not None def test_start_span(self): @@ -127,7 +125,7 @@ def test_start_span(self): def test_start_span_propagation(self): # `start_span` must inherit the current active `Scope` span tracer = self.tracer() - with tracer.start_active_span(operation_name='Fry') as parent: + with tracer.start_active_span('Fry', False) as parent: with tracer.start_span(operation_name='Farnsworth') as child: if self.check_scope_manager(): assert self.is_parent(parent.span, child) @@ -136,7 +134,7 @@ def test_start_span_propagation_ignore_active_span(self): # `start_span` doesn't inherit the current active `Scope` span # if the flag is set tracer = self.tracer() - with tracer.start_active_span(operation_name='Fry') as parent: + with tracer.start_active_span('Fry', False) as parent: with tracer.start_span(operation_name='Farnsworth', ignore_active_span=True) as child: if self.check_scope_manager(): @@ -299,7 +297,7 @@ def test_unknown_format(self): def test_tracer_start_active_span_scope(self): # the Tracer ScopeManager should store the active Scope tracer = self.tracer() - scope = tracer.start_active_span(operation_name='Fry') + scope = tracer.start_active_span('Fry', False) if self.check_scope_manager(): assert tracer.scope_manager.active == scope @@ -309,8 +307,8 @@ def test_tracer_start_active_span_scope(self): def test_tracer_start_active_span_nesting(self): # when a Scope is closed, the previous one must be activated tracer = self.tracer() - with tracer.start_active_span(operation_name='Fry') as parent: - with tracer.start_active_span(operation_name='Farnsworth'): + with tracer.start_active_span('Fry', False) as parent: + with tracer.start_active_span('Farnsworth', False): pass if self.check_scope_manager(): @@ -322,10 +320,9 @@ def test_tracer_start_active_span_nesting(self): def test_tracer_start_active_span_nesting_finish_on_close(self): # finish_on_close must be correctly handled tracer = self.tracer() - parent = tracer.start_active_span(operation_name='Fry', - finish_on_close=False) + parent = tracer.start_active_span('Fry', False) with mock.patch.object(parent.span, 'finish') as finish: - with tracer.start_active_span(operation_name='Farnsworth'): + with tracer.start_active_span('Farnsworth', False): pass parent.close() @@ -337,8 +334,8 @@ def test_tracer_start_active_span_nesting_finish_on_close(self): def test_tracer_start_active_span_wrong_close_order(self): # only the active `Scope` can be closed tracer = self.tracer() - parent = tracer.start_active_span(operation_name='Fry') - child = tracer.start_active_span(operation_name='Farnsworth') + parent = tracer.start_active_span('Fry', False) + child = tracer.start_active_span('Farnsworth', False) parent.close() if self.check_scope_manager(): diff --git a/opentracing/tracer.py b/opentracing/tracer.py index 99f0a59..ca914f0 100644 --- a/opentracing/tracer.py +++ b/opentracing/tracer.py @@ -51,13 +51,13 @@ def scope_manager(self): return self._scope_manager def start_active_span(self, - operation_name=None, + operation_name, + finish_on_close, child_of=None, references=None, tags=None, start_time=None, - ignore_active_span=False, - finish_on_close=False): + ignore_active_span=False): """Returns a newly started and activated `Scope`. Returned `Scope` supports with-statement contexts. For example: @@ -69,8 +69,7 @@ def start_active_span(self, It's also possible to finish the `Span` when the `Scope` context expires: - with tracer.start_active_span('...', - finish_on_close=True) as scope: + with tracer.start_active_span('...', True) as scope: scope.span.set_tag('http.method', 'GET') do_some_work() # Span finishes automatically when the Scope is closed as From 999146306e7fe196af661ac285f4aa8fd69edd20 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Tue, 6 Feb 2018 16:05:58 +0100 Subject: [PATCH 16/17] Tracer.__init__() shouldn't share the default ScopeManager. We want to be on the safe side: now by default we let scope_manager=None, and upon checking it within __init__(), create a new ScopeManager if needed, so instances of Tracer don't share it. --- opentracing/tracer.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/opentracing/tracer.py b/opentracing/tracer.py index ca914f0..2614fd3 100644 --- a/opentracing/tracer.py +++ b/opentracing/tracer.py @@ -39,8 +39,9 @@ class Tracer(object): _supported_formats = [Format.TEXT_MAP, Format.BINARY, Format.HTTP_HEADERS] - def __init__(self, scope_manager=ScopeManager()): - self._scope_manager = scope_manager + def __init__(self, scope_manager=None): + self._scope_manager = ScopeManager() if scope_manager is None \ + else scope_manager self._noop_span_context = SpanContext() self._noop_span = Span(tracer=self, context=self._noop_span_context) self._noop_scope = Scope(self._scope_manager, self._noop_span) From 4db96b3a9bfa9f54bf84c3bc53fe009261559312 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Mon, 12 Feb 2018 14:04:42 +0100 Subject: [PATCH 17/17] Update documentation prior to releasing a candidate package. --- README.rst | 79 ++++++++++++++++++++++++++++++------ docs/api.rst | 6 +++ opentracing/scope.py | 14 +++---- opentracing/scope_manager.py | 19 +++++---- opentracing/tracer.py | 17 ++++---- 5 files changed, 99 insertions(+), 36 deletions(-) diff --git a/README.rst b/README.rst index 4ac10af..5d216db 100644 --- a/README.rst +++ b/README.rst @@ -34,18 +34,18 @@ The work of instrumentation libraries generally consists of three steps: Span object in the process. If the request does not contain an active trace, the service starts a new trace and a new *root* Span. 2. The service needs to store the current Span in some request-local storage, - where it can be retrieved from when a child Span must be created, e.g. in case - of the service making an RPC to another service. + (called ``Span`` *activation*) where it can be retrieved from when a child Span must + be created, e.g. in case of the service making an RPC to another service. 3. When making outbound calls to another service, the current Span must be retrieved from request-local storage, a child span must be created (e.g., by using the ``start_child_span()`` helper), and that child span must be embedded into the outbound request (e.g., using HTTP headers) via OpenTracing's inject/extract API. -Below are the code examples for steps 1 and 3. Implementation of request-local -storage needed for step 2 is specific to the service and/or frameworks / -instrumentation libraries it is using (TODO: reference to other OSS projects -with examples of instrumentation). +Below are the code examples for the previously mentioned steps. Implementation +of request-local storage needed for step 2 is specific to the service and/or frameworks / +instrumentation libraries it is using, exposed as a ``ScopeManager`` child contained +as ``Tracer.scope_manager``. See details below. Inbound request ^^^^^^^^^^^^^^^ @@ -56,12 +56,12 @@ Somewhere in your server's request handler code: def handle_request(request): span = before_request(request, opentracing.tracer) - # use span as Context Manager to ensure span.finish() will be called - with span: - # store span in some request-local storage - with RequestContext(span): - # actual business logic - handle_request_for_real(request) + # store span in some request-local storage using Tracer.scope_manager, + # using the returned `Scope` as Context Manager to ensure + # `Span` will be cleared and (in this case) `Span.finish()` be called. + with tracer.scope_manager.activate(span, True) as scope: + # actual business logic + handle_request_for_real(request) def before_request(request, tracer): @@ -141,6 +141,61 @@ Somewhere in your service that's about to make an outgoing call: return outbound_span +Scope and within-process propagation +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +For getting/setting the current active ``Span`` in the used request-local storage, +OpenTracing requires that every ``Tracer`` contains a ``ScopeManager`` that grants +access to the active ``Span`` through a ``Scope``. Any ``Span`` may be transferred to +another task or thread, but not ``Scope``. + +.. code-block:: python + + # Access to the active span is straightforward. + scope = tracer.scope_manager.active() + if scope is not None: + scope.span.set_tag('...', '...') + +The common case starts a ``Scope`` that's automatically registered for intra-process +propagation via ``ScopeManager``. + +Note that ``start_active_span('...', True)`` finishes the span on ``Scope.close()`` +(``start_active_span('...', False)`` does not finish it, in contrast). + +.. code-block:: python + + # Manual activation of the Span. + span = tracer.start_span(operation_name='someWork') + with tracer.scope_manager.activate(span, True) as scope: + # Do things. + + # Automatic activation of the Span. + # finish_on_close is a required parameter. + with tracer.start_active_span('someWork', finish_on_close=True) as scope: + # Do things. + + # Handling done through a try construct: + span = tracer.start_span(operation_name='someWork') + scope = tracer.scope_manager.activate(span, True) + try: + # Do things. + except Exception as e: + scope.set_tag('error', '...') + finally: + scope.finish() + +**If there is a Scope, it will act as the parent to any newly started Span** unless +the programmer passes ``ignore_active_span=True`` at ``start_span()``/``start_active_span()`` +time or specified parent context explicitly: + +.. code-block:: python + + scope = tracer.start_active_span('someWork', ignore_active_span=True) + +Each service/framework ought to provide a specific ``ScopeManager`` implementation +that relies on their own request-local storage (thread-local storage, or coroutine-based storage +for asynchronous frameworks, for example). + Development ----------- diff --git a/docs/api.rst b/docs/api.rst index c7f9d11..5e1bf6f 100644 --- a/docs/api.rst +++ b/docs/api.rst @@ -9,6 +9,12 @@ Classes .. autoclass:: opentracing.SpanContext :members: +.. autoclass:: opentracing.Scope + :members: + +.. autoclass:: opentracing.ScopeManager + :members: + .. autoclass:: opentracing.Tracer :members: diff --git a/opentracing/scope.py b/opentracing/scope.py index 23b1153..9aa321a 100644 --- a/opentracing/scope.py +++ b/opentracing/scope.py @@ -31,7 +31,7 @@ class Scope(object): and on the path. """ def __init__(self, manager, span): - """Initialize a `Scope` for the given `Span` object + """Initializes a `Scope` for the given `Span` object. :param manager: the `ScopeManager` that created this `Scope` :param span: the `Span` used for this `Scope` @@ -41,17 +41,17 @@ def __init__(self, manager, span): @property def span(self): - """Return the `Span` that's been scoped by this `Scope`.""" + """Returns the `Span` wrapped by this `Scope`.""" return self._span @property def manager(self): - """Return the `ScopeManager` that created this `Scope`.""" + """Returns the `ScopeManager` that created this `Scope`.""" return self._manager def close(self): - """Mark the end of the active period for the current thread and `Scope`, - updating the `ScopeManager#active` in the process. + """Marks the end of the active period for this `Scope`, + updating `ScopeManager#active` in the process. NOTE: Calling `close()` more than once on a single `Scope` instance leads to undefined behavior. @@ -59,11 +59,11 @@ def close(self): pass def __enter__(self): - """Allow `Scope` to be used inside a Python Context Manager.""" + """Allows `Scope` to be used inside a Python Context Manager.""" return self def __exit__(self, exc_type, exc_val, exc_tb): - """Call `close()` when the execution is outside the Python + """Calls `close()` when the execution is outside the Python Context Manager. """ self.close() diff --git a/opentracing/scope_manager.py b/opentracing/scope_manager.py index 9c29944..72b0d10 100644 --- a/opentracing/scope_manager.py +++ b/opentracing/scope_manager.py @@ -26,8 +26,8 @@ class ScopeManager(object): """The `ScopeManager` interface abstracts both the activation of `Span` - instances (via `ScopeManager#activate(Span)`) and access to an active - `Span` / `Scope` (via `ScopeManager#active`). + instances (via `ScopeManager#activate(span, finish_on_close)`) and + access to an active `Span` / `Scope` (via `ScopeManager#active`). """ def __init__(self): # TODO: `tracer` should not be None, but we don't have a reference; @@ -37,20 +37,21 @@ def __init__(self): self._noop_scope = Scope(self, self._noop_span) def activate(self, span, finish_on_close): - """Make a `Span` instance active. + """Makes a `Span` instance active. + + :param span: the `Span` that should become active. + :param finish_on_close: whether span should be automatically + finished when `Scope#close()` is called. - :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. + the `Span`. It is a programming error to neglect to call + `Scope#close()` on the returned instance. """ return self._noop_scope @property def active(self): - """Return the currently active `Scope` which can be used to access the + """Returns 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 diff --git a/opentracing/tracer.py b/opentracing/tracer.py index 2614fd3..42ba1e2 100644 --- a/opentracing/tracer.py +++ b/opentracing/tracer.py @@ -60,12 +60,13 @@ def start_active_span(self, start_time=None, ignore_active_span=False): """Returns a newly started and activated `Scope`. - Returned `Scope` supports with-statement contexts. For example: - with tracer.start_active_span('...') as scope: + The returned `Scope` supports with-statement contexts. For example: + + with tracer.start_active_span('...', False) as scope: scope.span.set_tag('http.method', 'GET') do_some_work() - # Span is *not* finished automatically outside the `Scope` `with`. + # Span is not finished outside the `Scope` `with`. It's also possible to finish the `Span` when the `Scope` context expires: @@ -73,11 +74,13 @@ def start_active_span(self, with tracer.start_active_span('...', True) as scope: scope.span.set_tag('http.method', 'GET') do_some_work() - # Span finishes automatically when the Scope is closed as + # Span finishes when the Scope is closed as # `finish_on_close` is `True` :param operation_name: name of the operation represented by the new span from the perspective of the current service. + :param finish_on_close: whether span should automatically be finished + when `Scope#close()` is called. :param child_of: (optional) a Span or SpanContext instance representing the parent in a REFERENCE_CHILD_OF Reference. If specified, the `references` parameter must be omitted. @@ -89,10 +92,8 @@ def start_active_span(self, to avoid extra data copying. :param start_time: an explicit Span start time as a unix timestamp per time.time(). - :param ignore_active_span: an explicit flag that ignores the current - active `Scope` and creates a root `Span`. - :param finish_on_close: whether span should automatically be finished - when `Scope#close()` is called. + :param ignore_active_span: (optional) an explicit flag that ignores + the current active `Scope` and creates a root `Span`. :return: a `Scope`, already registered via the `ScopeManager`. """