From 3e495b9c5e0d3d2fc87239e710c1227cc8675d77 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 9 Sep 2019 19:47:05 +0200 Subject: [PATCH 001/103] Add OpenTracing shim package skeleton --- opentracing-shim/README.rst | 19 ++++ opentracing-shim/setup.py | 56 ++++++++++++ .../src/opentracingshim/__init__.py | 90 +++++++++++++++++++ .../src/opentracingshim/version.py | 15 ++++ 4 files changed, 180 insertions(+) create mode 100644 opentracing-shim/README.rst create mode 100644 opentracing-shim/setup.py create mode 100644 opentracing-shim/src/opentracingshim/__init__.py create mode 100644 opentracing-shim/src/opentracingshim/version.py diff --git a/opentracing-shim/README.rst b/opentracing-shim/README.rst new file mode 100644 index 0000000000..12852fd752 --- /dev/null +++ b/opentracing-shim/README.rst @@ -0,0 +1,19 @@ +OpenTelemetry Python API +============================================================================ + +|pypi| + +.. |pypi| image:: https://badge.fury.io/py/opentelemetry-api.svg + :target: https://pypi.org/project/opentelemetry-api/ + +Installation +------------ + +:: + + pip install opentelemetry-ot-shim + +References +---------- + +* `OpenTelemetry Project `_ diff --git a/opentracing-shim/setup.py b/opentracing-shim/setup.py new file mode 100644 index 0000000000..4e25962fc6 --- /dev/null +++ b/opentracing-shim/setup.py @@ -0,0 +1,56 @@ +# Copyright 2019, OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import os + +import setuptools + +BASE_DIR = os.path.dirname(__file__) +VERSION_FILENAME = os.path.join(BASE_DIR, "src", "opentracingshim", "version.py") +PACKAGE_INFO = {} +with open(VERSION_FILENAME) as f: + exec(f.read(), PACKAGE_INFO) + +setuptools.setup( + name="opentelemetry-ot-shim", + version=PACKAGE_INFO["__version__"], + author="OpenTelemetry Authors", + author_email="cncf-opentelemetry-contributors@lists.cncf.io", + classifiers=[ + "Development Status :: 3 - Alpha", + "Intended Audience :: Developers", + "License :: OSI Approved :: Apache Software License", + "Programming Language :: Python", + "Programming Language :: Python :: 3", + "Programming Language :: Python :: 3.4", + "Programming Language :: Python :: 3.5", + "Programming Language :: Python :: 3.6", + "Programming Language :: Python :: 3.7", + ], + description="OpenTracing shim for OpenTelemetry", + include_package_data=True, + long_description=open("README.rst").read(), + install_requires=["typing; python_version<'3.5'"], + extras_require={}, + license="Apache-2.0", + package_dir={"": "src"}, + packages=setuptools.find_namespace_packages( + where="src", include="opentracingshim.*" + ), + url=( + "https://github.com/open-telemetry/opentelemetry-python" + "/tree/master/opentracing-shim" + ), + zip_safe=False, +) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py new file mode 100644 index 0000000000..401a5faad6 --- /dev/null +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -0,0 +1,90 @@ +# Copyright 2019, OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from opentelemetry.trace import Tracer as OTelTracer +from opentelemetry.trace import Span as OTelSpan +from opentracing import Scope as OTScope +from opentracing import Tracer as OTTracer +from opentracing import Span as OTSpan + +def create_tracer(tracer: OTelTracer) -> OTTracer: + return TracerShim(tracer) + +class SpanShim(OTSpan): + # def __init__(self, span: OTelSpan): + # self._otel_span = span + def __init__(self): + pass + +class ScopeShim(OTScope): + def __init__(self): + self._span = SpanShim() + + @property + def span(self): + return self._span + +class TracerShim(OTTracer): + def __init__(self, tracer: OTelTracer): + self._otel_tracer = tracer + # TODO: Finish implementation. + + @property + def scope_manager(self): + # return self._scope_manager + # TODO: Implement. + pass + + @property + def active_span(self): + # scope = self._scope_manager.active + # return None if scope is None else scope.span + # TODO: Implement. + pass + + def start_active_span(self, + operation_name, + child_of=None, + references=None, + tags=None, + start_time=None, + ignore_active_span=False, + finish_on_close=True) -> ScopeShim: + # TODO: Implement. + return ScopeShim() + + def start_span(self, + operation_name=None, + child_of=None, + references=None, + tags=None, + start_time=None, + ignore_active_span=False): + # return self._noop_span + # TODO: Implement. + pass + + def inject(self, span_context, format, carrier): + # if format in Tracer._supported_formats: + # return + # raise UnsupportedFormatException(format) + # TODO: Implement. + pass + + def extract(self, format, carrier): + # if format in Tracer._supported_formats: + # return self._noop_span_context + # raise UnsupportedFormatException(format) + # TODO: Implement. + pass diff --git a/opentracing-shim/src/opentracingshim/version.py b/opentracing-shim/src/opentracingshim/version.py new file mode 100644 index 0000000000..a457c2b665 --- /dev/null +++ b/opentracing-shim/src/opentracingshim/version.py @@ -0,0 +1,15 @@ +# Copyright 2019, OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +__version__ = "0.1.dev0" From 18a71c10561115da9973c00f001832a1fde54486 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 11 Sep 2019 20:42:33 +0200 Subject: [PATCH 002/103] Test OT shim --- opentracing-shim/tests/__init__.py | 0 opentracing-shim/tests/test_shim.py | 48 +++++++++++++++++++++++++++++ tox.ini | 12 ++++++-- 3 files changed, 57 insertions(+), 3 deletions(-) create mode 100644 opentracing-shim/tests/__init__.py create mode 100644 opentracing-shim/tests/test_shim.py diff --git a/opentracing-shim/tests/__init__.py b/opentracing-shim/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py new file mode 100644 index 0000000000..83c9909586 --- /dev/null +++ b/opentracing-shim/tests/test_shim.py @@ -0,0 +1,48 @@ +# Copyright 2019, OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + +from opentelemetry import trace +from opentelemetry.sdk.trace import Tracer as OTelTracer +from opentracing import Tracer, Scope, Span + +import opentracingshim + +class TestShim(unittest.TestCase): + def setUp(self): + self.tracer = trace.tracer() + self.ot_tracer = opentracingshim.create_tracer(self.tracer) + + @classmethod + def setUpClass(cls): + """Set preferred tracer implementation only once rather than before + every test method. + """ + # TODO: Do we need to call setUpClass() on super()? + # Seems to work fine without it. + super(TestShim, cls).setUpClass() + trace.set_preferred_tracer_implementation(lambda T: OTelTracer()) + + def test_basic(self): + # Verify shim is an OpenTracing tracer. + self.assertIsInstance(self.ot_tracer, Tracer) + + with self.ot_tracer.start_active_span("TestBasic") as scope: + self.assertIsInstance(scope, Scope) + self.assertIsInstance(scope.span, Span) + + def test_set_tag(self): + with self.ot_tracer.start_active_span("TestSetTag") as scope: + scope.span.set_tag("my", "tag") diff --git a/tox.ini b/tox.ini index 19816d3d9c..d5260bffff 100644 --- a/tox.ini +++ b/tox.ini @@ -2,8 +2,8 @@ skipsdist = True skip_missing_interpreters = True envlist = - py3{4,5,6,7,8}-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger} - pypy3-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger} + py3{4,5,6,7,8}-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger,ot-shim} + pypy3-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger,ot-shim} lint py37-{mypy,mypyinstalled} docs @@ -15,6 +15,7 @@ python = [testenv] deps = mypy,mypyinstalled: mypy~=0.740 + ot-shim: opentracing setenv = mypy: MYPYPATH={toxinidir}/opentelemetry-api/src/ @@ -26,6 +27,7 @@ changedir = test-ext-jaeger: ext/opentelemetry-ext-jaeger/tests test-ext-wsgi: ext/opentelemetry-ext-wsgi/tests test-example-app: examples/opentelemetry-example-app/tests + test-ot-shim: opentracing-shim/tests commands_pre = ; Install without -e to test the actual installation @@ -41,6 +43,7 @@ commands_pre = http-requests: pip install {toxinidir}/ext/opentelemetry-ext-http-requests jaeger: pip install {toxinidir}/opentelemetry-sdk jaeger: pip install {toxinidir}/ext/opentelemetry-ext-jaeger + ot-shim: pip install {toxinidir}/opentelemetry-sdk {toxinidir}/opentracing-shim ; Using file:// here because otherwise tox invokes just "pip install ; opentelemetry-api", leading to an error @@ -74,6 +77,7 @@ commands_pre = pip install -e {toxinidir}/ext/opentelemetry-ext-jaeger pip install -e {toxinidir}/ext/opentelemetry-ext-wsgi pip install -e {toxinidir}/examples/opentelemetry-example-app + pip install -e {toxinidir}/opentracing-shim commands = ; Prefer putting everything in one pylint command to profit from duplication @@ -92,7 +96,9 @@ commands = ext/opentelemetry-ext-jaeger/tests/ \ ext/opentelemetry-ext-wsgi/tests/ \ examples/opentelemetry-example-app/src/opentelemetry_example_app/ \ - examples/opentelemetry-example-app/tests/ + examples/opentelemetry-example-app/tests/ \ + opentracing-shim/src/ \ + opentracing-shim/tests/ flake8 . isort --check-only --diff --recursive . From 4745ecac39bc623b8845170f3277cdb3ba99a9be Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 11 Sep 2019 22:29:31 +0200 Subject: [PATCH 003/103] Rename wrapper classes --- opentracing-shim/src/opentracingshim/__init__.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 401a5faad6..53ab5722fe 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -19,23 +19,23 @@ from opentracing import Span as OTSpan def create_tracer(tracer: OTelTracer) -> OTTracer: - return TracerShim(tracer) + return TracerWrapper(tracer) -class SpanShim(OTSpan): +class SpanWrapper(OTSpan): # def __init__(self, span: OTelSpan): # self._otel_span = span def __init__(self): pass -class ScopeShim(OTScope): +class ScopeWrapper(OTScope): def __init__(self): - self._span = SpanShim() + self._span = SpanWrapper() @property def span(self): return self._span -class TracerShim(OTTracer): +class TracerWrapper(OTTracer): def __init__(self, tracer: OTelTracer): self._otel_tracer = tracer # TODO: Finish implementation. @@ -60,9 +60,9 @@ def start_active_span(self, tags=None, start_time=None, ignore_active_span=False, - finish_on_close=True) -> ScopeShim: + finish_on_close=True) -> ScopeWrapper: # TODO: Implement. - return ScopeShim() + return ScopeWrapper() def start_span(self, operation_name=None, From 16d512652a4213e79f177b88d32d1f08add79564 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 12 Sep 2019 00:02:09 +0200 Subject: [PATCH 004/103] Add missing stubs Add missing stubs for fully implementing the Span and Scope classes from OpenTracing. --- .../src/opentracingshim/__init__.py | 103 +++++++++++++++++- 1 file changed, 98 insertions(+), 5 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 53ab5722fe..83d1f22224 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -22,19 +22,112 @@ def create_tracer(tracer: OTelTracer) -> OTTracer: return TracerWrapper(tracer) class SpanWrapper(OTSpan): - # def __init__(self, span: OTelSpan): - # self._otel_span = span - def __init__(self): + # def __init__(self, tracer, context): + # self._tracer = tracer + # self._context = context + def __init__(self, span: OTelSpan): + self._otel_span = span + + @property + def otel_span(self): + """Returns the OpenTelemetry span embedded in the SpanWrapper.""" + return self._otel_span + + @property + def context(self): + # return self._context + pass + + @property + def tracer(self): + # return self._tracer + pass + + def set_operation_name(self, operation_name): + self._otel_span.update_name(operation_name) + return self + + def finish(self, finish_time=None): + pass + + def set_tag(self, key, value): + return self + + def log_kv(self, key_values, timestamp=None): + return self + + def set_baggage_item(self, key, value): + return self + + def get_baggage_item(self, key): + return None + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc_val, exc_tb): + Span._on_error(self, exc_type, exc_val, exc_tb) + self.finish() + + @staticmethod + def _on_error(span, exc_type, exc_val, exc_tb): + # if not span or not exc_val: + # return + + # span.set_tag(tags.ERROR, True) + # span.log_kv({ + # logs.EVENT: tags.ERROR, + # logs.MESSAGE: str(exc_val), + # logs.ERROR_OBJECT: exc_val, + # logs.ERROR_KIND: exc_type, + # logs.STACK: exc_tb, + # }) + pass + + def log_event(self, event, payload=None): + # """DEPRECATED""" + # if payload is None: + # return self.log_kv({logs.EVENT: event}) + # else: + # return self.log_kv({logs.EVENT: event, 'payload': payload}) + pass + + def log(self, **kwargs): + # """DEPRECATED""" + # key_values = {} + # if logs.EVENT in kwargs: + # key_values[logs.EVENT] = kwargs[logs.EVENT] + # if 'payload' in kwargs: + # key_values['payload'] = kwargs['payload'] + # timestamp = None + # if 'timestamp' in kwargs: + # timestamp = kwargs['timestamp'] + # return self.log_kv(key_values, timestamp) pass class ScopeWrapper(OTScope): - def __init__(self): - self._span = SpanWrapper() + def __init__(self, manager, span): + self._manager = manager + self._span = span @property def span(self): return self._span + @property + def manager(self): + return self._manager + + def close(self): + pass + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc_val, exc_tb): + SpanWrapper._on_error(self.span, exc_type, exc_val, exc_tb) + self.close() + class TracerWrapper(OTTracer): def __init__(self, tracer: OTelTracer): self._otel_tracer = tracer From ebfbcd262b6d0aa4919bf7840d263cb4b9b3334e Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 12 Sep 2019 00:02:51 +0200 Subject: [PATCH 005/103] Return an initialized ScopeWrapper --- opentracing-shim/src/opentracingshim/__init__.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 83d1f22224..fa0d6c259a 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -154,8 +154,10 @@ def start_active_span(self, start_time=None, ignore_active_span=False, finish_on_close=True) -> ScopeWrapper: - # TODO: Implement. - return ScopeWrapper() + # TODO: Activate the OTel span. + # otel_span = self._otel_tracer.start_span(operation_name) + otel_span = self._otel_tracer.create_span(operation_name) + return ScopeWrapper(None, SpanWrapper(otel_span)) def start_span(self, operation_name=None, From fea256c46523481c83e9795550a0c321622c0d24 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 12 Sep 2019 00:08:33 +0200 Subject: [PATCH 006/103] Add TODO --- opentracing-shim/tests/test_shim.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 83c9909586..f10404f900 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -42,6 +42,9 @@ def test_basic(self): with self.ot_tracer.start_active_span("TestBasic") as scope: self.assertIsInstance(scope, Scope) self.assertIsInstance(scope.span, Span) + # TODO: Verify that the current span in the OTel Tracer is the + # expected one. + # self.assertEqual(self.tracer.get_current_span(), "fake") def test_set_tag(self): with self.ot_tracer.start_active_span("TestSetTag") as scope: From ad0ac644b4395dd1a86b479a940702b305a683d1 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 12 Sep 2019 00:08:41 +0200 Subject: [PATCH 007/103] Test setting span name --- opentracing-shim/tests/test_shim.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index f10404f900..443539b3bf 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -46,6 +46,13 @@ def test_basic(self): # expected one. # self.assertEqual(self.tracer.get_current_span(), "fake") + def test_set_operation_name(self): + with self.ot_tracer.start_active_span("TestName") as scope: + self.assertEqual(scope.span.otel_span.name, "TestName") + + scope.span.set_operation_name("NewName") + self.assertEqual(scope.span.otel_span.name, "NewName") + def test_set_tag(self): with self.ot_tracer.start_active_span("TestSetTag") as scope: scope.span.set_tag("my", "tag") From 3830cfc3c54466f35280a8765afaa8f01cfd6b6a Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 12 Sep 2019 00:11:46 +0200 Subject: [PATCH 008/103] Run black --- .../src/opentracingshim/__init__.py | 38 +++++++++++-------- opentracing-shim/tests/test_shim.py | 1 + 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index fa0d6c259a..4ec450f245 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -18,9 +18,11 @@ from opentracing import Tracer as OTTracer from opentracing import Span as OTSpan + def create_tracer(tracer: OTelTracer) -> OTTracer: return TracerWrapper(tracer) + class SpanWrapper(OTSpan): # def __init__(self, tracer, context): # self._tracer = tracer @@ -105,6 +107,7 @@ def log(self, **kwargs): # return self.log_kv(key_values, timestamp) pass + class ScopeWrapper(OTScope): def __init__(self, manager, span): self._manager = manager @@ -128,6 +131,7 @@ def __exit__(self, exc_type, exc_val, exc_tb): SpanWrapper._on_error(self.span, exc_type, exc_val, exc_tb) self.close() + class TracerWrapper(OTTracer): def __init__(self, tracer: OTelTracer): self._otel_tracer = tracer @@ -146,26 +150,30 @@ def active_span(self): # TODO: Implement. pass - def start_active_span(self, - operation_name, - child_of=None, - references=None, - tags=None, - start_time=None, - ignore_active_span=False, - finish_on_close=True) -> ScopeWrapper: + def start_active_span( + self, + operation_name, + child_of=None, + references=None, + tags=None, + start_time=None, + ignore_active_span=False, + finish_on_close=True, + ) -> ScopeWrapper: # TODO: Activate the OTel span. # otel_span = self._otel_tracer.start_span(operation_name) otel_span = self._otel_tracer.create_span(operation_name) return ScopeWrapper(None, SpanWrapper(otel_span)) - def start_span(self, - operation_name=None, - child_of=None, - references=None, - tags=None, - start_time=None, - ignore_active_span=False): + def start_span( + self, + operation_name=None, + child_of=None, + references=None, + tags=None, + start_time=None, + ignore_active_span=False, + ): # return self._noop_span # TODO: Implement. pass diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 443539b3bf..8f40fe2102 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -20,6 +20,7 @@ import opentracingshim + class TestShim(unittest.TestCase): def setUp(self): self.tracer = trace.tracer() From 564c1f290c47979dd8def20bf40906b21b98b0fd Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Sat, 14 Sep 2019 00:38:37 +0200 Subject: [PATCH 009/103] Re-organize imports - Explicitly use opentracing.* to make OpenTracing imports easily distinguishable. - Leave OpenTelemetry class names as-is. --- .../src/opentracingshim/__init__.py | 20 +++++++++---------- opentracing-shim/tests/test_shim.py | 13 ++++++------ 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 4ec450f245..8d5be728ae 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -12,22 +12,20 @@ # See the License for the specific language governing permissions and # limitations under the License. -from opentelemetry.trace import Tracer as OTelTracer -from opentelemetry.trace import Span as OTelSpan -from opentracing import Scope as OTScope -from opentracing import Tracer as OTTracer -from opentracing import Span as OTSpan +from opentelemetry.trace import Tracer +from opentelemetry.trace import Span +import opentracing -def create_tracer(tracer: OTelTracer) -> OTTracer: +def create_tracer(tracer: Tracer) -> opentracing.Tracer: return TracerWrapper(tracer) -class SpanWrapper(OTSpan): +class SpanWrapper(opentracing.Span): # def __init__(self, tracer, context): # self._tracer = tracer # self._context = context - def __init__(self, span: OTelSpan): + def __init__(self, span: Span): self._otel_span = span @property @@ -108,7 +106,7 @@ def log(self, **kwargs): pass -class ScopeWrapper(OTScope): +class ScopeWrapper(opentracing.Scope): def __init__(self, manager, span): self._manager = manager self._span = span @@ -132,8 +130,8 @@ def __exit__(self, exc_type, exc_val, exc_tb): self.close() -class TracerWrapper(OTTracer): - def __init__(self, tracer: OTelTracer): +class TracerWrapper(opentracing.Tracer): + def __init__(self, tracer: Tracer): self._otel_tracer = tracer # TODO: Finish implementation. diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 8f40fe2102..4391cf09cc 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -15,9 +15,8 @@ import unittest from opentelemetry import trace -from opentelemetry.sdk.trace import Tracer as OTelTracer -from opentracing import Tracer, Scope, Span - +from opentelemetry.sdk.trace import Tracer +import opentracing import opentracingshim @@ -34,15 +33,15 @@ def setUpClass(cls): # TODO: Do we need to call setUpClass() on super()? # Seems to work fine without it. super(TestShim, cls).setUpClass() - trace.set_preferred_tracer_implementation(lambda T: OTelTracer()) + trace.set_preferred_tracer_implementation(lambda T: Tracer()) def test_basic(self): # Verify shim is an OpenTracing tracer. - self.assertIsInstance(self.ot_tracer, Tracer) + self.assertIsInstance(self.ot_tracer, opentracing.Tracer) with self.ot_tracer.start_active_span("TestBasic") as scope: - self.assertIsInstance(scope, Scope) - self.assertIsInstance(scope.span, Span) + self.assertIsInstance(scope, opentracing.Scope) + self.assertIsInstance(scope.span, opentracing.Span) # TODO: Verify that the current span in the OTel Tracer is the # expected one. # self.assertEqual(self.tracer.get_current_span(), "fake") From bb722182bd55e569f2bbf6dd46f802d821c247dc Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Sat, 14 Sep 2019 00:46:09 +0200 Subject: [PATCH 010/103] Make start_active_span() a context manager Since start_span() is a context manager and since we want the OpenTelemetry Span to stop when the SpanWrapper stops, we need to turn start_active_span() into a context manager and yield the ScopeWrapper rather than return it. --- opentracing-shim/src/opentracingshim/__init__.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 8d5be728ae..1e1aa86906 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -15,6 +15,7 @@ from opentelemetry.trace import Tracer from opentelemetry.trace import Span import opentracing +from contextlib import contextmanager def create_tracer(tracer: Tracer) -> opentracing.Tracer: @@ -148,6 +149,7 @@ def active_span(self): # TODO: Implement. pass + @contextmanager def start_active_span( self, operation_name, @@ -158,10 +160,8 @@ def start_active_span( ignore_active_span=False, finish_on_close=True, ) -> ScopeWrapper: - # TODO: Activate the OTel span. - # otel_span = self._otel_tracer.start_span(operation_name) - otel_span = self._otel_tracer.create_span(operation_name) - return ScopeWrapper(None, SpanWrapper(otel_span)) + with self._otel_tracer.start_span(operation_name) as span: + yield ScopeWrapper(opentracing.ScopeManager, SpanWrapper(span)) def start_span( self, From 419c6f4477f9216ed7a0f16c09cb232977c23300 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Sat, 14 Sep 2019 01:15:46 +0200 Subject: [PATCH 011/103] Verify OTel span ends after OT span ends --- opentracing-shim/tests/test_shim.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 4391cf09cc..f966e013d6 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -44,7 +44,9 @@ def test_basic(self): self.assertIsInstance(scope.span, opentracing.Span) # TODO: Verify that the current span in the OTel Tracer is the # expected one. - # self.assertEqual(self.tracer.get_current_span(), "fake") + + # Verify the span has ended in the OpenTelemetry tracer. + self.assertIsNotNone(scope.span.otel_span.end_time) def test_set_operation_name(self): with self.ot_tracer.start_active_span("TestName") as scope: From d6f522438300b912b9e7371db5659664ecd9619f Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Sat, 14 Sep 2019 01:31:20 +0200 Subject: [PATCH 012/103] Verify OTel span starts and stops properly --- opentracing-shim/tests/test_shim.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index f966e013d6..cdd0fa66bb 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -42,11 +42,13 @@ def test_basic(self): with self.ot_tracer.start_active_span("TestBasic") as scope: self.assertIsInstance(scope, opentracing.Scope) self.assertIsInstance(scope.span, opentracing.Span) - # TODO: Verify that the current span in the OTel Tracer is the - # expected one. + + # Verify the span is active in the OpenTelemetry tracer. + self.assertEqual(self.tracer.get_current_span(), scope.span.otel_span) # Verify the span has ended in the OpenTelemetry tracer. self.assertIsNotNone(scope.span.otel_span.end_time) + self.assertIsNone(self.tracer.get_current_span()) def test_set_operation_name(self): with self.ot_tracer.start_active_span("TestName") as scope: From af35112816dfdbd159baf249266ff35dff128eda Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Sun, 15 Sep 2019 18:08:43 +0200 Subject: [PATCH 013/103] Implement set_tag --- opentracing-shim/src/opentracingshim/__init__.py | 2 +- opentracing-shim/tests/test_shim.py | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 1e1aa86906..3a08cd28a1 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -52,7 +52,7 @@ def finish(self, finish_time=None): pass def set_tag(self, key, value): - return self + self._otel_span.set_attribute(key, value) def log_kv(self, key_values, timestamp=None): return self diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index cdd0fa66bb..c1de4ebdbf 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -59,4 +59,8 @@ def test_set_operation_name(self): def test_set_tag(self): with self.ot_tracer.start_active_span("TestSetTag") as scope: + with self.assertRaises(KeyError): + scope.span.otel_span.attributes["my"] + scope.span.set_tag("my", "tag") + self.assertEqual(scope.span.otel_span.attributes["my"], "tag") From 2ce0fb87ed06b393511ad2d6699614c6b780c880 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 16 Sep 2019 02:28:39 +0200 Subject: [PATCH 014/103] Implement start_span --- opentracing-shim/src/opentracingshim/__init__.py | 8 ++++---- opentracing-shim/tests/test_shim.py | 9 +++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 3a08cd28a1..b70ac5757b 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -171,10 +171,10 @@ def start_span( tags=None, start_time=None, ignore_active_span=False, - ): - # return self._noop_span - # TODO: Implement. - pass + ) -> SpanWrapper: + span = self._otel_tracer.create_span(operation_name) + span.start() + return SpanWrapper(span) def inject(self, span_context, format, carrier): # if format in Tracer._supported_formats: diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index c1de4ebdbf..9de02e3049 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -50,6 +50,15 @@ def test_basic(self): self.assertIsNotNone(scope.span.otel_span.end_time) self.assertIsNone(self.tracer.get_current_span()) + + def test_start_span(self): + span = self.ot_tracer.start_span("TestSpan") + + self.assertIsInstance(span, opentracing.Span) + + # Verify the span is started. + self.assertIsNotNone(span.otel_span.start_time) + def test_set_operation_name(self): with self.ot_tracer.start_active_span("TestName") as scope: self.assertEqual(scope.span.otel_span.name, "TestName") From 3cf9fa33fff54cd06b013a2aa5fa008ac7b7dca6 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 16 Sep 2019 02:31:55 +0200 Subject: [PATCH 015/103] Refactor basic shim test Move type test to a separate method because it's not related to start_active_span(). --- opentracing-shim/tests/test_shim.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 9de02e3049..886675501d 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -35,11 +35,12 @@ def setUpClass(cls): super(TestShim, cls).setUpClass() trace.set_preferred_tracer_implementation(lambda T: Tracer()) - def test_basic(self): + def test_shim_type(self): # Verify shim is an OpenTracing tracer. self.assertIsInstance(self.ot_tracer, opentracing.Tracer) - with self.ot_tracer.start_active_span("TestBasic") as scope: + def test_start_active_span(self): + with self.ot_tracer.start_active_span("TestSpan") as scope: self.assertIsInstance(scope, opentracing.Scope) self.assertIsInstance(scope.span, opentracing.Span) From aa7024ae25c40b82580d65c31a3bd0d29383d101 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 16 Sep 2019 02:33:33 +0200 Subject: [PATCH 016/103] Verify span parent-child relationship --- opentracing-shim/tests/test_shim.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 886675501d..10a9d42f94 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -51,6 +51,14 @@ def test_start_active_span(self): self.assertIsNotNone(scope.span.otel_span.end_time) self.assertIsNone(self.tracer.get_current_span()) + # Verify parent-child relationship. + with self.ot_tracer.start_active_span("ParentSpan") as parent: + parent_trace_id = parent.span.otel_span.get_context().trace_id + + with self.ot_tracer.start_active_span("ChildSpan") as child: + child_trace_id = child.span.otel_span.get_context().trace_id + + self.assertEqual(parent_trace_id, child_trace_id) def test_start_span(self): span = self.ot_tracer.start_span("TestSpan") From 4361e48b38fd86fb249613c33384f4be64541527 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 16 Sep 2019 02:33:47 +0200 Subject: [PATCH 017/103] Ignore wrong pointless-statement Pylint error --- opentracing-shim/tests/test_shim.py | 1 + 1 file changed, 1 insertion(+) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 10a9d42f94..ed450b3bcb 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -78,6 +78,7 @@ def test_set_operation_name(self): def test_set_tag(self): with self.ot_tracer.start_active_span("TestSetTag") as scope: with self.assertRaises(KeyError): + # pylint: disable=pointless-statement scope.span.otel_span.attributes["my"] scope.span.set_tag("my", "tag") From ac128bbd24b646b65bfeed9ce401a3b4ade76ee9 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 16 Sep 2019 16:01:19 +0200 Subject: [PATCH 018/103] Handle explicit parent --- opentracing-shim/src/opentracingshim/__init__.py | 6 +++++- opentracing-shim/tests/test_shim.py | 9 +++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index b70ac5757b..f15a2e47cf 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -172,7 +172,11 @@ def start_span( start_time=None, ignore_active_span=False, ) -> SpanWrapper: - span = self._otel_tracer.create_span(operation_name) + parent = child_of + if parent is not None: + parent = child_of.otel_span + + span = self._otel_tracer.create_span(operation_name, parent) span.start() return SpanWrapper(span) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index ed450b3bcb..0076bad65b 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -68,6 +68,15 @@ def test_start_span(self): # Verify the span is started. self.assertIsNotNone(span.otel_span.start_time) + def test_explicit_parent(self): + parent = self.ot_tracer.start_span("ParentSpan") + child = self.ot_tracer.start_span("ChildSpan", child_of=parent) + + parent_trace_id = parent.otel_span.get_context().trace_id + child_trace_id = child.otel_span.get_context().trace_id + + self.assertEqual(child_trace_id, parent_trace_id) + def test_set_operation_name(self): with self.ot_tracer.start_active_span("TestName") as scope: self.assertEqual(scope.span.otel_span.name, "TestName") From d60927ba3ff0c56f88fd94222bea04e0cc6b741f Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 16 Sep 2019 16:41:49 +0200 Subject: [PATCH 019/103] Add TODOs --- opentracing-shim/src/opentracingshim/__init__.py | 8 ++++++++ opentracing-shim/tests/test_shim.py | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index f15a2e47cf..ae6e9340a2 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -22,6 +22,14 @@ def create_tracer(tracer: Tracer) -> opentracing.Tracer: return TracerWrapper(tracer) +class SpanContextWrapper(opentracing.SpanContext): + # def __init__(self, trace_id, span_id): + # self._trace_id = trace_id + # self._span_id = span_id + pass + # TODO: Implement. + + class SpanWrapper(opentracing.Span): # def __init__(self, tracer, context): # self._tracer = tracer diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 0076bad65b..b10160880a 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -69,6 +69,7 @@ def test_start_span(self): self.assertIsNotNone(span.otel_span.start_time) def test_explicit_parent(self): + # Test explicit parent of type Span. parent = self.ot_tracer.start_span("ParentSpan") child = self.ot_tracer.start_span("ChildSpan", child_of=parent) @@ -77,6 +78,9 @@ def test_explicit_parent(self): self.assertEqual(child_trace_id, parent_trace_id) + # TODO: Test explicit parent of type SpanContext (the above tests only + # with a parent of type Span). + def test_set_operation_name(self): with self.ot_tracer.start_active_span("TestName") as scope: self.assertEqual(scope.span.otel_span.name, "TestName") @@ -92,3 +96,7 @@ def test_set_tag(self): scope.span.set_tag("my", "tag") self.assertEqual(scope.span.otel_span.attributes["my"], "tag") + + def test_span(self): + pass + # TODO: Verify finish() on span. From 8a759cd89452c39c9159e59bb748278c22798f10 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 16 Sep 2019 18:03:15 +0200 Subject: [PATCH 020/103] Implement SpanContextWrapper --- opentracing-shim/src/opentracingshim/__init__.py | 14 +++++++------- opentracing-shim/tests/test_shim.py | 8 ++++++++ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index ae6e9340a2..e13729df11 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -12,8 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -from opentelemetry.trace import Tracer -from opentelemetry.trace import Span +from opentelemetry.trace import Span, SpanContext, Tracer import opentracing from contextlib import contextmanager @@ -23,11 +22,12 @@ def create_tracer(tracer: Tracer) -> opentracing.Tracer: class SpanContextWrapper(opentracing.SpanContext): - # def __init__(self, trace_id, span_id): - # self._trace_id = trace_id - # self._span_id = span_id - pass - # TODO: Implement. + def __init__(self, otel_context: SpanContext): + self._otel_context = otel_context + + @property + def otel_context(self): + return self._otel_context class SpanWrapper(opentracing.Span): diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index b10160880a..19c3d9b497 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -100,3 +100,11 @@ def test_set_tag(self): def test_span(self): pass # TODO: Verify finish() on span. + + def test_span_context(self): + otel_context = trace.SpanContext(1234, 5678) + context = opentracingshim.SpanContextWrapper(otel_context) + + self.assertIsInstance(context, opentracing.SpanContext) + self.assertEqual(context.otel_context.trace_id, 1234) + self.assertEqual(context.otel_context.span_id, 5678) From 190b48095c86a84733f896d4befe5934021ffbcb Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 16 Sep 2019 19:47:28 +0200 Subject: [PATCH 021/103] Add TODOs --- .../src/opentracingshim/__init__.py | 47 ++++++++++--------- opentracing-shim/tests/test_shim.py | 12 +++++ 2 files changed, 37 insertions(+), 22 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index e13729df11..357d3520b4 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -29,6 +29,8 @@ def __init__(self, otel_context: SpanContext): def otel_context(self): return self._otel_context + # TODO: Finish implementation. + class SpanWrapper(opentracing.Span): # def __init__(self, tracer, context): @@ -36,6 +38,7 @@ class SpanWrapper(opentracing.Span): # self._context = context def __init__(self, span: Span): self._otel_span = span + # TODO: Finish initialization. @property def otel_span(self): @@ -46,11 +49,13 @@ def otel_span(self): def context(self): # return self._context pass + # TODO: Implement. @property def tracer(self): # return self._tracer pass + # TODO: Implement. def set_operation_name(self, operation_name): self._otel_span.update_name(operation_name) @@ -58,40 +63,31 @@ def set_operation_name(self, operation_name): def finish(self, finish_time=None): pass + # TODO: Implement. def set_tag(self, key, value): self._otel_span.set_attribute(key, value) def log_kv(self, key_values, timestamp=None): return self + # TODO: Implement. def set_baggage_item(self, key, value): return self + # TODO: Implement. def get_baggage_item(self, key): return None + # TODO: Implement. def __enter__(self): return self def __exit__(self, exc_type, exc_val, exc_tb): - Span._on_error(self, exc_type, exc_val, exc_tb) - self.finish() - - @staticmethod - def _on_error(span, exc_type, exc_val, exc_tb): - # if not span or not exc_val: - # return - - # span.set_tag(tags.ERROR, True) - # span.log_kv({ - # logs.EVENT: tags.ERROR, - # logs.MESSAGE: str(exc_val), - # logs.ERROR_OBJECT: exc_val, - # logs.ERROR_KIND: exc_type, - # logs.STACK: exc_tb, - # }) + # Span._on_error(self, exc_type, exc_val, exc_tb) + # self.finish() pass + # TODO: Implement. def log_event(self, event, payload=None): # """DEPRECATED""" @@ -100,6 +96,7 @@ def log_event(self, event, payload=None): # else: # return self.log_kv({logs.EVENT: event, 'payload': payload}) pass + # TODO: Implement. def log(self, **kwargs): # """DEPRECATED""" @@ -113,6 +110,7 @@ def log(self, **kwargs): # timestamp = kwargs['timestamp'] # return self.log_kv(key_values, timestamp) pass + # TODO: Implement. class ScopeWrapper(opentracing.Scope): @@ -130,13 +128,16 @@ def manager(self): def close(self): pass + # TODO: Implement. def __enter__(self): return self def __exit__(self, exc_type, exc_val, exc_tb): - SpanWrapper._on_error(self.span, exc_type, exc_val, exc_tb) - self.close() + # SpanWrapper._on_error(self.span, exc_type, exc_val, exc_tb) + # self.close() + pass + # TODO: Implement. class TracerWrapper(opentracing.Tracer): @@ -147,15 +148,15 @@ def __init__(self, tracer: Tracer): @property def scope_manager(self): # return self._scope_manager - # TODO: Implement. pass + # TODO: Implement. @property def active_span(self): # scope = self._scope_manager.active # return None if scope is None else scope.span - # TODO: Implement. pass + # TODO: Implement. @contextmanager def start_active_span( @@ -168,6 +169,7 @@ def start_active_span( ignore_active_span=False, finish_on_close=True, ) -> ScopeWrapper: + # TODO: Handle optional arguments. with self._otel_tracer.start_span(operation_name) as span: yield ScopeWrapper(opentracing.ScopeManager, SpanWrapper(span)) @@ -180,6 +182,7 @@ def start_span( start_time=None, ignore_active_span=False, ) -> SpanWrapper: + # TODO: Handle optional arguments. parent = child_of if parent is not None: parent = child_of.otel_span @@ -192,12 +195,12 @@ def inject(self, span_context, format, carrier): # if format in Tracer._supported_formats: # return # raise UnsupportedFormatException(format) - # TODO: Implement. pass + # TODO: Implement. def extract(self, format, carrier): # if format in Tracer._supported_formats: # return self._noop_span_context # raise UnsupportedFormatException(format) - # TODO: Implement. pass + # TODO: Implement. diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 19c3d9b497..c21f4da2ae 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -40,6 +40,8 @@ def test_shim_type(self): self.assertIsInstance(self.ot_tracer, opentracing.Tracer) def test_start_active_span(self): + # TODO: Verify whether we should save `scope` and `span` outside the + # `with` block. with self.ot_tracer.start_active_span("TestSpan") as scope: self.assertIsInstance(scope, opentracing.Scope) self.assertIsInstance(scope.span, opentracing.Span) @@ -55,10 +57,20 @@ def test_start_active_span(self): with self.ot_tracer.start_active_span("ParentSpan") as parent: parent_trace_id = parent.span.otel_span.get_context().trace_id + # TODO: Check active_span() on OT tracer. + with self.ot_tracer.start_active_span("ChildSpan") as child: child_trace_id = child.span.otel_span.get_context().trace_id + # TODO: Check active_span() on OT tracer. + self.assertEqual(parent_trace_id, child_trace_id) + # TODO: Verify that the child span's `parent` field is equal to + # the parent span's `span_id` field. + + # TODO: Check active_span() on OT tracer. + + # TODO: Check active_span() on OT tracer. def test_start_span(self): span = self.ot_tracer.start_span("TestSpan") From 941d629870b0c0692916beca368fbdd2ab463ce0 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 19 Sep 2019 16:12:20 +0200 Subject: [PATCH 022/103] Run Black --- opentracing-shim/setup.py | 4 +++- opentracing-shim/tests/test_shim.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/opentracing-shim/setup.py b/opentracing-shim/setup.py index 4e25962fc6..c8fa935655 100644 --- a/opentracing-shim/setup.py +++ b/opentracing-shim/setup.py @@ -17,7 +17,9 @@ import setuptools BASE_DIR = os.path.dirname(__file__) -VERSION_FILENAME = os.path.join(BASE_DIR, "src", "opentracingshim", "version.py") +VERSION_FILENAME = os.path.join( + BASE_DIR, "src", "opentracingshim", "version.py" +) PACKAGE_INFO = {} with open(VERSION_FILENAME) as f: exec(f.read(), PACKAGE_INFO) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index c21f4da2ae..332e37dfe6 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -47,7 +47,9 @@ def test_start_active_span(self): self.assertIsInstance(scope.span, opentracing.Span) # Verify the span is active in the OpenTelemetry tracer. - self.assertEqual(self.tracer.get_current_span(), scope.span.otel_span) + self.assertEqual( + self.tracer.get_current_span(), scope.span.otel_span + ) # Verify the span has ended in the OpenTelemetry tracer. self.assertIsNotNone(scope.span.otel_span.end_time) From ecb30261b43e4d8fae8e3b7a72caa6bf87e97487 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 19 Sep 2019 16:19:30 +0200 Subject: [PATCH 023/103] Add opentracing to lint dependencies --- tox.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/tox.ini b/tox.ini index d5260bffff..64905d80a6 100644 --- a/tox.ini +++ b/tox.ini @@ -68,6 +68,7 @@ deps = flake8~=3.7 isort~=4.3 black>=19.3b0,==19.* + opentracing~=2.2 commands_pre = pip install -e {toxinidir}/opentelemetry-api From db05d6ca856533ee86bb1ada7fcf25b2e0d078d7 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 19 Sep 2019 18:05:21 +0200 Subject: [PATCH 024/103] Add baggage method stub --- .../src/opentracingshim/__init__.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 357d3520b4..eefe28399f 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -29,7 +29,23 @@ def __init__(self, otel_context: SpanContext): def otel_context(self): return self._otel_context - # TODO: Finish implementation. + @property + def baggage(self): + """ + Return baggage associated with this :class:`SpanContext`. + If no baggage has been added to the :class:`Span`, returns an empty + dict. + + The caller must not modify the returned dictionary. + + See also: :meth:`Span.set_baggage_item()` / + :meth:`Span.get_baggage_item()` + + :rtype: dict + :return: baggage associated with this :class:`SpanContext` or ``{}``. + """ + return {} + # TODO: Implement. class SpanWrapper(opentracing.Span): From 9efa7c51eba0d7f94f3fe8e95625963c4abff3c5 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 20 Sep 2019 17:19:02 +0200 Subject: [PATCH 025/103] Implement active_span() --- opentracing-shim/src/opentracingshim/__init__.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index eefe28399f..ff8473507d 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -169,10 +169,7 @@ def scope_manager(self): @property def active_span(self): - # scope = self._scope_manager.active - # return None if scope is None else scope.span - pass - # TODO: Implement. + return self._otel_tracer.get_current_span() @contextmanager def start_active_span( From 1fe644dee9dbac7facf39dbd1f4b1e1d214abf4f Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 20 Sep 2019 17:21:23 +0200 Subject: [PATCH 026/103] Include context in SpanWrapper --- opentracing-shim/src/opentracingshim/__init__.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index ff8473507d..a3f0e21541 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -49,12 +49,9 @@ def baggage(self): class SpanWrapper(opentracing.Span): - # def __init__(self, tracer, context): - # self._tracer = tracer - # self._context = context - def __init__(self, span: Span): + def __init__(self, tracer, context, span: Span): self._otel_span = span - # TODO: Finish initialization. + opentracing.Span.__init__(self, tracer, context) @property def otel_span(self): @@ -63,8 +60,7 @@ def otel_span(self): @property def context(self): - # return self._context - pass + return self._context # TODO: Implement. @property @@ -184,7 +180,8 @@ def start_active_span( ) -> ScopeWrapper: # TODO: Handle optional arguments. with self._otel_tracer.start_span(operation_name) as span: - yield ScopeWrapper(opentracing.ScopeManager, SpanWrapper(span)) + wrapped_span = SpanWrapper(self, span.get_context(), span) + yield ScopeWrapper(opentracing.ScopeManager, wrapped_span) def start_span( self, @@ -202,7 +199,7 @@ def start_span( span = self._otel_tracer.create_span(operation_name, parent) span.start() - return SpanWrapper(span) + return SpanWrapper(self, span.get_context(), span) def inject(self, span_context, format, carrier): # if format in Tracer._supported_formats: From 43bbac884bc3275d565e08186fa4328e7e76924b Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 20 Sep 2019 17:28:40 +0200 Subject: [PATCH 027/103] Verify active span is correct --- opentracing-shim/tests/test_shim.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 332e37dfe6..0cae12d2f7 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -59,20 +59,24 @@ def test_start_active_span(self): with self.ot_tracer.start_active_span("ParentSpan") as parent: parent_trace_id = parent.span.otel_span.get_context().trace_id - # TODO: Check active_span() on OT tracer. + # Verify parent span is the active span. + self.assertEqual(self.ot_tracer.active_span.context, parent.span.context) with self.ot_tracer.start_active_span("ChildSpan") as child: child_trace_id = child.span.otel_span.get_context().trace_id - # TODO: Check active_span() on OT tracer. + # Verify child span is the active span. + self.assertEqual(self.ot_tracer.active_span.context, child.span.context) self.assertEqual(parent_trace_id, child_trace_id) # TODO: Verify that the child span's `parent` field is equal to # the parent span's `span_id` field. - # TODO: Check active_span() on OT tracer. + # Verify parent span becomes the active span again. + self.assertEqual(self.ot_tracer.active_span.context, parent.span.context) - # TODO: Check active_span() on OT tracer. + # Verify there is no active span. + self.assertIsNone(self.ot_tracer.active_span) def test_start_span(self): span = self.ot_tracer.start_span("TestSpan") From c53ff5688b95e2e0f8b45a92756b5b408eb2b7ea Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 20 Sep 2019 17:38:49 +0200 Subject: [PATCH 028/103] Separate parent-child tests --- opentracing-shim/tests/test_shim.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 0cae12d2f7..9bdd93c2cb 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -55,7 +55,7 @@ def test_start_active_span(self): self.assertIsNotNone(scope.span.otel_span.end_time) self.assertIsNone(self.tracer.get_current_span()) - # Verify parent-child relationship. + def test_parent_child(self): with self.ot_tracer.start_active_span("ParentSpan") as parent: parent_trace_id = parent.span.otel_span.get_context().trace_id @@ -68,6 +68,7 @@ def test_start_active_span(self): # Verify child span is the active span. self.assertEqual(self.ot_tracer.active_span.context, child.span.context) + # Verify parent-child relationship. self.assertEqual(parent_trace_id, child_trace_id) # TODO: Verify that the child span's `parent` field is equal to # the parent span's `span_id` field. From 5e5e6cf7d7460f8205a30c469fa1648230f1d55e Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Sat, 21 Sep 2019 17:02:50 +0200 Subject: [PATCH 029/103] Run Black --- opentracing-shim/tests/test_shim.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 9bdd93c2cb..74670d885f 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -60,13 +60,17 @@ def test_parent_child(self): parent_trace_id = parent.span.otel_span.get_context().trace_id # Verify parent span is the active span. - self.assertEqual(self.ot_tracer.active_span.context, parent.span.context) + self.assertEqual( + self.ot_tracer.active_span.context, parent.span.context + ) with self.ot_tracer.start_active_span("ChildSpan") as child: child_trace_id = child.span.otel_span.get_context().trace_id # Verify child span is the active span. - self.assertEqual(self.ot_tracer.active_span.context, child.span.context) + self.assertEqual( + self.ot_tracer.active_span.context, child.span.context + ) # Verify parent-child relationship. self.assertEqual(parent_trace_id, child_trace_id) @@ -74,7 +78,9 @@ def test_parent_child(self): # the parent span's `span_id` field. # Verify parent span becomes the active span again. - self.assertEqual(self.ot_tracer.active_span.context, parent.span.context) + self.assertEqual( + self.ot_tracer.active_span.context, parent.span.context + ) # Verify there is no active span. self.assertIsNone(self.ot_tracer.active_span) From fe41317e9ef8fefa821d3bf0ff04bc0b4928afb6 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Sat, 21 Sep 2019 17:06:54 +0200 Subject: [PATCH 030/103] Ignore "format" argument in inject/extract The inject() and extract() methods on opentracing.Tracer have an argument called "format", which clashes with a Python builtin. We can't rename the argument in the subclass without generating W0221. --- opentracing-shim/src/opentracingshim/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index a3f0e21541..8c46c1deb5 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -202,6 +202,7 @@ def start_span( return SpanWrapper(self, span.get_context(), span) def inject(self, span_context, format, carrier): + # pylint: disable=redefined-builtin # if format in Tracer._supported_formats: # return # raise UnsupportedFormatException(format) @@ -209,6 +210,7 @@ def inject(self, span_context, format, carrier): # TODO: Implement. def extract(self, format, carrier): + # pylint: disable=redefined-builtin # if format in Tracer._supported_formats: # return self._noop_span_context # raise UnsupportedFormatException(format) From 1bdf9a7ab64b47d6107063902a03e951299b47a8 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Sat, 21 Sep 2019 17:19:09 +0200 Subject: [PATCH 031/103] Disable super-init-not-called warnings --- opentracing-shim/src/opentracingshim/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 8c46c1deb5..b69cbe9073 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -127,6 +127,7 @@ def log(self, **kwargs): class ScopeWrapper(opentracing.Scope): def __init__(self, manager, span): + # pylint: disable=super-init-not-called self._manager = manager self._span = span @@ -154,6 +155,7 @@ def __exit__(self, exc_type, exc_val, exc_tb): class TracerWrapper(opentracing.Tracer): def __init__(self, tracer: Tracer): + # pylint: disable=super-init-not-called self._otel_tracer = tracer # TODO: Finish implementation. From 550b4615e0ae25f1d7fcb300c258c219308e21bd Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Sat, 21 Sep 2019 17:21:19 +0200 Subject: [PATCH 032/103] Sort imports --- opentracing-shim/src/opentracingshim/__init__.py | 6 ++++-- opentracing-shim/tests/test_shim.py | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index b69cbe9073..cb0c89e69c 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -12,10 +12,12 @@ # See the License for the specific language governing permissions and # limitations under the License. -from opentelemetry.trace import Span, SpanContext, Tracer -import opentracing from contextlib import contextmanager +import opentracing + +from opentelemetry.trace import Span, SpanContext, Tracer + def create_tracer(tracer: Tracer) -> opentracing.Tracer: return TracerWrapper(tracer) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 74670d885f..b9076acfc3 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -14,10 +14,11 @@ import unittest -from opentelemetry import trace -from opentelemetry.sdk.trace import Tracer import opentracing + import opentracingshim +from opentelemetry import trace +from opentelemetry.sdk.trace import Tracer class TestShim(unittest.TestCase): From 13433b28fe0fbf709ecf050cf72724eeee36daac Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Sun, 22 Sep 2019 14:44:39 +0200 Subject: [PATCH 033/103] Remove TODO --- opentracing-shim/tests/test_shim.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index b9076acfc3..9d442a730f 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -41,8 +41,6 @@ def test_shim_type(self): self.assertIsInstance(self.ot_tracer, opentracing.Tracer) def test_start_active_span(self): - # TODO: Verify whether we should save `scope` and `span` outside the - # `with` block. with self.ot_tracer.start_active_span("TestSpan") as scope: self.assertIsInstance(scope, opentracing.Scope) self.assertIsInstance(scope.span, opentracing.Span) From 26908a3b4898a9319cb9cd378ad505c4f67698d5 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Sun, 22 Sep 2019 15:05:14 +0200 Subject: [PATCH 034/103] Verify child span's `parent` field is correct Verify the `parent` field of the child span is set to the parent span object. --- opentracing-shim/tests/test_shim.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 9d442a730f..bb302c205e 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -73,8 +73,9 @@ def test_parent_child(self): # Verify parent-child relationship. self.assertEqual(parent_trace_id, child_trace_id) - # TODO: Verify that the child span's `parent` field is equal to - # the parent span's `span_id` field. + self.assertEqual( + child.span.otel_span.parent, parent.span.otel_span + ) # Verify parent span becomes the active span again. self.assertEqual( From d1f1afc8110f8ae1d8d0941c6dc927840c19f2b8 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Tue, 24 Sep 2019 13:34:36 +0200 Subject: [PATCH 035/103] Handle explicit parent of type SpanContext --- opentracing-shim/src/opentracingshim/__init__.py | 5 ++++- opentracing-shim/tests/test_shim.py | 14 ++++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index cb0c89e69c..b3a20df29f 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -199,7 +199,10 @@ def start_span( # TODO: Handle optional arguments. parent = child_of if parent is not None: - parent = child_of.otel_span + if isinstance(parent, SpanWrapper): + parent = child_of.otel_span + elif isinstance(parent, SpanContextWrapper): + parent = child_of.otel_context span = self._otel_tracer.create_span(operation_name, parent) span.start() diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index bb302c205e..f6d3610e4a 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -102,9 +102,19 @@ def test_explicit_parent(self): child_trace_id = child.otel_span.get_context().trace_id self.assertEqual(child_trace_id, parent_trace_id) + self.assertEqual(child.otel_span.parent, parent.otel_span) - # TODO: Test explicit parent of type SpanContext (the above tests only - # with a parent of type Span). + # Test explicit parent of type SpanContext. + parent = self.ot_tracer.start_span("ParentSpan") + child = self.ot_tracer.start_span( + "SpanWithContextParent", child_of=parent.context + ) + + parent_trace_id = parent.otel_span.get_context().trace_id + child_trace_id = child.otel_span.get_context().trace_id + + self.assertEqual(child_trace_id, parent_trace_id) + self.assertEqual(child.otel_span.parent, parent.context) def test_set_operation_name(self): with self.ot_tracer.start_active_span("TestName") as scope: From b16650b65888c2ce483cf90732e7275a0c124144 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Tue, 24 Sep 2019 13:38:20 +0200 Subject: [PATCH 036/103] Rename self.ot_tracer to self.shim --- opentracing-shim/tests/test_shim.py | 32 ++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index f6d3610e4a..62070a1ff7 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -24,7 +24,7 @@ class TestShim(unittest.TestCase): def setUp(self): self.tracer = trace.tracer() - self.ot_tracer = opentracingshim.create_tracer(self.tracer) + self.shim = opentracingshim.create_tracer(self.tracer) @classmethod def setUpClass(cls): @@ -38,10 +38,10 @@ def setUpClass(cls): def test_shim_type(self): # Verify shim is an OpenTracing tracer. - self.assertIsInstance(self.ot_tracer, opentracing.Tracer) + self.assertIsInstance(self.shim, opentracing.Tracer) def test_start_active_span(self): - with self.ot_tracer.start_active_span("TestSpan") as scope: + with self.shim.start_active_span("TestSpan") as scope: self.assertIsInstance(scope, opentracing.Scope) self.assertIsInstance(scope.span, opentracing.Span) @@ -55,20 +55,20 @@ def test_start_active_span(self): self.assertIsNone(self.tracer.get_current_span()) def test_parent_child(self): - with self.ot_tracer.start_active_span("ParentSpan") as parent: + with self.shim.start_active_span("ParentSpan") as parent: parent_trace_id = parent.span.otel_span.get_context().trace_id # Verify parent span is the active span. self.assertEqual( - self.ot_tracer.active_span.context, parent.span.context + self.shim.active_span.context, parent.span.context ) - with self.ot_tracer.start_active_span("ChildSpan") as child: + with self.shim.start_active_span("ChildSpan") as child: child_trace_id = child.span.otel_span.get_context().trace_id # Verify child span is the active span. self.assertEqual( - self.ot_tracer.active_span.context, child.span.context + self.shim.active_span.context, child.span.context ) # Verify parent-child relationship. @@ -79,14 +79,14 @@ def test_parent_child(self): # Verify parent span becomes the active span again. self.assertEqual( - self.ot_tracer.active_span.context, parent.span.context + self.shim.active_span.context, parent.span.context ) # Verify there is no active span. - self.assertIsNone(self.ot_tracer.active_span) + self.assertIsNone(self.shim.active_span) def test_start_span(self): - span = self.ot_tracer.start_span("TestSpan") + span = self.shim.start_span("TestSpan") self.assertIsInstance(span, opentracing.Span) @@ -95,8 +95,8 @@ def test_start_span(self): def test_explicit_parent(self): # Test explicit parent of type Span. - parent = self.ot_tracer.start_span("ParentSpan") - child = self.ot_tracer.start_span("ChildSpan", child_of=parent) + parent = self.shim.start_span("ParentSpan") + child = self.shim.start_span("ChildSpan", child_of=parent) parent_trace_id = parent.otel_span.get_context().trace_id child_trace_id = child.otel_span.get_context().trace_id @@ -105,8 +105,8 @@ def test_explicit_parent(self): self.assertEqual(child.otel_span.parent, parent.otel_span) # Test explicit parent of type SpanContext. - parent = self.ot_tracer.start_span("ParentSpan") - child = self.ot_tracer.start_span( + parent = self.shim.start_span("ParentSpan") + child = self.shim.start_span( "SpanWithContextParent", child_of=parent.context ) @@ -117,14 +117,14 @@ def test_explicit_parent(self): self.assertEqual(child.otel_span.parent, parent.context) def test_set_operation_name(self): - with self.ot_tracer.start_active_span("TestName") as scope: + with self.shim.start_active_span("TestName") as scope: self.assertEqual(scope.span.otel_span.name, "TestName") scope.span.set_operation_name("NewName") self.assertEqual(scope.span.otel_span.name, "NewName") def test_set_tag(self): - with self.ot_tracer.start_active_span("TestSetTag") as scope: + with self.shim.start_active_span("TestSetTag") as scope: with self.assertRaises(KeyError): # pylint: disable=pointless-statement scope.span.otel_span.attributes["my"] From d398c8c4c49aceef40e762a096fa71b2ea15d290 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Tue, 24 Sep 2019 14:02:30 +0200 Subject: [PATCH 037/103] Make active_span return a wrapped span We can't return an OTel object to the consumer of the OT API. --- opentracing-shim/src/opentracingshim/__init__.py | 5 ++++- opentracing-shim/tests/test_shim.py | 7 ++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index b3a20df29f..716bd5f74f 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -169,7 +169,10 @@ def scope_manager(self): @property def active_span(self): - return self._otel_tracer.get_current_span() + span = self._otel_tracer.get_current_span() + if span is None: + return None + return SpanWrapper(self, span.get_context(), span) @contextmanager def start_active_span( diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 62070a1ff7..03fecf26eb 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -46,9 +46,10 @@ def test_start_active_span(self): self.assertIsInstance(scope.span, opentracing.Span) # Verify the span is active in the OpenTelemetry tracer. - self.assertEqual( - self.tracer.get_current_span(), scope.span.otel_span - ) + # TODO: We can't check for equality of self.shim.active_span and + # scope.span because the same OpenTelemetry span is returned inside + # different SpanWrapper objects. Is this a problem? + self.assertEqual(self.shim.active_span.context, scope.span.context) # Verify the span has ended in the OpenTelemetry tracer. self.assertIsNotNone(scope.span.otel_span.end_time) From f214aa7540564064a3cfb3fe8fed8c470007c5ef Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Tue, 24 Sep 2019 16:35:20 +0200 Subject: [PATCH 038/103] Implement tracer property on SpanWrapper --- opentracing-shim/src/opentracingshim/__init__.py | 4 +--- opentracing-shim/tests/test_shim.py | 5 ++++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 716bd5f74f..0e9fb0bf98 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -67,9 +67,7 @@ def context(self): @property def tracer(self): - # return self._tracer - pass - # TODO: Implement. + return self._tracer def set_operation_name(self, operation_name): self._otel_span.update_name(operation_name) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 03fecf26eb..13bd13f7a7 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -134,7 +134,10 @@ def test_set_tag(self): self.assertEqual(scope.span.otel_span.attributes["my"], "tag") def test_span(self): - pass + # Test tracer property. + span = self.shim.start_span("TestSpan") + self.assertEqual(span.tracer, self.shim) + # TODO: Verify finish() on span. def test_span_context(self): From 89ec88c0a8f64e9cbdadf32bc6b565aa49394188 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Tue, 24 Sep 2019 16:42:58 +0200 Subject: [PATCH 039/103] Implement finish() on SpanWrapper --- opentracing-shim/src/opentracingshim/__init__.py | 7 +++++-- opentracing-shim/tests/test_shim.py | 5 ++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 0e9fb0bf98..c0f8693d8a 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -74,8 +74,11 @@ def set_operation_name(self, operation_name): return self def finish(self, finish_time=None): - pass - # TODO: Implement. + self._otel_span.end() + # TODO: Handle finish_time. The OpenTelemetry API doesn't currently + # support setting end time on a span and we cannot assume that all + # OpenTelemetry Tracer implementations have an `end_time` field. + # https://github.com/open-telemetry/opentelemetry-python/issues/134 def set_tag(self, key, value): self._otel_span.set_attribute(key, value) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 13bd13f7a7..d991a398cd 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -138,7 +138,10 @@ def test_span(self): span = self.shim.start_span("TestSpan") self.assertEqual(span.tracer, self.shim) - # TODO: Verify finish() on span. + # Test finish() on span. + self.assertIsNone(span.otel_span.end_time) + span.finish() + self.assertIsNotNone(span.otel_span.end_time) def test_span_context(self): otel_context = trace.SpanContext(1234, 5678) From cdf42b99044873e52790aa7222cc1da51ca38f44 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Tue, 24 Sep 2019 16:44:42 +0200 Subject: [PATCH 040/103] Clean up old comments and docstrings --- .../src/opentracingshim/__init__.py | 40 ------------------- 1 file changed, 40 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index c0f8693d8a..9a73aba8f3 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -33,19 +33,6 @@ def otel_context(self): @property def baggage(self): - """ - Return baggage associated with this :class:`SpanContext`. - If no baggage has been added to the :class:`Span`, returns an empty - dict. - - The caller must not modify the returned dictionary. - - See also: :meth:`Span.set_baggage_item()` / - :meth:`Span.get_baggage_item()` - - :rtype: dict - :return: baggage associated with this :class:`SpanContext` or ``{}``. - """ return {} # TODO: Implement. @@ -63,7 +50,6 @@ def otel_span(self): @property def context(self): return self._context - # TODO: Implement. @property def tracer(self): @@ -99,31 +85,14 @@ def __enter__(self): return self def __exit__(self, exc_type, exc_val, exc_tb): - # Span._on_error(self, exc_type, exc_val, exc_tb) - # self.finish() pass # TODO: Implement. def log_event(self, event, payload=None): - # """DEPRECATED""" - # if payload is None: - # return self.log_kv({logs.EVENT: event}) - # else: - # return self.log_kv({logs.EVENT: event, 'payload': payload}) pass # TODO: Implement. def log(self, **kwargs): - # """DEPRECATED""" - # key_values = {} - # if logs.EVENT in kwargs: - # key_values[logs.EVENT] = kwargs[logs.EVENT] - # if 'payload' in kwargs: - # key_values['payload'] = kwargs['payload'] - # timestamp = None - # if 'timestamp' in kwargs: - # timestamp = kwargs['timestamp'] - # return self.log_kv(key_values, timestamp) pass # TODO: Implement. @@ -150,8 +119,6 @@ def __enter__(self): return self def __exit__(self, exc_type, exc_val, exc_tb): - # SpanWrapper._on_error(self.span, exc_type, exc_val, exc_tb) - # self.close() pass # TODO: Implement. @@ -164,7 +131,6 @@ def __init__(self, tracer: Tracer): @property def scope_manager(self): - # return self._scope_manager pass # TODO: Implement. @@ -214,16 +180,10 @@ def start_span( def inject(self, span_context, format, carrier): # pylint: disable=redefined-builtin - # if format in Tracer._supported_formats: - # return - # raise UnsupportedFormatException(format) pass # TODO: Implement. def extract(self, format, carrier): # pylint: disable=redefined-builtin - # if format in Tracer._supported_formats: - # return self._noop_span_context - # raise UnsupportedFormatException(format) pass # TODO: Implement. From ac45a5c1b25fa7b6dd839827e6937e00c570ce33 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 27 Sep 2019 16:52:17 +0200 Subject: [PATCH 041/103] Implement log_kv --- .../src/opentracingshim/__init__.py | 13 +++++++++-- opentracing-shim/src/opentracingshim/util.py | 23 +++++++++++++++++++ opentracing-shim/tests/test_shim.py | 14 +++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 opentracing-shim/src/opentracingshim/util.py diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 9a73aba8f3..6a4e98fef0 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -16,7 +16,8 @@ import opentracing -from opentelemetry.trace import Span, SpanContext, Tracer +from opentelemetry.trace import Event, Span, SpanContext, Tracer +from opentracingshim import util def create_tracer(tracer: Tracer) -> opentracing.Tracer: @@ -70,8 +71,16 @@ def set_tag(self, key, value): self._otel_span.set_attribute(key, value) def log_kv(self, key_values, timestamp=None): + # TODO: Is it OK to set the event name to an empty string? log_kv + # doesn't have a `name` argument. + if timestamp is None: + timestamp = util.time_ns() + + event = Event("", timestamp, key_values) + self._otel_span.add_lazy_event(event) + + # Return self for call chaining. return self - # TODO: Implement. def set_baggage_item(self, key, value): return self diff --git a/opentracing-shim/src/opentracingshim/util.py b/opentracing-shim/src/opentracingshim/util.py new file mode 100644 index 0000000000..d814433eee --- /dev/null +++ b/opentracing-shim/src/opentracingshim/util.py @@ -0,0 +1,23 @@ +# Copyright 2019, OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import time + +try: + time_ns = time.time_ns +# Python versions < 3.7 +except AttributeError: + + def time_ns(): + return int(time.time() * 1e9) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index d991a398cd..7a0bbb9dbd 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import time import unittest import opentracing @@ -143,6 +144,19 @@ def test_span(self): span.finish() self.assertIsNotNone(span.otel_span.end_time) + def test_log_kv(self): + span = self.shim.start_span("TestSpan") + + span.log_kv({"foo": "bar"}) + self.assertEqual(span.otel_span.events[0].attributes["foo"], "bar") + # Verify timestamp was generated automatically. + self.assertIsNotNone(span.otel_span.events[0].timestamp) + + # Test explicit timestamp. + now = time.time() + span.log_kv({"foo": "bar"}, now) + self.assertEqual(span.otel_span.events[1].timestamp, now) + def test_span_context(self): otel_context = trace.SpanContext(1234, 5678) context = opentracingshim.SpanContextWrapper(otel_context) From a21f490afa6dd81571f01a151ec250c050aac985 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 30 Sep 2019 14:15:21 +0200 Subject: [PATCH 042/103] Handle event name in log_kv() Since the OpenTelemetry API expects an event name and the OpenTracing API doesn't provide this information, we use a helper function to derive an event name from the key-value pairs passed to the `log_kv()` method. The helper function is adapted from the Java OpenTracing bridge implementation: https://github.com/open-telemetry/opentelemetry-java/blob/2379965a97c306f160de510d47daaaa77ef32550/opentracing_shim/src/main/java/io/opentelemetry/opentracingshim/SpanShim.java#L152 --- .../src/opentracingshim/__init__.py | 5 +-- opentracing-shim/src/opentracingshim/util.py | 16 ++++++++ opentracing-shim/tests/test_util.py | 37 +++++++++++++++++++ 3 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 opentracing-shim/tests/test_util.py diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 6a4e98fef0..5911740f39 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -71,12 +71,11 @@ def set_tag(self, key, value): self._otel_span.set_attribute(key, value) def log_kv(self, key_values, timestamp=None): - # TODO: Is it OK to set the event name to an empty string? log_kv - # doesn't have a `name` argument. if timestamp is None: timestamp = util.time_ns() - event = Event("", timestamp, key_values) + event_name = util.event_name_from_kv(key_values) + event = Event(event_name, timestamp, key_values) self._otel_span.add_lazy_event(event) # Return self for call chaining. diff --git a/opentracing-shim/src/opentracingshim/util.py b/opentracing-shim/src/opentracingshim/util.py index d814433eee..188966cedd 100644 --- a/opentracing-shim/src/opentracingshim/util.py +++ b/opentracing-shim/src/opentracingshim/util.py @@ -21,3 +21,19 @@ def time_ns(): return int(time.time() * 1e9) + + +# A default event name to be used for logging events when a better event name +# can't be derived from the event's key-value pairs. +DEFAULT_EVENT_NAME = "log" + + +def event_name_from_kv(key_values: dict) -> str: + """A helper function which returns an event name from the given dict, or a + default event name. + """ + + if key_values is None or "event" not in key_values: + return DEFAULT_EVENT_NAME + + return key_values["event"] diff --git a/opentracing-shim/tests/test_util.py b/opentracing-shim/tests/test_util.py new file mode 100644 index 0000000000..3b2dabcc89 --- /dev/null +++ b/opentracing-shim/tests/test_util.py @@ -0,0 +1,37 @@ +# Copyright 2019, OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + +from opentracingshim import util + + +class TestUtil(unittest.TestCase): + def test_event_name_from_kv(self): + # Test basic behavior. + event_name = "send HTTP request" + res = util.event_name_from_kv({"event": event_name, "foo": "bar"}) + self.assertEqual(res, event_name) + + # Test None. + res = util.event_name_from_kv(None) + self.assertEqual(res, util.DEFAULT_EVENT_NAME) + + # Test empty dict. + res = util.event_name_from_kv({}) + self.assertEqual(res, util.DEFAULT_EVENT_NAME) + + # Test missing `event` field. + res = util.event_name_from_kv({"foo": "bar"}) + self.assertEqual(res, util.DEFAULT_EVENT_NAME) From b9c8ae393fc222c82cad6cd4a5896dad91d8916d Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 30 Sep 2019 19:24:16 +0200 Subject: [PATCH 043/103] Remove unused context manager methods `__enter__()` and `__exit__()` on `SpanWrapper` aren't needed because the same methods on the base class `opentracing.Span` do what we need them to do when `SpanWrapper` is used as a context manager. `__enter__()` and `__exit__()` on `ScopeWrapper` aren't needed because we use the `@contextmanager` decorator on `start_active_span(). When the `with start_active_span()` block ends, the `with` block in the *implementation* of `start_active_span()` ends as well, which in turn makes the `with` block in OpenTelemetry's `start_span()` end, which makes the `finally` block in `use_span()` run, which calls `end()` on the OpenTelemetry span. --- opentracing-shim/src/opentracingshim/__init__.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 5911740f39..c76c76337e 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -89,13 +89,6 @@ def get_baggage_item(self, key): return None # TODO: Implement. - def __enter__(self): - return self - - def __exit__(self, exc_type, exc_val, exc_tb): - pass - # TODO: Implement. - def log_event(self, event, payload=None): pass # TODO: Implement. @@ -123,13 +116,6 @@ def close(self): pass # TODO: Implement. - def __enter__(self): - return self - - def __exit__(self, exc_type, exc_val, exc_tb): - pass - # TODO: Implement. - class TracerWrapper(opentracing.Tracer): def __init__(self, tracer: Tracer): From 5bf6592dfaf8491818805ff197b5184b94d47362 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 30 Sep 2019 19:42:06 +0200 Subject: [PATCH 044/103] Verify context manager behavior of `start_span()` --- opentracing-shim/tests/test_shim.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 7a0bbb9dbd..c5fd6db825 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -88,12 +88,14 @@ def test_parent_child(self): self.assertIsNone(self.shim.active_span) def test_start_span(self): - span = self.shim.start_span("TestSpan") + with self.shim.start_span("TestSpan") as span: + self.assertIsInstance(span, opentracing.Span) - self.assertIsInstance(span, opentracing.Span) + # Verify the span is started. + self.assertIsNotNone(span.otel_span.start_time) - # Verify the span is started. - self.assertIsNotNone(span.otel_span.start_time) + # Verify the span has ended. + self.assertIsNotNone(span.otel_span.end_time) def test_explicit_parent(self): # Test explicit parent of type Span. From 5eba2678be12743be9dd428a1d19affa9d45c959 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 30 Sep 2019 19:48:27 +0200 Subject: [PATCH 045/103] Remove deprecated logging methods on `SpanWrapper` --- opentracing-shim/src/opentracingshim/__init__.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index c76c76337e..68e4e8a358 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -89,13 +89,9 @@ def get_baggage_item(self, key): return None # TODO: Implement. - def log_event(self, event, payload=None): - pass - # TODO: Implement. - - def log(self, **kwargs): - pass - # TODO: Implement. + # TODO: Verify calls to deprecated methods `log_event()` and `log()` on + # base class work properly (it's probably fine because both methods call + # `log_kv()`). class ScopeWrapper(opentracing.Scope): From 29e75a9895cab8f22cf4d9ee68f54ee3231732da Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Tue, 1 Oct 2019 15:53:35 +0200 Subject: [PATCH 046/103] Refactor tests --- opentracing-shim/tests/test_shim.py | 88 ++++++++++++++++++++++------- 1 file changed, 69 insertions(+), 19 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index c5fd6db825..5215316545 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -41,39 +41,84 @@ def test_shim_type(self): # Verify shim is an OpenTracing tracer. self.assertIsInstance(self.shim, opentracing.Tracer) - def test_start_active_span(self): + def test_basic_behavior(self): + """Test span creation, activation and deactivation.""" + with self.shim.start_active_span("TestSpan") as scope: + # Verify correct type of Scope and Span objects. self.assertIsInstance(scope, opentracing.Scope) self.assertIsInstance(scope.span, opentracing.Span) + # Verify the span is started. + self.assertIsNotNone(scope.span.otel_span.start_time) + # Verify the span is active in the OpenTelemetry tracer. # TODO: We can't check for equality of self.shim.active_span and # scope.span because the same OpenTelemetry span is returned inside - # different SpanWrapper objects. Is this a problem? + # different SpanWrapper objects. A possible solution is described + # here: + # https://github.com/open-telemetry/opentelemetry-python/issues/161#issuecomment-534136274 self.assertEqual(self.shim.active_span.context, scope.span.context) # Verify the span has ended in the OpenTelemetry tracer. self.assertIsNotNone(scope.span.otel_span.end_time) + + # Verify no span is active on the OpenTelemetry tracer. self.assertIsNone(self.tracer.get_current_span()) - def test_parent_child(self): - with self.shim.start_active_span("ParentSpan") as parent: - parent_trace_id = parent.span.otel_span.get_context().trace_id + def test_start_span(self): + """Test span creation using `start_span()`.""" + + with self.shim.start_span("TestSpan") as span: + # Verify correct type of Span object. + self.assertIsInstance(span, opentracing.Span) + + # Verify the span is started. + self.assertIsNotNone(span.otel_span.start_time) + # Verify the span has ended in the OpenTelemetry tracer. + self.assertIsNotNone(span.otel_span.end_time) + + # Verify `start_span()` does NOT make the span active. + self.assertIsNone(self.shim.active_span) + + def test_start_span_no_contextmanager(self): + """Test `start_span()` without a `with` statement.""" + + span = self.shim.start_span("TestSpan") + + # Verify the span is started. + self.assertIsNotNone(span.otel_span.start_time) + + # Verify `start_span()` does NOT make the span active. + self.assertIsNone(self.shim.active_span) + + span.finish() + + # Verify the span has ended in the OpenTelemetry tracer. + self.assertIsNotNone(span.otel_span.end_time) + + def test_parent_child_implicit(self): + """Test parent-child relationship of spans without specifying the + parent span upon creation, as well as span activation/deactivation. + """ + + with self.shim.start_active_span("ParentSpan") as parent: # Verify parent span is the active span. self.assertEqual( self.shim.active_span.context, parent.span.context ) with self.shim.start_active_span("ChildSpan") as child: - child_trace_id = child.span.otel_span.get_context().trace_id - # Verify child span is the active span. self.assertEqual( self.shim.active_span.context, child.span.context ) # Verify parent-child relationship. + parent_trace_id = parent.span.otel_span.get_context().trace_id + child_trace_id = child.span.otel_span.get_context().trace_id + self.assertEqual(parent_trace_id, child_trace_id) self.assertEqual( child.span.otel_span.parent, parent.span.otel_span @@ -81,24 +126,23 @@ def test_parent_child(self): # Verify parent span becomes the active span again. self.assertEqual( - self.shim.active_span.context, parent.span.context + self.shim.active_span.context, + parent.span.context + # TODO: Check equality of the spans themselves rather than + # their context once the SpanWrapper reconstruction problem + # has been addressed (see previous TODO). ) # Verify there is no active span. self.assertIsNone(self.shim.active_span) - def test_start_span(self): - with self.shim.start_span("TestSpan") as span: - self.assertIsInstance(span, opentracing.Span) - - # Verify the span is started. - self.assertIsNotNone(span.otel_span.start_time) + def test_parent_child_explicit_span(self): + """Test parent-child relationship of spans when specifying a `Span` + object as a parent upon creation. + """ - # Verify the span has ended. - self.assertIsNotNone(span.otel_span.end_time) + # TODO: Test explicit parent also with `start_active_span()`. - def test_explicit_parent(self): - # Test explicit parent of type Span. parent = self.shim.start_span("ParentSpan") child = self.shim.start_span("ChildSpan", child_of=parent) @@ -108,7 +152,13 @@ def test_explicit_parent(self): self.assertEqual(child_trace_id, parent_trace_id) self.assertEqual(child.otel_span.parent, parent.otel_span) - # Test explicit parent of type SpanContext. + def test_parent_child_explicit_span_context(self): + """Test parent-child relationship of spans when specifying a + `SpanContext` object as a parent upon creation. + """ + + # TODO: Test explicit parent also with `start_active_span()`. + parent = self.shim.start_span("ParentSpan") child = self.shim.start_span( "SpanWithContextParent", child_of=parent.context From 1537c529a21b9647775229644a000879cc2a4213 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 13:57:34 +0200 Subject: [PATCH 047/103] Implement ScopeManager --- .../src/opentracingshim/__init__.py | 32 ++++++++++++++++--- opentracing-shim/tests/test_shim.py | 17 ++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 68e4e8a358..342a406677 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -113,16 +113,40 @@ def close(self): # TODO: Implement. -class TracerWrapper(opentracing.Tracer): +class ScopeManager(opentracing.ScopeManager): def __init__(self, tracer: Tracer): + # pylint: disable=super-init-not-called + self._tracer = tracer + + @contextmanager + def activate(self, span, finish_on_close): + with self._tracer.use_span( + span.otel_span, end_on_exit=finish_on_close + ) as otel_span: + wrapped_span = SpanWrapper( + self._tracer, otel_span.get_context(), otel_span + ) + yield ScopeWrapper(self, wrapped_span) + + @property + def active(self): + span = self._tracer.get_current_span() + wrapped_span = SpanWrapper(self._tracer, span.get_context(), span) + return ScopeWrapper(self, wrapped_span) + + +class TracerWrapper(opentracing.Tracer): + def __init__(self, tracer: Tracer, scope_manager: ScopeManager = None): # pylint: disable=super-init-not-called self._otel_tracer = tracer - # TODO: Finish implementation. + if scope_manager is not None: + self._scope_manager = scope_manager + else: + self._scope_manager = ScopeManager(tracer) @property def scope_manager(self): - pass - # TODO: Implement. + return self._scope_manager @property def active_span(self): diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 5215316545..e39c68ea22 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -170,6 +170,23 @@ def test_parent_child_explicit_span_context(self): self.assertEqual(child_trace_id, parent_trace_id) self.assertEqual(child.otel_span.parent, parent.context) + def test_explicit_span_activation(self): + """Test manual activation and deactivation of a span.""" + + span = self.shim.start_span("TestSpan") + + # Verify no span is currently active. + self.assertIsNone(self.shim.active_span) + + with self.shim.scope_manager.activate( + span, finish_on_close=True + ) as scope: + # Verify span is active. + self.assertEqual(self.shim.active_span.context, scope.span.context) + + # Verify no span is active. + self.assertIsNone(self.shim.active_span) + def test_set_operation_name(self): with self.shim.start_active_span("TestName") as scope: self.assertEqual(scope.span.otel_span.name, "TestName") From 25ceeca9dc59c5b5c8c1cd9eb5905dd305cf8340 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 14:59:44 +0200 Subject: [PATCH 048/103] Implement close() on ScopeWrapper --- opentracing-shim/src/opentracingshim/__init__.py | 5 +++-- opentracing-shim/tests/test_shim.py | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 342a406677..f269914056 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -109,8 +109,9 @@ def manager(self): return self._manager def close(self): - pass - # TODO: Implement. + self._span.finish() + # TODO: Set active span on OpenTelemetry tracer. + # https://github.com/open-telemetry/opentelemetry-python/issues/161#issuecomment-534136274 class ScopeManager(opentracing.ScopeManager): diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index e39c68ea22..9440ca8db1 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -233,3 +233,17 @@ def test_span_context(self): self.assertIsInstance(context, opentracing.SpanContext) self.assertEqual(context.otel_context.trace_id, 1234) self.assertEqual(context.otel_context.span_id, 5678) + + def test_explicit_scope_close(self): + """Test `close()` method on `ScopeWrapper`.""" + + span = self.shim.start_span("TestSpan") + scope = opentracingshim.ScopeWrapper(self.shim.scope_manager, span) + + # Verify span hasn't ended. + self.assertIsNone(span.otel_span.end_time) + + scope.close() + + # Verify span has ended. + self.assertIsNotNone(span.otel_span.end_time) From f1e4cba879f725bbde270eea7df40387818020db Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 15:45:05 +0200 Subject: [PATCH 049/103] Rename ScopeManager to ScopeManagerWrapper --- opentracing-shim/src/opentracingshim/__init__.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index f269914056..221acf8802 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -114,7 +114,7 @@ def close(self): # https://github.com/open-telemetry/opentelemetry-python/issues/161#issuecomment-534136274 -class ScopeManager(opentracing.ScopeManager): +class ScopeManagerWrapper(opentracing.ScopeManager): def __init__(self, tracer: Tracer): # pylint: disable=super-init-not-called self._tracer = tracer @@ -137,13 +137,15 @@ def active(self): class TracerWrapper(opentracing.Tracer): - def __init__(self, tracer: Tracer, scope_manager: ScopeManager = None): + def __init__( + self, tracer: Tracer, scope_manager: ScopeManagerWrapper = None + ): # pylint: disable=super-init-not-called self._otel_tracer = tracer if scope_manager is not None: self._scope_manager = scope_manager else: - self._scope_manager = ScopeManager(tracer) + self._scope_manager = ScopeManagerWrapper(tracer) @property def scope_manager(self): From 4bf91c1cd14e59efb4c45c0e02c8b4e8da461ab6 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 17:18:11 +0200 Subject: [PATCH 050/103] Implement start_active_span() using start_span() --- opentracing-shim/src/opentracingshim/__init__.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 221acf8802..54f11d8611 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -169,10 +169,16 @@ def start_active_span( ignore_active_span=False, finish_on_close=True, ) -> ScopeWrapper: - # TODO: Handle optional arguments. - with self._otel_tracer.start_span(operation_name) as span: - wrapped_span = SpanWrapper(self, span.get_context(), span) - yield ScopeWrapper(opentracing.ScopeManager, wrapped_span) + span = self.start_span( + operation_name=operation_name, + child_of=child_of, + references=references, + tags=tags, + start_time=start_time, + ignore_active_span=ignore_active_span, + ) + with self._scope_manager.activate(span, finish_on_close) as scope: + yield scope def start_span( self, From 6f9d22c67ee2b4d7e0d3729404f6e5eb4d8cb22f Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 17:20:06 +0200 Subject: [PATCH 051/103] Wrap context in start_span() We shouldn't return an OpenTelemetry object to OpenTracing API calls. --- opentracing-shim/src/opentracingshim/__init__.py | 3 ++- opentracing-shim/tests/test_shim.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 54f11d8611..85d9ce1f70 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -199,7 +199,8 @@ def start_span( span = self._otel_tracer.create_span(operation_name, parent) span.start() - return SpanWrapper(self, span.get_context(), span) + context = SpanContextWrapper(span.get_context()) + return SpanWrapper(self, context, span) def inject(self, span_context, format, carrier): # pylint: disable=redefined-builtin diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 9440ca8db1..aa46ba8077 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -168,7 +168,7 @@ def test_parent_child_explicit_span_context(self): child_trace_id = child.otel_span.get_context().trace_id self.assertEqual(child_trace_id, parent_trace_id) - self.assertEqual(child.otel_span.parent, parent.context) + self.assertEqual(child.otel_span.parent, parent.context.otel_context) def test_explicit_span_activation(self): """Test manual activation and deactivation of a span.""" From a3cb8b0628c805e2be748f3262bb0cc201a5129d Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 17:21:32 +0200 Subject: [PATCH 052/103] Raise exception on invalid parent type According to the OpenTracing API, `child_of` can only be of type `Span` or `SpanContext`. In the shim these are represented as `SpanWrapper` and `SpanContextWrapper`, respectively. In case we receive a different type in `child_of`, it's safer to bail out than to try to set an unexpected object as the span's parent. --- opentracing-shim/src/opentracingshim/__init__.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 85d9ce1f70..67d3c61913 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -196,6 +196,10 @@ def start_span( parent = child_of.otel_span elif isinstance(parent, SpanContextWrapper): parent = child_of.otel_context + else: + raise RuntimeError( + "Invalid parent type when calling start_span()." + ) span = self._otel_tracer.create_span(operation_name, parent) span.start() From b726baf50613a336b6663c8d8edfb58609bebb4d Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 17:29:28 +0200 Subject: [PATCH 053/103] Use active span as default parent --- opentracing-shim/src/opentracingshim/__init__.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 67d3c61913..074cd56d7a 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -201,6 +201,14 @@ def start_span( "Invalid parent type when calling start_span()." ) + # Use active span as parent when no explicit parent is specified. + if ( + self.active_span is not None + and not ignore_active_span + and not parent + ): + parent = self.active_span.otel_span + span = self._otel_tracer.create_span(operation_name, parent) span.start() context = SpanContextWrapper(span.get_context()) From 74735d5fe574bd541ee2bcab82a6151a06b670ff Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 17:32:33 +0200 Subject: [PATCH 054/103] Add TODO --- opentracing-shim/tests/test_shim.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index aa46ba8077..15374a11b4 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -247,3 +247,5 @@ def test_explicit_scope_close(self): # Verify span has ended. self.assertIsNotNone(span.otel_span.end_time) + + # TODO: Test finish_on_close on `start_active_span()``. From cfa9ff8c248b5b932781c35b998e53d59daf97c0 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 17:56:38 +0200 Subject: [PATCH 055/103] Add TODOs --- opentracing-shim/src/opentracingshim/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 074cd56d7a..80e76a5fdd 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -189,7 +189,9 @@ def start_span( start_time=None, ignore_active_span=False, ) -> SpanWrapper: - # TODO: Handle optional arguments. + # TODO: Handle `references` argument. + # TODO: Handle `tags` argument. + # TODO: Handle `start_time` argument. parent = child_of if parent is not None: if isinstance(parent, SpanWrapper): From 1c68052408de0b8fdb376a361bca8d1e5c8d1221 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 19:27:32 +0200 Subject: [PATCH 056/103] Implement references --- opentracing-shim/src/opentracingshim/__init__.py | 8 +++++++- opentracing-shim/tests/test_shim.py | 11 +++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 80e76a5fdd..b6222384d6 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -189,7 +189,6 @@ def start_span( start_time=None, ignore_active_span=False, ) -> SpanWrapper: - # TODO: Handle `references` argument. # TODO: Handle `tags` argument. # TODO: Handle `start_time` argument. parent = child_of @@ -212,6 +211,13 @@ def start_span( parent = self.active_span.otel_span span = self._otel_tracer.create_span(operation_name, parent) + + if references: + if not isinstance(references, list): + references = [references] + for ref in references: + span.add_link(ref.referenced_context.otel_context) + span.start() context = SpanContextWrapper(span.get_context()) return SpanWrapper(self, context, span) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 15374a11b4..76a9949d78 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -170,6 +170,17 @@ def test_parent_child_explicit_span_context(self): self.assertEqual(child_trace_id, parent_trace_id) self.assertEqual(child.otel_span.parent, parent.context.otel_context) + def test_references(self): + """Test span creation using the `references` argument.""" + + parent = self.shim.start_span("ParentSpan") + ref = opentracing.child_of(parent.context) + child = self.shim.start_span("ChildSpan", references=[ref]) + + self.assertEqual( + child.otel_span.links[0].context, parent.context.otel_context + ) + def test_explicit_span_activation(self): """Test manual activation and deactivation of a span.""" From af038a2a479908ca0a03aa449b41c8e7512831e5 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 19:31:06 +0200 Subject: [PATCH 057/103] Add TODO --- opentracing-shim/src/opentracingshim/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index b6222384d6..ebfdae06e3 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -201,6 +201,8 @@ def start_span( raise RuntimeError( "Invalid parent type when calling start_span()." ) + # TODO: Should we use the OpenTracing base classes for the type + # check? # Use active span as parent when no explicit parent is specified. if ( From e57b5f04e6ae0461832bd5fc9a3c24d23f7cdaf3 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 19:46:35 +0200 Subject: [PATCH 058/103] Handle tags on span creation --- .../src/opentracingshim/__init__.py | 5 ++++- opentracing-shim/tests/test_shim.py | 17 +++++++++-------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index ebfdae06e3..756c8b7e2c 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -189,7 +189,6 @@ def start_span( start_time=None, ignore_active_span=False, ) -> SpanWrapper: - # TODO: Handle `tags` argument. # TODO: Handle `start_time` argument. parent = child_of if parent is not None: @@ -220,6 +219,10 @@ def start_span( for ref in references: span.add_link(ref.referenced_context.otel_context) + if tags: + for key, value in tags.items(): + span.set_attribute(key, value) + span.start() context = SpanContextWrapper(span.get_context()) return SpanWrapper(self, context, span) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 76a9949d78..2b30a56463 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -205,14 +205,15 @@ def test_set_operation_name(self): scope.span.set_operation_name("NewName") self.assertEqual(scope.span.otel_span.name, "NewName") - def test_set_tag(self): - with self.shim.start_active_span("TestSetTag") as scope: - with self.assertRaises(KeyError): - # pylint: disable=pointless-statement - scope.span.otel_span.attributes["my"] - - scope.span.set_tag("my", "tag") - self.assertEqual(scope.span.otel_span.attributes["my"], "tag") + def test_tags(self): + """Test tags behavior.""" + + tags = {"foo": "bar"} + with self.shim.start_active_span("TestSetTag", tags=tags) as scope: + scope.span.set_tag("baz", "qux") + + self.assertEqual(scope.span.otel_span.attributes["foo"], "bar") + self.assertEqual(scope.span.otel_span.attributes["baz"], "qux") def test_span(self): # Test tracer property. From 0d15ec605b59b6df32cb77028c15345f7ac87de0 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 22:34:16 +0200 Subject: [PATCH 059/103] Handle explicit span start time --- opentracing-shim/src/opentracingshim/__init__.py | 3 +-- opentracing-shim/tests/test_shim.py | 7 +++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 756c8b7e2c..32ed943f3d 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -189,7 +189,6 @@ def start_span( start_time=None, ignore_active_span=False, ) -> SpanWrapper: - # TODO: Handle `start_time` argument. parent = child_of if parent is not None: if isinstance(parent, SpanWrapper): @@ -223,7 +222,7 @@ def start_span( for key, value in tags.items(): span.set_attribute(key, value) - span.start() + span.start(start_time=start_time) context = SpanContextWrapper(span.get_context()) return SpanWrapper(self, context, span) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 2b30a56463..88fe4046a2 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -82,6 +82,13 @@ def test_start_span(self): # Verify `start_span()` does NOT make the span active. self.assertIsNone(self.shim.active_span) + def test_explicit_start_time(self): + """Test `start_time` argument.""" + + now = opentracingshim.util.time_ns() + with self.shim.start_active_span("TestSpan", start_time=now) as scope: + self.assertEqual(scope.span.otel_span.start_time, now) + def test_start_span_no_contextmanager(self): """Test `start_span()` without a `with` statement.""" From efa0f0c830fdf0aef1353c9d38048cc5993b325f Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 2 Oct 2019 22:34:32 +0200 Subject: [PATCH 060/103] Use time_ns() instead of time() --- opentracing-shim/tests/test_shim.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 88fe4046a2..21fc156a7a 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import time import unittest import opentracing @@ -241,7 +240,7 @@ def test_log_kv(self): self.assertIsNotNone(span.otel_span.events[0].timestamp) # Test explicit timestamp. - now = time.time() + now = opentracingshim.util.time_ns() span.log_kv({"foo": "bar"}, now) self.assertEqual(span.otel_span.events[1].timestamp, now) From f45ef7b1b3907b1d29af968ae19e17933342d620 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 4 Oct 2019 19:36:22 +0200 Subject: [PATCH 061/103] Overall refactor - Reorder tests. - Replace `otel_*` properties with `unwrap()` method. - Use correct type in `tracer` argument of `ScopeManagerWrapper`. --- .../src/opentracingshim/__init__.py | 32 ++- opentracing-shim/tests/test_shim.py | 240 +++++++++++------- 2 files changed, 173 insertions(+), 99 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 32ed943f3d..29ad3dd672 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -28,8 +28,9 @@ class SpanContextWrapper(opentracing.SpanContext): def __init__(self, otel_context: SpanContext): self._otel_context = otel_context - @property - def otel_context(self): + def unwrap(self): + """Returns the wrapped OpenTelemetry `SpanContext` object.""" + return self._otel_context @property @@ -43,9 +44,9 @@ def __init__(self, tracer, context, span: Span): self._otel_span = span opentracing.Span.__init__(self, tracer, context) - @property - def otel_span(self): - """Returns the OpenTelemetry span embedded in the SpanWrapper.""" + def unwrap(self): + """Returns the wrapped OpenTelemetry `Span` object.""" + return self._otel_span @property @@ -115,14 +116,14 @@ def close(self): class ScopeManagerWrapper(opentracing.ScopeManager): - def __init__(self, tracer: Tracer): + def __init__(self, tracer: "TracerWrapper"): # pylint: disable=super-init-not-called self._tracer = tracer @contextmanager def activate(self, span, finish_on_close): - with self._tracer.use_span( - span.otel_span, end_on_exit=finish_on_close + with self._tracer.unwrap().use_span( + span.unwrap(), end_on_exit=finish_on_close ) as otel_span: wrapped_span = SpanWrapper( self._tracer, otel_span.get_context(), otel_span @@ -145,7 +146,12 @@ def __init__( if scope_manager is not None: self._scope_manager = scope_manager else: - self._scope_manager = ScopeManagerWrapper(tracer) + self._scope_manager = ScopeManagerWrapper(self) + + def unwrap(self): + """Returns the wrapped OpenTelemetry `Tracer` object.""" + + return self._otel_tracer @property def scope_manager(self): @@ -192,9 +198,9 @@ def start_span( parent = child_of if parent is not None: if isinstance(parent, SpanWrapper): - parent = child_of.otel_span + parent = child_of.unwrap() elif isinstance(parent, SpanContextWrapper): - parent = child_of.otel_context + parent = child_of.unwrap() else: raise RuntimeError( "Invalid parent type when calling start_span()." @@ -208,7 +214,7 @@ def start_span( and not ignore_active_span and not parent ): - parent = self.active_span.otel_span + parent = self.active_span.unwrap() span = self._otel_tracer.create_span(operation_name, parent) @@ -216,7 +222,7 @@ def start_span( if not isinstance(references, list): references = [references] for ref in references: - span.add_link(ref.referenced_context.otel_context) + span.add_link(ref.referenced_context.unwrap()) if tags: for key, value in tags.items(): diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 21fc156a7a..11bb36efe3 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -40,8 +40,8 @@ def test_shim_type(self): # Verify shim is an OpenTracing tracer. self.assertIsInstance(self.shim, opentracing.Tracer) - def test_basic_behavior(self): - """Test span creation, activation and deactivation.""" + def test_start_active_span(self): + """Test span creation and activation using `start_active_span()`.""" with self.shim.start_active_span("TestSpan") as scope: # Verify correct type of Scope and Span objects. @@ -49,21 +49,21 @@ def test_basic_behavior(self): self.assertIsInstance(scope.span, opentracing.Span) # Verify the span is started. - self.assertIsNotNone(scope.span.otel_span.start_time) + self.assertIsNotNone(scope.span.unwrap().start_time) - # Verify the span is active in the OpenTelemetry tracer. + # Verify the span is active. + self.assertEqual(self.shim.active_span.context, scope.span.context) # TODO: We can't check for equality of self.shim.active_span and # scope.span because the same OpenTelemetry span is returned inside # different SpanWrapper objects. A possible solution is described # here: # https://github.com/open-telemetry/opentelemetry-python/issues/161#issuecomment-534136274 - self.assertEqual(self.shim.active_span.context, scope.span.context) - # Verify the span has ended in the OpenTelemetry tracer. - self.assertIsNotNone(scope.span.otel_span.end_time) + # Verify the span has ended. + self.assertIsNotNone(scope.span.unwrap().end_time) - # Verify no span is active on the OpenTelemetry tracer. - self.assertIsNone(self.tracer.get_current_span()) + # Verify no span is active. + self.assertIsNone(self.shim.active_span) def test_start_span(self): """Test span creation using `start_span()`.""" @@ -73,40 +73,101 @@ def test_start_span(self): self.assertIsInstance(span, opentracing.Span) # Verify the span is started. - self.assertIsNotNone(span.otel_span.start_time) + self.assertIsNotNone(span.unwrap().start_time) - # Verify the span has ended in the OpenTelemetry tracer. - self.assertIsNotNone(span.otel_span.end_time) + # Verify `start_span()` does NOT make the span active. + self.assertIsNone(self.shim.active_span) + + # Verify the span has ended. + self.assertIsNotNone(span.unwrap().end_time) + + def test_start_span_no_contextmanager(self): + """Test `start_span()` without a `with` statement.""" + + span = self.shim.start_span("TestSpan") + + # Verify the span is started. + self.assertIsNotNone(span.unwrap().start_time) # Verify `start_span()` does NOT make the span active. self.assertIsNone(self.shim.active_span) + span.finish() + + # Verify the span has ended. + self.assertIsNotNone(span.unwrap().end_time) + def test_explicit_start_time(self): """Test `start_time` argument.""" now = opentracingshim.util.time_ns() with self.shim.start_active_span("TestSpan", start_time=now) as scope: - self.assertEqual(scope.span.otel_span.start_time, now) + self.assertEqual(scope.span.unwrap().start_time, now) - def test_start_span_no_contextmanager(self): - """Test `start_span()` without a `with` statement.""" + def test_explicit_span_activation(self): + """Test manual activation and deactivation of a span.""" span = self.shim.start_span("TestSpan") - # Verify the span is started. - self.assertIsNotNone(span.otel_span.start_time) + # Verify no span is currently active. + self.assertIsNone(self.shim.active_span) - # Verify `start_span()` does NOT make the span active. + with self.shim.scope_manager.activate( + span, finish_on_close=True + ) as scope: + # Verify span is active. + self.assertEqual(self.shim.active_span.context, scope.span.context) + + # Verify no span is active. self.assertIsNone(self.shim.active_span) + def test_finish_on_close(self): + """Test `finish_on_close` argument.""" + + span = self.shim.start_span("TestSpan") + + with self.shim.scope_manager.activate( + span, finish_on_close=True + ) as scope: + # Verify span is active. + self.assertEqual(self.shim.active_span.context, scope.span.context) + + # Verify span has ended. + self.assertIsNotNone(span.unwrap().end_time) + + span = self.shim.start_span("TestSpan") + + with self.shim.scope_manager.activate( + span, finish_on_close=False + ) as scope: + # Verify span is active. + self.assertEqual(self.shim.active_span.context, scope.span.context) + + # Verify span hasn't ended. + self.assertIsNone(span.unwrap().end_time) + span.finish() - # Verify the span has ended in the OpenTelemetry tracer. - self.assertIsNotNone(span.otel_span.end_time) + # Verify span has ended. + self.assertIsNotNone(span.unwrap().end_time) + + def test_explicit_scope_close(self): + """Test `close()` method on `ScopeWrapper`.""" + + span = self.shim.start_span("TestSpan") + scope = opentracingshim.ScopeWrapper(self.shim.scope_manager, span) + + # Verify span hasn't ended. + self.assertIsNone(span.unwrap().end_time) + + scope.close() + + # Verify span has ended. + self.assertIsNotNone(span.unwrap().end_time) def test_parent_child_implicit(self): - """Test parent-child relationship of spans without specifying the - parent span upon creation, as well as span activation/deactivation. + """Test parent-child relationship and activation/deactivation of spans + without specifying the parent span upon creation. """ with self.shim.start_active_span("ParentSpan") as parent: @@ -122,12 +183,12 @@ def test_parent_child_implicit(self): ) # Verify parent-child relationship. - parent_trace_id = parent.span.otel_span.get_context().trace_id - child_trace_id = child.span.otel_span.get_context().trace_id + parent_trace_id = parent.span.unwrap().get_context().trace_id + child_trace_id = child.span.unwrap().get_context().trace_id self.assertEqual(parent_trace_id, child_trace_id) self.assertEqual( - child.span.otel_span.parent, parent.span.otel_span + child.span.unwrap().parent, parent.span.unwrap() ) # Verify parent span becomes the active span again. @@ -147,123 +208,130 @@ def test_parent_child_explicit_span(self): object as a parent upon creation. """ - # TODO: Test explicit parent also with `start_active_span()`. + parent = self.shim.start_span("ParentSpan") + with self.shim.start_active_span( + "ChildSpan", child_of=parent + ) as child: + parent_trace_id = parent.unwrap().get_context().trace_id + child_trace_id = child.span.unwrap().get_context().trace_id + + self.assertEqual(child_trace_id, parent_trace_id) + self.assertEqual(child.span.unwrap().parent, parent.unwrap()) + + parent.finish() parent = self.shim.start_span("ParentSpan") child = self.shim.start_span("ChildSpan", child_of=parent) - parent_trace_id = parent.otel_span.get_context().trace_id - child_trace_id = child.otel_span.get_context().trace_id + parent_trace_id = parent.unwrap().get_context().trace_id + child_trace_id = child.unwrap().get_context().trace_id self.assertEqual(child_trace_id, parent_trace_id) - self.assertEqual(child.otel_span.parent, parent.otel_span) + self.assertEqual(child.unwrap().parent, parent.unwrap()) + + child.finish() + parent.finish() def test_parent_child_explicit_span_context(self): """Test parent-child relationship of spans when specifying a `SpanContext` object as a parent upon creation. """ - # TODO: Test explicit parent also with `start_active_span()`. + parent = self.shim.start_span("ParentSpan") + with self.shim.start_active_span( + "ChildSpan", child_of=parent.context + ) as child: + parent_trace_id = parent.unwrap().get_context().trace_id + child_trace_id = child.span.unwrap().get_context().trace_id + + self.assertEqual(child_trace_id, parent_trace_id) + self.assertEqual( + child.span.unwrap().parent, parent.context.unwrap() + ) + + parent.finish() parent = self.shim.start_span("ParentSpan") child = self.shim.start_span( "SpanWithContextParent", child_of=parent.context ) - parent_trace_id = parent.otel_span.get_context().trace_id - child_trace_id = child.otel_span.get_context().trace_id + parent_trace_id = parent.unwrap().get_context().trace_id + child_trace_id = child.unwrap().get_context().trace_id self.assertEqual(child_trace_id, parent_trace_id) - self.assertEqual(child.otel_span.parent, parent.context.otel_context) + self.assertEqual(child.unwrap().parent, parent.context.unwrap()) + + child.finish() + parent.finish() def test_references(self): """Test span creation using the `references` argument.""" parent = self.shim.start_span("ParentSpan") ref = opentracing.child_of(parent.context) - child = self.shim.start_span("ChildSpan", references=[ref]) - - self.assertEqual( - child.otel_span.links[0].context, parent.context.otel_context - ) - - def test_explicit_span_activation(self): - """Test manual activation and deactivation of a span.""" - span = self.shim.start_span("TestSpan") - - # Verify no span is currently active. - self.assertIsNone(self.shim.active_span) - - with self.shim.scope_manager.activate( - span, finish_on_close=True - ) as scope: - # Verify span is active. - self.assertEqual(self.shim.active_span.context, scope.span.context) + with self.shim.start_active_span( + "ChildSpan", references=[ref] + ) as child: + self.assertEqual( + child.span.unwrap().links[0].context, parent.context.unwrap() + ) - # Verify no span is active. - self.assertIsNone(self.shim.active_span) + parent.finish() def test_set_operation_name(self): + """Test `set_operation_name()` method.""" + with self.shim.start_active_span("TestName") as scope: - self.assertEqual(scope.span.otel_span.name, "TestName") + self.assertEqual(scope.span.unwrap().name, "TestName") scope.span.set_operation_name("NewName") - self.assertEqual(scope.span.otel_span.name, "NewName") + self.assertEqual(scope.span.unwrap().name, "NewName") def test_tags(self): - """Test tags behavior.""" + """Test tags behavior using the `tags` argument and the `set_tags()` + method. + """ tags = {"foo": "bar"} with self.shim.start_active_span("TestSetTag", tags=tags) as scope: scope.span.set_tag("baz", "qux") - self.assertEqual(scope.span.otel_span.attributes["foo"], "bar") - self.assertEqual(scope.span.otel_span.attributes["baz"], "qux") + self.assertEqual(scope.span.unwrap().attributes["foo"], "bar") + self.assertEqual(scope.span.unwrap().attributes["baz"], "qux") - def test_span(self): - # Test tracer property. - span = self.shim.start_span("TestSpan") - self.assertEqual(span.tracer, self.shim) + def test_span_tracer(self): + """Test the `tracer` property on `Span` objects.""" - # Test finish() on span. - self.assertIsNone(span.otel_span.end_time) - span.finish() - self.assertIsNotNone(span.otel_span.end_time) + with self.shim.start_active_span("TestSpan") as scope: + self.assertEqual(scope.span.tracer, self.shim) def test_log_kv(self): + """Test the `log_kv()` method on `Span` objects.""" + span = self.shim.start_span("TestSpan") span.log_kv({"foo": "bar"}) - self.assertEqual(span.otel_span.events[0].attributes["foo"], "bar") + self.assertEqual(span.unwrap().events[0].attributes["foo"], "bar") # Verify timestamp was generated automatically. - self.assertIsNotNone(span.otel_span.events[0].timestamp) + self.assertIsNotNone(span.unwrap().events[0].timestamp) # Test explicit timestamp. now = opentracingshim.util.time_ns() span.log_kv({"foo": "bar"}, now) - self.assertEqual(span.otel_span.events[1].timestamp, now) + self.assertEqual(span.unwrap().events[1].timestamp, now) + + span.finish() def test_span_context(self): + """Test construction of `SpanContextWrapper` objects.""" + otel_context = trace.SpanContext(1234, 5678) context = opentracingshim.SpanContextWrapper(otel_context) self.assertIsInstance(context, opentracing.SpanContext) - self.assertEqual(context.otel_context.trace_id, 1234) - self.assertEqual(context.otel_context.span_id, 5678) - - def test_explicit_scope_close(self): - """Test `close()` method on `ScopeWrapper`.""" - - span = self.shim.start_span("TestSpan") - scope = opentracingshim.ScopeWrapper(self.shim.scope_manager, span) - - # Verify span hasn't ended. - self.assertIsNone(span.otel_span.end_time) - - scope.close() - - # Verify span has ended. - self.assertIsNotNone(span.otel_span.end_time) + self.assertEqual(context.unwrap().trace_id, 1234) + self.assertEqual(context.unwrap().span_id, 5678) - # TODO: Test finish_on_close on `start_active_span()``. + # TODO: Test `finish_on_close` on `start_active_span()``. From a4cce55d5c7489417f943d8a285b71fcd3b828c0 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 4 Oct 2019 19:52:28 +0200 Subject: [PATCH 062/103] Test explicit span finish --- opentracing-shim/tests/test_shim.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 11bb36efe3..111d3a1092 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -94,6 +94,16 @@ def test_start_span_no_contextmanager(self): span.finish() + def test_explicit_span_finish(self): + """Test `finish()` method on `Span` objects.""" + + span = self.shim.start_span("TestSpan") + + # Verify span hasn't ended. + self.assertIsNone(span.unwrap().end_time) + + span.finish() + # Verify the span has ended. self.assertIsNotNone(span.unwrap().end_time) @@ -148,9 +158,6 @@ def test_finish_on_close(self): span.finish() - # Verify span has ended. - self.assertIsNotNone(span.unwrap().end_time) - def test_explicit_scope_close(self): """Test `close()` method on `ScopeWrapper`.""" From 1901cec46a9a083bef6e3829ecc85d954b2e0cbe Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 4 Oct 2019 19:52:46 +0200 Subject: [PATCH 063/103] Refactor finish_on_close tests --- opentracing-shim/tests/test_shim.py | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 111d3a1092..90da39566f 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -131,8 +131,31 @@ def test_explicit_span_activation(self): # Verify no span is active. self.assertIsNone(self.shim.active_span) - def test_finish_on_close(self): - """Test `finish_on_close` argument.""" + def test_start_active_span_finish_on_close(self): + """Test `finish_on_close` argument of `start_active_span()`.""" + + with self.shim.start_active_span( + "TestSpan", finish_on_close=True + ) as scope: + # Verify span hasn't ended. + self.assertIsNone(scope.span.unwrap().end_time) + + # Verify span has ended. + self.assertIsNotNone(scope.span.unwrap().end_time) + + with self.shim.start_active_span( + "TestSpan", finish_on_close=False + ) as scope: + # Verify span hasn't ended. + self.assertIsNone(scope.span.unwrap().end_time) + + # Verify span hasn't ended after scope had been closed. + self.assertIsNone(scope.span.unwrap().end_time) + + scope.span.finish() + + def test_activate_finish_on_close(self): + """Test `finish_on_close` argument of `activate()`.""" span = self.shim.start_span("TestSpan") @@ -340,5 +363,3 @@ def test_span_context(self): self.assertIsInstance(context, opentracing.SpanContext) self.assertEqual(context.unwrap().trace_id, 1234) self.assertEqual(context.unwrap().span_id, 5678) - - # TODO: Test `finish_on_close` on `start_active_span()``. From 1f8fd20b60c30403df4b4e6811696711e0d3b67c Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 4 Oct 2019 20:11:24 +0200 Subject: [PATCH 064/103] Use context manager where possible Use a `with` statement to finish spans automatically where possible, i.e. when not testing things which require explicit calling to `span.finish()`. --- opentracing-shim/tests/test_shim.py | 119 +++++++++++++--------------- 1 file changed, 54 insertions(+), 65 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 90da39566f..18587ed609 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -238,77 +238,69 @@ def test_parent_child_explicit_span(self): object as a parent upon creation. """ - parent = self.shim.start_span("ParentSpan") - with self.shim.start_active_span( - "ChildSpan", child_of=parent - ) as child: - parent_trace_id = parent.unwrap().get_context().trace_id - child_trace_id = child.span.unwrap().get_context().trace_id - - self.assertEqual(child_trace_id, parent_trace_id) - self.assertEqual(child.span.unwrap().parent, parent.unwrap()) + with self.shim.start_span("ParentSpan") as parent: + with self.shim.start_active_span( + "ChildSpan", child_of=parent + ) as child: + parent_trace_id = parent.unwrap().get_context().trace_id + child_trace_id = child.span.unwrap().get_context().trace_id - parent.finish() + self.assertEqual(child_trace_id, parent_trace_id) + self.assertEqual(child.span.unwrap().parent, parent.unwrap()) - parent = self.shim.start_span("ParentSpan") - child = self.shim.start_span("ChildSpan", child_of=parent) + with self.shim.start_span("ParentSpan") as parent: + child = self.shim.start_span("ChildSpan", child_of=parent) - parent_trace_id = parent.unwrap().get_context().trace_id - child_trace_id = child.unwrap().get_context().trace_id + parent_trace_id = parent.unwrap().get_context().trace_id + child_trace_id = child.unwrap().get_context().trace_id - self.assertEqual(child_trace_id, parent_trace_id) - self.assertEqual(child.unwrap().parent, parent.unwrap()) + self.assertEqual(child_trace_id, parent_trace_id) + self.assertEqual(child.unwrap().parent, parent.unwrap()) - child.finish() - parent.finish() + child.finish() def test_parent_child_explicit_span_context(self): """Test parent-child relationship of spans when specifying a `SpanContext` object as a parent upon creation. """ - parent = self.shim.start_span("ParentSpan") - with self.shim.start_active_span( - "ChildSpan", child_of=parent.context - ) as child: - parent_trace_id = parent.unwrap().get_context().trace_id - child_trace_id = child.span.unwrap().get_context().trace_id - - self.assertEqual(child_trace_id, parent_trace_id) - self.assertEqual( - child.span.unwrap().parent, parent.context.unwrap() - ) - - parent.finish() - - parent = self.shim.start_span("ParentSpan") - child = self.shim.start_span( - "SpanWithContextParent", child_of=parent.context - ) + with self.shim.start_span("ParentSpan") as parent: + with self.shim.start_active_span( + "ChildSpan", child_of=parent.context + ) as child: + parent_trace_id = parent.unwrap().get_context().trace_id + child_trace_id = child.span.unwrap().get_context().trace_id - parent_trace_id = parent.unwrap().get_context().trace_id - child_trace_id = child.unwrap().get_context().trace_id + self.assertEqual(child_trace_id, parent_trace_id) + self.assertEqual( + child.span.unwrap().parent, parent.context.unwrap() + ) - self.assertEqual(child_trace_id, parent_trace_id) - self.assertEqual(child.unwrap().parent, parent.context.unwrap()) + with self.shim.start_span("ParentSpan") as parent: + with self.shim.start_span( + "SpanWithContextParent", child_of=parent.context + ) as child: + parent_trace_id = parent.unwrap().get_context().trace_id + child_trace_id = child.unwrap().get_context().trace_id - child.finish() - parent.finish() + self.assertEqual(child_trace_id, parent_trace_id) + self.assertEqual( + child.unwrap().parent, parent.context.unwrap() + ) def test_references(self): """Test span creation using the `references` argument.""" - parent = self.shim.start_span("ParentSpan") - ref = opentracing.child_of(parent.context) - - with self.shim.start_active_span( - "ChildSpan", references=[ref] - ) as child: - self.assertEqual( - child.span.unwrap().links[0].context, parent.context.unwrap() - ) + with self.shim.start_span("ParentSpan") as parent: + ref = opentracing.child_of(parent.context) - parent.finish() + with self.shim.start_active_span( + "ChildSpan", references=[ref] + ) as child: + self.assertEqual( + child.span.unwrap().links[0].context, + parent.context.unwrap(), + ) def test_set_operation_name(self): """Test `set_operation_name()` method.""" @@ -340,19 +332,16 @@ def test_span_tracer(self): def test_log_kv(self): """Test the `log_kv()` method on `Span` objects.""" - span = self.shim.start_span("TestSpan") - - span.log_kv({"foo": "bar"}) - self.assertEqual(span.unwrap().events[0].attributes["foo"], "bar") - # Verify timestamp was generated automatically. - self.assertIsNotNone(span.unwrap().events[0].timestamp) - - # Test explicit timestamp. - now = opentracingshim.util.time_ns() - span.log_kv({"foo": "bar"}, now) - self.assertEqual(span.unwrap().events[1].timestamp, now) - - span.finish() + with self.shim.start_span("TestSpan") as span: + span.log_kv({"foo": "bar"}) + self.assertEqual(span.unwrap().events[0].attributes["foo"], "bar") + # Verify timestamp was generated automatically. + self.assertIsNotNone(span.unwrap().events[0].timestamp) + + # Test explicit timestamp. + now = opentracingshim.util.time_ns() + span.log_kv({"foo": "bar"}, now) + self.assertEqual(span.unwrap().events[1].timestamp, now) def test_span_context(self): """Test construction of `SpanContextWrapper` objects.""" From 45ed0b9f5a0cf203bc196178557b3c206e97d63f Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 4 Oct 2019 20:32:44 +0200 Subject: [PATCH 065/103] Disable too-many-public-methods Pylint check We have many methods because we have many tests. --- opentracing-shim/tests/test_shim.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 18587ed609..3f959c3dce 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -22,6 +22,8 @@ class TestShim(unittest.TestCase): + # pylint: disable=too-many-public-methods + def setUp(self): self.tracer = trace.tracer() self.shim = opentracingshim.create_tracer(self.tracer) From c32c27815a275d97c86408eddb6e0a3ea86adef3 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 4 Oct 2019 22:39:14 +0200 Subject: [PATCH 066/103] Update README --- opentracing-shim/README.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/opentracing-shim/README.rst b/opentracing-shim/README.rst index 12852fd752..71603778ce 100644 --- a/opentracing-shim/README.rst +++ b/opentracing-shim/README.rst @@ -1,10 +1,10 @@ -OpenTelemetry Python API +OpenTracing Shim for OpenTelemetry ============================================================================ |pypi| -.. |pypi| image:: https://badge.fury.io/py/opentelemetry-api.svg - :target: https://pypi.org/project/opentelemetry-api/ +.. |pypi| image:: https://badge.fury.io/py/opentelemetry-ot-shim.svg + :target: https://pypi.org/project/opentelemetry-ot-shim/ Installation ------------ From 92b760f7c4ad632b54ea8de52222d100d7303ec4 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 7 Oct 2019 15:07:25 +0200 Subject: [PATCH 067/103] Add missing dependencies to setup.py --- opentracing-shim/setup.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/opentracing-shim/setup.py b/opentracing-shim/setup.py index c8fa935655..483e0a2943 100644 --- a/opentracing-shim/setup.py +++ b/opentracing-shim/setup.py @@ -43,7 +43,11 @@ description="OpenTracing shim for OpenTelemetry", include_package_data=True, long_description=open("README.rst").read(), - install_requires=["typing; python_version<'3.5'"], + install_requires=[ + "typing; python_version<'3.5'", + "opentracing", + "opentelemetry-api", + ], extras_require={}, license="Apache-2.0", package_dir={"": "src"}, From b6abe128a35da03e96d94cb82ca44d44dd31adca Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 7 Oct 2019 16:53:12 +0200 Subject: [PATCH 068/103] Handle time conversion between OT and OTel Handle time format differences between OpenTracing and OpenTelemetry. OpenTracing expects time values in seconds since the epoch represented as floats. OpenTelemetry expects time values in nanoseconds since the epoch represented as ints. --- .../src/opentracingshim/__init__.py | 9 ++++++- opentracing-shim/src/opentracingshim/util.py | 26 +++++++++++++++++++ opentracing-shim/tests/test_shim.py | 7 +++-- opentracing-shim/tests/test_util.py | 13 ++++++++++ 4 files changed, 52 insertions(+), 3 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 29ad3dd672..3a18163196 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -228,7 +228,14 @@ def start_span( for key, value in tags.items(): span.set_attribute(key, value) - span.start(start_time=start_time) + # The OpenTracing API expects time values to be `float` values which + # represent the number of seconds since the epoch. OpenTelemetry + # represents time values as nanoseconds since the epoch. + start_time_ns = start_time + if start_time_ns is not None: + start_time_ns = util.time_seconds_to_ns(start_time) + + span.start(start_time=start_time_ns) context = SpanContextWrapper(span.get_context()) return SpanWrapper(self, context, span) diff --git a/opentracing-shim/src/opentracingshim/util.py b/opentracing-shim/src/opentracingshim/util.py index 188966cedd..7640ae0535 100644 --- a/opentracing-shim/src/opentracingshim/util.py +++ b/opentracing-shim/src/opentracingshim/util.py @@ -28,6 +28,32 @@ def time_ns(): DEFAULT_EVENT_NAME = "log" +def time_seconds_to_ns(time_seconds: float) -> int: + """Converts a time value in seconds to a time value in nanoseconds. + + `time_seconds` is a `float` as returned by `time.time()` which represents + the number of seconds since the epoch. + + The returned value is an `int` representing the number of nanoseconds since + the epoch. + """ + + return int(time_seconds * 1e9) + + +def time_seconds_from_ns(time_nanoseconds: int) -> float: + """Converts a time value in nanoseconds to a time value in seconds. + + `time_nanoseconds` is an `int` representing the number of nanoseconds since + the epoch. + + The returned value is a `float` representing the number of seconds since + the epoch. + """ + + return time_nanoseconds / 1e9 + + def event_name_from_kv(key_values: dict) -> str: """A helper function which returns an event name from the given dict, or a default event name. diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 3f959c3dce..0b0d4dd3ac 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import time import unittest import opentracing @@ -19,6 +20,7 @@ import opentracingshim from opentelemetry import trace from opentelemetry.sdk.trace import Tracer +from opentracingshim import util class TestShim(unittest.TestCase): @@ -112,9 +114,10 @@ def test_explicit_span_finish(self): def test_explicit_start_time(self): """Test `start_time` argument.""" - now = opentracingshim.util.time_ns() + now = time.time() with self.shim.start_active_span("TestSpan", start_time=now) as scope: - self.assertEqual(scope.span.unwrap().start_time, now) + result = util.time_seconds_from_ns(scope.span.unwrap().start_time) + self.assertEqual(result, now) def test_explicit_span_activation(self): """Test manual activation and deactivation of a span.""" diff --git a/opentracing-shim/tests/test_util.py b/opentracing-shim/tests/test_util.py index 3b2dabcc89..99e0b61ddc 100644 --- a/opentracing-shim/tests/test_util.py +++ b/opentracing-shim/tests/test_util.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import time import unittest from opentracingshim import util @@ -35,3 +36,15 @@ def test_event_name_from_kv(self): # Test missing `event` field. res = util.event_name_from_kv({"foo": "bar"}) self.assertEqual(res, util.DEFAULT_EVENT_NAME) + + def test_time_seconds_to_ns(self): + time_seconds = time.time() + result = util.time_seconds_to_ns(time_seconds) + + self.assertEqual(result, int(time_seconds * 1e9)) + + def test_time_seconds_from_ns(self): + time_nanoseconds = util.time_ns() + result = util.time_seconds_from_ns(time_nanoseconds) + + self.assertEqual(result, time_nanoseconds / 1e9) From fca38adea3db3a2eb289cafdd8a059ee6f744b66 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 7 Oct 2019 16:53:55 +0200 Subject: [PATCH 069/103] Handle explicit end time when ending spans --- opentracing-shim/src/opentracingshim/__init__.py | 11 +++++------ opentracing-shim/tests/test_shim.py | 10 ++++++++++ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 3a18163196..390e5a0087 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -61,12 +61,11 @@ def set_operation_name(self, operation_name): self._otel_span.update_name(operation_name) return self - def finish(self, finish_time=None): - self._otel_span.end() - # TODO: Handle finish_time. The OpenTelemetry API doesn't currently - # support setting end time on a span and we cannot assume that all - # OpenTelemetry Tracer implementations have an `end_time` field. - # https://github.com/open-telemetry/opentelemetry-python/issues/134 + def finish(self, finish_time: float = None): + end_time = finish_time + if end_time is not None: + end_time = util.time_seconds_to_ns(finish_time) + self._otel_span.end(end_time=end_time) def set_tag(self, key, value): self._otel_span.set_attribute(key, value) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 0b0d4dd3ac..1613674a80 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -119,6 +119,16 @@ def test_explicit_start_time(self): result = util.time_seconds_from_ns(scope.span.unwrap().start_time) self.assertEqual(result, now) + def test_explicit_end_time(self): + """Test `end_time` argument of `finish()` method.""" + + span = self.shim.start_span("TestSpan") + now = time.time() + span.finish(now) + + end_time = util.time_seconds_from_ns(span.unwrap().end_time) + self.assertEqual(end_time, now) + def test_explicit_span_activation(self): """Test manual activation and deactivation of a span.""" From e04c0734b225a297e2a142673ff0848e36bc4d36 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 7 Oct 2019 18:04:24 +0200 Subject: [PATCH 070/103] Return self for call chaining on SpanWrapper Return `self` from all "setter" methods on `SpanWrapper` to allow call chaining. --- opentracing-shim/src/opentracingshim/__init__.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 390e5a0087..bc4b337d51 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -59,6 +59,8 @@ def tracer(self): def set_operation_name(self, operation_name): self._otel_span.update_name(operation_name) + + # Return self for call chaining. return self def finish(self, finish_time: float = None): @@ -70,6 +72,9 @@ def finish(self, finish_time: float = None): def set_tag(self, key, value): self._otel_span.set_attribute(key, value) + # Return self for call chaining. + return self + def log_kv(self, key_values, timestamp=None): if timestamp is None: timestamp = util.time_ns() @@ -82,6 +87,7 @@ def log_kv(self, key_values, timestamp=None): return self def set_baggage_item(self, key, value): + # Return self for call chaining. return self # TODO: Implement. From ca8146fc46ded5608878ddf8be65991df34fac35 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 7 Oct 2019 20:18:44 +0200 Subject: [PATCH 071/103] Remove type hints It seems we cannot consistently use Python type hints in the OpenTracing shim since we rely on the `opentracing` library which doesn't contain type hints, nor does it have PEP 561 compliant stub packages or a module on typeshed. Rather than including partial typing information it's better not to include typing information at all. --- .../src/opentracingshim/__init__.py | 20 +++++++++---------- opentracing-shim/src/opentracingshim/util.py | 6 +++--- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index bc4b337d51..e46ae9dd23 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -16,16 +16,16 @@ import opentracing -from opentelemetry.trace import Event, Span, SpanContext, Tracer +from opentelemetry.trace import Event from opentracingshim import util -def create_tracer(tracer: Tracer) -> opentracing.Tracer: +def create_tracer(tracer): return TracerWrapper(tracer) class SpanContextWrapper(opentracing.SpanContext): - def __init__(self, otel_context: SpanContext): + def __init__(self, otel_context): self._otel_context = otel_context def unwrap(self): @@ -40,7 +40,7 @@ def baggage(self): class SpanWrapper(opentracing.Span): - def __init__(self, tracer, context, span: Span): + def __init__(self, tracer, context, span): self._otel_span = span opentracing.Span.__init__(self, tracer, context) @@ -63,7 +63,7 @@ def set_operation_name(self, operation_name): # Return self for call chaining. return self - def finish(self, finish_time: float = None): + def finish(self, finish_time=None): end_time = finish_time if end_time is not None: end_time = util.time_seconds_to_ns(finish_time) @@ -121,7 +121,7 @@ def close(self): class ScopeManagerWrapper(opentracing.ScopeManager): - def __init__(self, tracer: "TracerWrapper"): + def __init__(self, tracer): # pylint: disable=super-init-not-called self._tracer = tracer @@ -143,9 +143,7 @@ def active(self): class TracerWrapper(opentracing.Tracer): - def __init__( - self, tracer: Tracer, scope_manager: ScopeManagerWrapper = None - ): + def __init__(self, tracer, scope_manager=None): # pylint: disable=super-init-not-called self._otel_tracer = tracer if scope_manager is not None: @@ -179,7 +177,7 @@ def start_active_span( start_time=None, ignore_active_span=False, finish_on_close=True, - ) -> ScopeWrapper: + ): span = self.start_span( operation_name=operation_name, child_of=child_of, @@ -199,7 +197,7 @@ def start_span( tags=None, start_time=None, ignore_active_span=False, - ) -> SpanWrapper: + ): parent = child_of if parent is not None: if isinstance(parent, SpanWrapper): diff --git a/opentracing-shim/src/opentracingshim/util.py b/opentracing-shim/src/opentracingshim/util.py index 7640ae0535..a4e5142ffc 100644 --- a/opentracing-shim/src/opentracingshim/util.py +++ b/opentracing-shim/src/opentracingshim/util.py @@ -28,7 +28,7 @@ def time_ns(): DEFAULT_EVENT_NAME = "log" -def time_seconds_to_ns(time_seconds: float) -> int: +def time_seconds_to_ns(time_seconds): """Converts a time value in seconds to a time value in nanoseconds. `time_seconds` is a `float` as returned by `time.time()` which represents @@ -41,7 +41,7 @@ def time_seconds_to_ns(time_seconds: float) -> int: return int(time_seconds * 1e9) -def time_seconds_from_ns(time_nanoseconds: int) -> float: +def time_seconds_from_ns(time_nanoseconds): """Converts a time value in nanoseconds to a time value in seconds. `time_nanoseconds` is an `int` representing the number of nanoseconds since @@ -54,7 +54,7 @@ def time_seconds_from_ns(time_nanoseconds: int) -> float: return time_nanoseconds / 1e9 -def event_name_from_kv(key_values: dict) -> str: +def event_name_from_kv(key_values): """A helper function which returns an event name from the given dict, or a default event name. """ From a263f1899902a46bcb17bf9ecbcd04779d3ea25c Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 7 Oct 2019 20:55:53 +0200 Subject: [PATCH 072/103] Add TODO --- opentracing-shim/tests/test_shim.py | 1 + 1 file changed, 1 insertion(+) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 1613674a80..634d45abd9 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -27,6 +27,7 @@ class TestShim(unittest.TestCase): # pylint: disable=too-many-public-methods def setUp(self): + # TODO: Review setup. self.tracer = trace.tracer() self.shim = opentracingshim.create_tracer(self.tracer) From b1f362fdb6b7494b2b2ac2510b4d93c1b20e6c4c Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 7 Oct 2019 21:14:46 +0200 Subject: [PATCH 073/103] Raise NotImplementedError in unimplemented methods --- opentracing-shim/src/opentracingshim/__init__.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index e46ae9dd23..d070be19b4 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -35,7 +35,7 @@ def unwrap(self): @property def baggage(self): - return {} + raise NotImplementedError # TODO: Implement. @@ -87,12 +87,11 @@ def log_kv(self, key_values, timestamp=None): return self def set_baggage_item(self, key, value): - # Return self for call chaining. - return self + raise NotImplementedError # TODO: Implement. def get_baggage_item(self, key): - return None + raise NotImplementedError # TODO: Implement. # TODO: Verify calls to deprecated methods `log_event()` and `log()` on @@ -244,10 +243,10 @@ def start_span( def inject(self, span_context, format, carrier): # pylint: disable=redefined-builtin - pass + raise NotImplementedError # TODO: Implement. def extract(self, format, carrier): # pylint: disable=redefined-builtin - pass + raise NotImplementedError # TODO: Implement. From fe773801f1e6c2212c4f12d40991323320b46548 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 7 Oct 2019 21:21:52 +0200 Subject: [PATCH 074/103] Use super() instead of an explicit class name --- opentracing-shim/src/opentracingshim/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index d070be19b4..5e5e4e8b9e 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -42,7 +42,7 @@ def baggage(self): class SpanWrapper(opentracing.Span): def __init__(self, tracer, context, span): self._otel_span = span - opentracing.Span.__init__(self, tracer, context) + super().__init__(tracer, context) def unwrap(self): """Returns the wrapped OpenTelemetry `Span` object.""" From 0252f6ca2426c2245a3f0fb60df4798524e5ada5 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 7 Oct 2019 22:01:12 +0200 Subject: [PATCH 075/103] Clean up OpenTracing inheritance In some cases we had duplicated logic from the base class' `__init__` method. In others we had overridden methods with the same logic as the equivalent methods in the base class. We clean up the above and add comments to help understand some of the inheritance-related choices. --- .../src/opentracingshim/__init__.py | 45 ++++++------------- 1 file changed, 13 insertions(+), 32 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index 5e5e4e8b9e..a32a833c6e 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -20,8 +20,8 @@ from opentracingshim import util -def create_tracer(tracer): - return TracerWrapper(tracer) +def create_tracer(tracer, scope_manager=None): + return TracerWrapper(tracer, scope_manager) class SpanContextWrapper(opentracing.SpanContext): @@ -49,14 +49,6 @@ def unwrap(self): return self._otel_span - @property - def context(self): - return self._context - - @property - def tracer(self): - return self._tracer - def set_operation_name(self, operation_name): self._otel_span.update_name(operation_name) @@ -100,19 +92,6 @@ def get_baggage_item(self, key): class ScopeWrapper(opentracing.Scope): - def __init__(self, manager, span): - # pylint: disable=super-init-not-called - self._manager = manager - self._span = span - - @property - def span(self): - return self._span - - @property - def manager(self): - return self._manager - def close(self): self._span.finish() # TODO: Set active span on OpenTelemetry tracer. @@ -121,6 +100,9 @@ def close(self): class ScopeManagerWrapper(opentracing.ScopeManager): def __init__(self, tracer): + # The only thing the `__init__()` method on the base class does is + # initialize `self._noop_span` and `self._noop_scope` with no-op + # objects. Therefore, it doesn't seem useful to call it. # pylint: disable=super-init-not-called self._tracer = tracer @@ -143,22 +125,21 @@ def active(self): class TracerWrapper(opentracing.Tracer): def __init__(self, tracer, scope_manager=None): - # pylint: disable=super-init-not-called self._otel_tracer = tracer - if scope_manager is not None: - self._scope_manager = scope_manager - else: - self._scope_manager = ScopeManagerWrapper(self) + + # If a scope manager isn't provided by the user, create a + # `ScopeManagerWrapper` instance and use it to initialize the + # `TracerWrapper`. + if scope_manager is None: + scope_manager = ScopeManagerWrapper(self) + + super().__init__(scope_manager=scope_manager) def unwrap(self): """Returns the wrapped OpenTelemetry `Tracer` object.""" return self._otel_tracer - @property - def scope_manager(self): - return self._scope_manager - @property def active_span(self): span = self._otel_tracer.get_current_span() From 442dd5c37cdfa49436718baa3e4c37457a8e741c Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 7 Oct 2019 22:19:03 +0200 Subject: [PATCH 076/103] Make comments in tests consistent --- opentracing-shim/tests/test_shim.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 634d45abd9..87e5b4b78c 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -53,10 +53,10 @@ def test_start_active_span(self): self.assertIsInstance(scope, opentracing.Scope) self.assertIsInstance(scope.span, opentracing.Span) - # Verify the span is started. + # Verify span is started. self.assertIsNotNone(scope.span.unwrap().start_time) - # Verify the span is active. + # Verify span is active. self.assertEqual(self.shim.active_span.context, scope.span.context) # TODO: We can't check for equality of self.shim.active_span and # scope.span because the same OpenTelemetry span is returned inside @@ -64,7 +64,7 @@ def test_start_active_span(self): # here: # https://github.com/open-telemetry/opentelemetry-python/issues/161#issuecomment-534136274 - # Verify the span has ended. + # Verify span has ended. self.assertIsNotNone(scope.span.unwrap().end_time) # Verify no span is active. @@ -77,13 +77,13 @@ def test_start_span(self): # Verify correct type of Span object. self.assertIsInstance(span, opentracing.Span) - # Verify the span is started. + # Verify span is started. self.assertIsNotNone(span.unwrap().start_time) # Verify `start_span()` does NOT make the span active. self.assertIsNone(self.shim.active_span) - # Verify the span has ended. + # Verify span has ended. self.assertIsNotNone(span.unwrap().end_time) def test_start_span_no_contextmanager(self): @@ -91,7 +91,7 @@ def test_start_span_no_contextmanager(self): span = self.shim.start_span("TestSpan") - # Verify the span is started. + # Verify span is started. self.assertIsNotNone(span.unwrap().start_time) # Verify `start_span()` does NOT make the span active. @@ -109,7 +109,7 @@ def test_explicit_span_finish(self): span.finish() - # Verify the span has ended. + # Verify span has ended. self.assertIsNotNone(span.unwrap().end_time) def test_explicit_start_time(self): From 8fd39efd57ae16dcce86001a8101c035c7cf8cfd Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 9 Oct 2019 18:29:01 +0200 Subject: [PATCH 077/103] Handle inaccuracies in time conversions Due to the differences between decimal and binary float representations, some time values yield "surprising" results when performing float arithmetic operations to convert seconds to nanoseconds and vice versa. --- .../src/opentracingshim/__init__.py | 8 ++++--- opentracing-shim/tests/test_shim.py | 24 +++++++++++++++---- opentracing-shim/tests/test_util.py | 15 ++++++++++++ 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/opentracing-shim/src/opentracingshim/__init__.py index a32a833c6e..e7bdb35610 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/opentracing-shim/src/opentracingshim/__init__.py @@ -68,11 +68,13 @@ def set_tag(self, key, value): return self def log_kv(self, key_values, timestamp=None): - if timestamp is None: - timestamp = util.time_ns() + if timestamp is not None: + event_timestamp = util.time_seconds_to_ns(timestamp) + else: + event_timestamp = util.time_ns() event_name = util.event_name_from_kv(key_values) - event = Event(event_name, timestamp, key_values) + event = Event(event_name, event_timestamp, key_values) self._otel_span.add_lazy_event(event) # Return self for call chaining. diff --git a/opentracing-shim/tests/test_shim.py b/opentracing-shim/tests/test_shim.py index 87e5b4b78c..d1bcdb79d6 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/opentracing-shim/tests/test_shim.py @@ -118,7 +118,11 @@ def test_explicit_start_time(self): now = time.time() with self.shim.start_active_span("TestSpan", start_time=now) as scope: result = util.time_seconds_from_ns(scope.span.unwrap().start_time) - self.assertEqual(result, now) + # Tolerate inaccuracies of less than a microsecond. + # TODO: Put a link to an explanation in the docs. + # TODO: This seems to work consistently, but we should find out the + # biggest possible loss of precision. + self.assertAlmostEqual(result, now, places=6) def test_explicit_end_time(self): """Test `end_time` argument of `finish()` method.""" @@ -128,7 +132,11 @@ def test_explicit_end_time(self): span.finish(now) end_time = util.time_seconds_from_ns(span.unwrap().end_time) - self.assertEqual(end_time, now) + # Tolerate inaccuracies of less than a microsecond. + # TODO: Put a link to an explanation in the docs. + # TODO: This seems to work consistently, but we should find out the + # biggest possible loss of precision. + self.assertAlmostEqual(end_time, now, places=6) def test_explicit_span_activation(self): """Test manual activation and deactivation of a span.""" @@ -355,9 +363,17 @@ def test_log_kv(self): self.assertIsNotNone(span.unwrap().events[0].timestamp) # Test explicit timestamp. - now = opentracingshim.util.time_ns() + now = time.time() span.log_kv({"foo": "bar"}, now) - self.assertEqual(span.unwrap().events[1].timestamp, now) + result = util.time_seconds_from_ns( + span.unwrap().events[1].timestamp + ) + self.assertEqual(span.unwrap().events[1].attributes["foo"], "bar") + # Tolerate inaccuracies of less than a microsecond. + # TODO: Put a link to an explanation in the docs. + # TODO: This seems to work consistently, but we should find out the + # biggest possible loss of precision. + self.assertAlmostEqual(result, now, places=6) def test_span_context(self): """Test construction of `SpanContextWrapper` objects.""" diff --git a/opentracing-shim/tests/test_util.py b/opentracing-shim/tests/test_util.py index 99e0b61ddc..869164eef1 100644 --- a/opentracing-shim/tests/test_util.py +++ b/opentracing-shim/tests/test_util.py @@ -48,3 +48,18 @@ def test_time_seconds_from_ns(self): result = util.time_seconds_from_ns(time_nanoseconds) self.assertEqual(result, time_nanoseconds / 1e9) + + def test_time_conversion_precision(self): + """Verify time conversion from seconds to nanoseconds and vice versa is + accurate enough. + """ + + time_seconds = 1570484241.9501917 + time_nanoseconds = util.time_seconds_to_ns(time_seconds) + result = util.time_seconds_from_ns(time_nanoseconds) + + # Tolerate inaccuracies of less than a microsecond. + # TODO: Put a link to an explanation in the docs. + # TODO: This seems to work consistently, but we should find out the + # biggest possible loss of precision. + self.assertAlmostEqual(result, time_seconds, places=6) From 9453625555bc786c15c4777947b4a6448cf7b34c Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 10 Oct 2019 13:45:51 +0200 Subject: [PATCH 078/103] Move OpenTracing shim under ext dir --- .../README.rst | 0 .../setup.cfg | 46 ++++++++++++++ .../setup.py | 26 ++++++++ .../ext}/opentracingshim/__init__.py | 2 +- .../ext}/opentracingshim/util.py | 0 .../ext}/opentracingshim/version.py | 0 .../tests/__init__.py | 0 .../tests/test_shim.py | 4 +- .../tests/test_util.py | 2 +- opentracing-shim/setup.py | 62 ------------------- tox.ini | 12 ++-- 11 files changed, 82 insertions(+), 72 deletions(-) rename {opentracing-shim => ext/opentelemetry-ext-opentracing-shim}/README.rst (100%) create mode 100644 ext/opentelemetry-ext-opentracing-shim/setup.cfg create mode 100644 ext/opentelemetry-ext-opentracing-shim/setup.py rename {opentracing-shim/src => ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext}/opentracingshim/__init__.py (99%) rename {opentracing-shim/src => ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext}/opentracingshim/util.py (100%) rename {opentracing-shim/src => ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext}/opentracingshim/version.py (100%) rename {opentracing-shim => ext/opentelemetry-ext-opentracing-shim}/tests/__init__.py (100%) rename {opentracing-shim => ext/opentelemetry-ext-opentracing-shim}/tests/test_shim.py (99%) rename {opentracing-shim => ext/opentelemetry-ext-opentracing-shim}/tests/test_util.py (97%) delete mode 100644 opentracing-shim/setup.py diff --git a/opentracing-shim/README.rst b/ext/opentelemetry-ext-opentracing-shim/README.rst similarity index 100% rename from opentracing-shim/README.rst rename to ext/opentelemetry-ext-opentracing-shim/README.rst diff --git a/ext/opentelemetry-ext-opentracing-shim/setup.cfg b/ext/opentelemetry-ext-opentracing-shim/setup.cfg new file mode 100644 index 0000000000..48d59de00c --- /dev/null +++ b/ext/opentelemetry-ext-opentracing-shim/setup.cfg @@ -0,0 +1,46 @@ +# Copyright 2019, OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +[metadata] +name = opentelemetry-ot-shim +description = OpenTracing Shim for OpenTelemetry +long_description = file: README.rst +long_description_content_type = text/x-rst +author = OpenTelemetry Authors +author_email = cncf-opentelemetry-contributors@lists.cncf.io +url = https://github.com/open-telemetry/opentelemetry-python/ext/opentelemetry-ext-opentracing-shim +platforms = any +license = Apache-2.0 +classifiers = + Development Status :: 3 - Alpha + Intended Audience :: Developers + License :: OSI Approved :: Apache Software License + Programming Language :: Python + Programming Language :: Python :: 3 + Programming Language :: Python :: 3.4 + Programming Language :: Python :: 3.5 + Programming Language :: Python :: 3.6 + Programming Language :: Python :: 3.7 + +[options] +python_requires = >=3.4 +package_dir= + =src +packages=find_namespace: +install_requires = + opentracing + opentelemetry-api + +[options.packages.find] +where = src diff --git a/ext/opentelemetry-ext-opentracing-shim/setup.py b/ext/opentelemetry-ext-opentracing-shim/setup.py new file mode 100644 index 0000000000..81c5b0ded9 --- /dev/null +++ b/ext/opentelemetry-ext-opentracing-shim/setup.py @@ -0,0 +1,26 @@ +# Copyright 2019, OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import os + +import setuptools + +BASE_DIR = os.path.dirname(__file__) +VERSION_FILENAME = os.path.join( + BASE_DIR, "src", "opentelemetry", "ext", "opentracingshim", "version.py" +) +PACKAGE_INFO = {} +with open(VERSION_FILENAME) as f: + exec(f.read(), PACKAGE_INFO) + +setuptools.setup(version=PACKAGE_INFO["__version__"]) diff --git a/opentracing-shim/src/opentracingshim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py similarity index 99% rename from opentracing-shim/src/opentracingshim/__init__.py rename to ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py index e7bdb35610..6d2287e843 100644 --- a/opentracing-shim/src/opentracingshim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py @@ -16,8 +16,8 @@ import opentracing +from opentelemetry.ext.opentracingshim import util from opentelemetry.trace import Event -from opentracingshim import util def create_tracer(tracer, scope_manager=None): diff --git a/opentracing-shim/src/opentracingshim/util.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/util.py similarity index 100% rename from opentracing-shim/src/opentracingshim/util.py rename to ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/util.py diff --git a/opentracing-shim/src/opentracingshim/version.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/version.py similarity index 100% rename from opentracing-shim/src/opentracingshim/version.py rename to ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/version.py diff --git a/opentracing-shim/tests/__init__.py b/ext/opentelemetry-ext-opentracing-shim/tests/__init__.py similarity index 100% rename from opentracing-shim/tests/__init__.py rename to ext/opentelemetry-ext-opentracing-shim/tests/__init__.py diff --git a/opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py similarity index 99% rename from opentracing-shim/tests/test_shim.py rename to ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py index d1bcdb79d6..4fd7311f7d 100644 --- a/opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -17,10 +17,10 @@ import opentracing -import opentracingshim +import opentelemetry.ext.opentracingshim as opentracingshim from opentelemetry import trace +from opentelemetry.ext.opentracingshim import util from opentelemetry.sdk.trace import Tracer -from opentracingshim import util class TestShim(unittest.TestCase): diff --git a/opentracing-shim/tests/test_util.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_util.py similarity index 97% rename from opentracing-shim/tests/test_util.py rename to ext/opentelemetry-ext-opentracing-shim/tests/test_util.py index 869164eef1..7a9410f47c 100644 --- a/opentracing-shim/tests/test_util.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_util.py @@ -15,7 +15,7 @@ import time import unittest -from opentracingshim import util +from opentelemetry.ext.opentracingshim import util class TestUtil(unittest.TestCase): diff --git a/opentracing-shim/setup.py b/opentracing-shim/setup.py deleted file mode 100644 index 483e0a2943..0000000000 --- a/opentracing-shim/setup.py +++ /dev/null @@ -1,62 +0,0 @@ -# Copyright 2019, OpenTelemetry Authors -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -import os - -import setuptools - -BASE_DIR = os.path.dirname(__file__) -VERSION_FILENAME = os.path.join( - BASE_DIR, "src", "opentracingshim", "version.py" -) -PACKAGE_INFO = {} -with open(VERSION_FILENAME) as f: - exec(f.read(), PACKAGE_INFO) - -setuptools.setup( - name="opentelemetry-ot-shim", - version=PACKAGE_INFO["__version__"], - author="OpenTelemetry Authors", - author_email="cncf-opentelemetry-contributors@lists.cncf.io", - classifiers=[ - "Development Status :: 3 - Alpha", - "Intended Audience :: Developers", - "License :: OSI Approved :: Apache Software License", - "Programming Language :: Python", - "Programming Language :: Python :: 3", - "Programming Language :: Python :: 3.4", - "Programming Language :: Python :: 3.5", - "Programming Language :: Python :: 3.6", - "Programming Language :: Python :: 3.7", - ], - description="OpenTracing shim for OpenTelemetry", - include_package_data=True, - long_description=open("README.rst").read(), - install_requires=[ - "typing; python_version<'3.5'", - "opentracing", - "opentelemetry-api", - ], - extras_require={}, - license="Apache-2.0", - package_dir={"": "src"}, - packages=setuptools.find_namespace_packages( - where="src", include="opentracingshim.*" - ), - url=( - "https://github.com/open-telemetry/opentelemetry-python" - "/tree/master/opentracing-shim" - ), - zip_safe=False, -) diff --git a/tox.ini b/tox.ini index 64905d80a6..a8378a183d 100644 --- a/tox.ini +++ b/tox.ini @@ -27,7 +27,7 @@ changedir = test-ext-jaeger: ext/opentelemetry-ext-jaeger/tests test-ext-wsgi: ext/opentelemetry-ext-wsgi/tests test-example-app: examples/opentelemetry-example-app/tests - test-ot-shim: opentracing-shim/tests + test-ot-shim: ext/opentelemetry-ext-opentracing-shim/tests commands_pre = ; Install without -e to test the actual installation @@ -43,7 +43,7 @@ commands_pre = http-requests: pip install {toxinidir}/ext/opentelemetry-ext-http-requests jaeger: pip install {toxinidir}/opentelemetry-sdk jaeger: pip install {toxinidir}/ext/opentelemetry-ext-jaeger - ot-shim: pip install {toxinidir}/opentelemetry-sdk {toxinidir}/opentracing-shim + ot-shim: pip install {toxinidir}/opentelemetry-sdk {toxinidir}/ext/opentelemetry-ext-opentracing-shim ; Using file:// here because otherwise tox invokes just "pip install ; opentelemetry-api", leading to an error @@ -78,7 +78,7 @@ commands_pre = pip install -e {toxinidir}/ext/opentelemetry-ext-jaeger pip install -e {toxinidir}/ext/opentelemetry-ext-wsgi pip install -e {toxinidir}/examples/opentelemetry-example-app - pip install -e {toxinidir}/opentracing-shim + pip install -e {toxinidir}/ext/opentelemetry-ext-opentracing-shim commands = ; Prefer putting everything in one pylint command to profit from duplication @@ -95,11 +95,11 @@ commands = ext/opentelemetry-ext-http-requests/tests/ \ ext/opentelemetry-ext-jaeger/src/opentelemetry \ ext/opentelemetry-ext-jaeger/tests/ \ + ext/opentelemetry-ext-opentracing-shim/src/ \ + ext/opentelemetry-ext-opentracing-shim/tests/ \ ext/opentelemetry-ext-wsgi/tests/ \ examples/opentelemetry-example-app/src/opentelemetry_example_app/ \ - examples/opentelemetry-example-app/tests/ \ - opentracing-shim/src/ \ - opentracing-shim/tests/ + examples/opentelemetry-example-app/tests/ flake8 . isort --check-only --diff --recursive . From 78183cccaf47926e9c74a9744d36062de1cdc378 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 10 Oct 2019 16:22:07 +0200 Subject: [PATCH 079/103] Rename package to opentelemetry-opentracing-shim "ot" isn't clear enough and could lead to confusion. --- ext/opentelemetry-ext-opentracing-shim/README.rst | 6 +++--- ext/opentelemetry-ext-opentracing-shim/setup.cfg | 2 +- tox.ini | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/README.rst b/ext/opentelemetry-ext-opentracing-shim/README.rst index 71603778ce..2e81391219 100644 --- a/ext/opentelemetry-ext-opentracing-shim/README.rst +++ b/ext/opentelemetry-ext-opentracing-shim/README.rst @@ -3,15 +3,15 @@ OpenTracing Shim for OpenTelemetry |pypi| -.. |pypi| image:: https://badge.fury.io/py/opentelemetry-ot-shim.svg - :target: https://pypi.org/project/opentelemetry-ot-shim/ +.. |pypi| image:: https://badge.fury.io/py/opentelemetry-opentracing-shim.svg + :target: https://pypi.org/project/opentelemetry-opentracing-shim/ Installation ------------ :: - pip install opentelemetry-ot-shim + pip install opentelemetry-opentracing-shim References ---------- diff --git a/ext/opentelemetry-ext-opentracing-shim/setup.cfg b/ext/opentelemetry-ext-opentracing-shim/setup.cfg index 48d59de00c..c3b750f80f 100644 --- a/ext/opentelemetry-ext-opentracing-shim/setup.cfg +++ b/ext/opentelemetry-ext-opentracing-shim/setup.cfg @@ -13,7 +13,7 @@ # limitations under the License. # [metadata] -name = opentelemetry-ot-shim +name = opentelemetry-opentracing-shim description = OpenTracing Shim for OpenTelemetry long_description = file: README.rst long_description_content_type = text/x-rst diff --git a/tox.ini b/tox.ini index a8378a183d..c13dd2f1fe 100644 --- a/tox.ini +++ b/tox.ini @@ -2,8 +2,8 @@ skipsdist = True skip_missing_interpreters = True envlist = - py3{4,5,6,7,8}-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger,ot-shim} - pypy3-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger,ot-shim} + py3{4,5,6,7,8}-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger,opentracing-shim} + pypy3-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger,opentracing-shim} lint py37-{mypy,mypyinstalled} docs @@ -15,7 +15,7 @@ python = [testenv] deps = mypy,mypyinstalled: mypy~=0.740 - ot-shim: opentracing + opentracing-shim: opentracing setenv = mypy: MYPYPATH={toxinidir}/opentelemetry-api/src/ @@ -27,7 +27,7 @@ changedir = test-ext-jaeger: ext/opentelemetry-ext-jaeger/tests test-ext-wsgi: ext/opentelemetry-ext-wsgi/tests test-example-app: examples/opentelemetry-example-app/tests - test-ot-shim: ext/opentelemetry-ext-opentracing-shim/tests + test-opentracing-shim: ext/opentelemetry-ext-opentracing-shim/tests commands_pre = ; Install without -e to test the actual installation @@ -43,7 +43,7 @@ commands_pre = http-requests: pip install {toxinidir}/ext/opentelemetry-ext-http-requests jaeger: pip install {toxinidir}/opentelemetry-sdk jaeger: pip install {toxinidir}/ext/opentelemetry-ext-jaeger - ot-shim: pip install {toxinidir}/opentelemetry-sdk {toxinidir}/ext/opentelemetry-ext-opentracing-shim + opentracing-shim: pip install {toxinidir}/opentelemetry-sdk {toxinidir}/ext/opentelemetry-ext-opentracing-shim ; Using file:// here because otherwise tox invokes just "pip install ; opentelemetry-api", leading to an error From 4f88e7c22fc498f01277e588214fc283ac9c9c99 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 10 Oct 2019 16:36:18 +0200 Subject: [PATCH 080/103] Call init on super before setting own attributes --- .../src/opentelemetry/ext/opentracingshim/__init__.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py index 6d2287e843..e0d0d0edcb 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py @@ -41,8 +41,8 @@ def baggage(self): class SpanWrapper(opentracing.Span): def __init__(self, tracer, context, span): - self._otel_span = span super().__init__(tracer, context) + self._otel_span = span def unwrap(self): """Returns the wrapped OpenTelemetry `Span` object.""" @@ -127,15 +127,13 @@ def active(self): class TracerWrapper(opentracing.Tracer): def __init__(self, tracer, scope_manager=None): - self._otel_tracer = tracer - # If a scope manager isn't provided by the user, create a # `ScopeManagerWrapper` instance and use it to initialize the # `TracerWrapper`. if scope_manager is None: scope_manager = ScopeManagerWrapper(self) - super().__init__(scope_manager=scope_manager) + self._otel_tracer = tracer def unwrap(self): """Returns the wrapped OpenTelemetry `Tracer` object.""" From 8a7c1a7cfa8c62c0e43c99cc65f295e1dc5a3ffc Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 10 Oct 2019 20:45:03 +0200 Subject: [PATCH 081/103] Don't override active_span on TracerWrapper If the user passes a custom ScopeManager, we want the TracerWrapper to call `active_span` on the custom ScopeManager. --- .../opentelemetry/ext/opentracingshim/__init__.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py index e0d0d0edcb..3cde8d7fae 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py @@ -120,7 +120,10 @@ def activate(self, span, finish_on_close): @property def active(self): - span = self._tracer.get_current_span() + span = self._tracer.unwrap().get_current_span() + if span is None: + return None + wrapped_span = SpanWrapper(self._tracer, span.get_context(), span) return ScopeWrapper(self, wrapped_span) @@ -140,13 +143,6 @@ def unwrap(self): return self._otel_tracer - @property - def active_span(self): - span = self._otel_tracer.get_current_span() - if span is None: - return None - return SpanWrapper(self, span.get_context(), span) - @contextmanager def start_active_span( self, From cbe971b1830227aba9fe03840fd74364c4d1dc97 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 11 Oct 2019 13:03:56 +0200 Subject: [PATCH 082/103] Refactor handling of invalid parent type --- .../ext/opentracingshim/__init__.py | 30 +++++++++++++------ .../tests/test_shim.py | 12 ++++++++ 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py index 3cde8d7fae..b9d97eae56 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import logging from contextlib import contextmanager import opentracing @@ -19,6 +20,8 @@ from opentelemetry.ext.opentracingshim import util from opentelemetry.trace import Event +logger = logging.getLogger(__name__) + def create_tracer(tracer, scope_manager=None): return TracerWrapper(tracer, scope_manager) @@ -174,18 +177,27 @@ def start_span( start_time=None, ignore_active_span=False, ): - parent = child_of - if parent is not None: - if isinstance(parent, SpanWrapper): - parent = child_of.unwrap() - elif isinstance(parent, SpanContextWrapper): + if child_of is None: + parent = None + else: + # TODO: Should we use the OpenTracing base classes for the type + # check? + if isinstance(child_of, (SpanWrapper, SpanContextWrapper)): + # The parent specified in `child_of` is valid and is either a + # `SpanWrapper` or a `SpanContextWrapper`. Unwrap the `Span` or + # `SpanContext` to extract the OpenTracing object and use this + # object as the parent of the created span. parent = child_of.unwrap() else: - raise RuntimeError( - "Invalid parent type when calling start_span()." + logger.warning( + "Unknown class %s passed in child_of argument to start_span() method.", + type(child_of), ) - # TODO: Should we use the OpenTracing base classes for the type - # check? + parent = None + # TODO: Refuse to create a span and return `None` instead of + # proceeding with a `None` parent? This would cause the created + # span to become a child of the active span, if any, or create + # a new trace and make the span the root span of that trace. # Use active span as parent when no explicit parent is specified. if ( diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py index 4fd7311f7d..6a69992dc0 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -312,6 +312,18 @@ def test_parent_child_explicit_span_context(self): child.unwrap().parent, parent.context.unwrap() ) + def test_parent_child_explicit_invalid(self): + """Test span creation with an explicit parent of an invalid type.""" + + with self.shim.start_active_span("ParentSpan") as parent: + with self.shim.start_active_span( + "ChildSpan", child_of=object + ) as child: + # Verify span was created as a child of the active span. + self.assertEqual( + child.span.unwrap().parent, parent.span.unwrap() + ) + def test_references(self): """Test span creation using the `references` argument.""" From 97f373456a3b4f4888414f6195f1708e16be4673 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 11 Oct 2019 13:44:50 +0200 Subject: [PATCH 083/103] Don't raise on unimplemented methods Log a warning instead in order not to break application code. --- .../ext/opentracingshim/__init__.py | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py index b9d97eae56..d85eedad37 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py @@ -38,7 +38,10 @@ def unwrap(self): @property def baggage(self): - raise NotImplementedError + logger.warning( + "Using unimplemented property baggage on class %s.", + self.__class__.__name__, + ) # TODO: Implement. @@ -84,11 +87,17 @@ def log_kv(self, key_values, timestamp=None): return self def set_baggage_item(self, key, value): - raise NotImplementedError + logger.warning( + "Calling unimplemented method set_baggage_item() on class %s", + self.__class__.__name__, + ) # TODO: Implement. def get_baggage_item(self, key): - raise NotImplementedError + logger.warning( + "Calling unimplemented method get_baggage_item() on class %s", + self.__class__.__name__, + ) # TODO: Implement. # TODO: Verify calls to deprecated methods `log_event()` and `log()` on @@ -232,10 +241,16 @@ def start_span( def inject(self, span_context, format, carrier): # pylint: disable=redefined-builtin - raise NotImplementedError + logger.warning( + "Calling unimplemented method inject() on class %s", + self.__class__.__name__, + ) # TODO: Implement. def extract(self, format, carrier): # pylint: disable=redefined-builtin - raise NotImplementedError + logger.warning( + "Calling unimplemented method extract() on class %s", + self.__class__.__name__, + ) # TODO: Implement. From 0017eeebdaa4dd4e7183e542126c1b262a376603 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 11 Oct 2019 15:56:31 +0200 Subject: [PATCH 084/103] Allow passing a timestamp to add_event() --- opentelemetry-api/src/opentelemetry/trace/__init__.py | 10 +++++++--- .../src/opentelemetry/sdk/trace/__init__.py | 7 +++++-- opentelemetry-sdk/tests/trace/test_trace.py | 5 ++++- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/trace/__init__.py b/opentelemetry-api/src/opentelemetry/trace/__init__.py index fff5d556b9..2a21212fae 100644 --- a/opentelemetry-api/src/opentelemetry/trace/__init__.py +++ b/opentelemetry-api/src/opentelemetry/trace/__init__.py @@ -180,12 +180,16 @@ def set_attribute(self, key: str, value: types.AttributeValue) -> None: """ def add_event( - self, name: str, attributes: types.Attributes = None + self, + name: str, + timestamp: int = None, + attributes: types.Attributes = None, ) -> None: """Adds an `Event`. - Adds a single `Event` with the name and, optionally, attributes passed - as arguments. + Adds a single `Event` with the name and, optionally, a timestamp and + attributes passed as arguments. Implementations should generate a + timestamp if the `timestamp` argument is omitted. """ def add_lazy_event(self, event: Event) -> None: diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 7b274f852f..f8a058d87f 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -199,12 +199,15 @@ def set_attribute(self, key: str, value: types.AttributeValue) -> None: self.attributes[key] = value def add_event( - self, name: str, attributes: types.Attributes = None + self, + name: str, + timestamp: int = None, + attributes: types.Attributes = None, ) -> None: self.add_lazy_event( trace_api.Event( name, - time_ns(), + time_ns() if timestamp is None else timestamp, Span.empty_attributes if attributes is None else attributes, ) ) diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index fa8547a0a5..d1e3033d52 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -227,8 +227,10 @@ def test_span_members(self): # events root.add_event("event0") - root.add_event("event1", {"name": "birthday"}) now = time_ns() + root.add_event( + "event1", timestamp=now, attributes={"name": "birthday"} + ) root.add_lazy_event( trace_api.Event("event2", now, {"name": "hello"}) ) @@ -240,6 +242,7 @@ def test_span_members(self): self.assertEqual(root.events[1].name, "event1") self.assertEqual(root.events[1].attributes, {"name": "birthday"}) + self.assertEqual(root.events[1].timestamp, now) self.assertEqual(root.events[2].name, "event2") self.assertEqual(root.events[2].attributes, {"name": "hello"}) From c644c106569fc65e540436999280cd25e5d5a2de Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 11 Oct 2019 16:32:29 +0200 Subject: [PATCH 085/103] Let the OTel implementation create event timestamps Let the OTel implementation create event timestamps instead of generating a timestamp in the shim, i.e. outside of OpenTelemetry. --- .../src/opentelemetry/ext/opentracingshim/__init__.py | 6 ++---- .../src/opentelemetry/ext/opentracingshim/util.py | 11 ----------- .../tests/test_util.py | 3 ++- 3 files changed, 4 insertions(+), 16 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py index d85eedad37..c220667170 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py @@ -18,7 +18,6 @@ import opentracing from opentelemetry.ext.opentracingshim import util -from opentelemetry.trace import Event logger = logging.getLogger(__name__) @@ -77,11 +76,10 @@ def log_kv(self, key_values, timestamp=None): if timestamp is not None: event_timestamp = util.time_seconds_to_ns(timestamp) else: - event_timestamp = util.time_ns() + event_timestamp = None event_name = util.event_name_from_kv(key_values) - event = Event(event_name, event_timestamp, key_values) - self._otel_span.add_lazy_event(event) + self._otel_span.add_event(event_name, event_timestamp, key_values) # Return self for call chaining. return self diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/util.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/util.py index a4e5142ffc..97e2415e44 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/util.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/util.py @@ -12,17 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import time - -try: - time_ns = time.time_ns -# Python versions < 3.7 -except AttributeError: - - def time_ns(): - return int(time.time() * 1e9) - - # A default event name to be used for logging events when a better event name # can't be derived from the event's key-value pairs. DEFAULT_EVENT_NAME = "log" diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_util.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_util.py index 7a9410f47c..5c09981591 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_util.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_util.py @@ -16,6 +16,7 @@ import unittest from opentelemetry.ext.opentracingshim import util +from opentelemetry.util import time_ns class TestUtil(unittest.TestCase): @@ -44,7 +45,7 @@ def test_time_seconds_to_ns(self): self.assertEqual(result, int(time_seconds * 1e9)) def test_time_seconds_from_ns(self): - time_nanoseconds = util.time_ns() + time_nanoseconds = time_ns() result = util.time_seconds_from_ns(time_nanoseconds) self.assertEqual(result, time_nanoseconds / 1e9) From 55e692a2d07d81ae92f823beb6ada66d8168a89d Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 14 Oct 2019 13:40:09 +0200 Subject: [PATCH 086/103] Refactor scope handling - Remove an unnecessary context manager layers. - Allow creating `ContextWrapper` objects either from a `SpanWrapper` or from a `Span` context manager. --- .../ext/opentracingshim/__init__.py | 67 +++++++++++++++---- 1 file changed, 54 insertions(+), 13 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py index c220667170..3637da1184 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py @@ -13,7 +13,6 @@ # limitations under the License. import logging -from contextlib import contextmanager import opentracing @@ -104,11 +103,55 @@ def get_baggage_item(self, key): class ScopeWrapper(opentracing.Scope): + """A `ScopeWrapper` wraps the OpenTelemetry functionality related to span + activation/deactivation while using OpenTracing `Scope` objects for + presentation. + + There are two ways to construct a `ScopeWrapper` object: using the `span` + argument and using the `span_cm` argument. One and only one of `span` or + `span_cm` must be specified. + + When calling the initializer while passing a `SpanWrapper` object in the + `span` argument, the `ScopeWrapper` is initialized as a regular `Scope` + object. + + When calling the initializer while passing a context manager *generator* in + the `span_cm` argument (as returned by the `use_span()` method of + OpenTelemetry `Tracer` objects, the resulting `ScopeWrapper` becomes + usable as a context manager (using `with` statements). + + It is necessary to have both ways for constructing `ScopeWrapper` + objects because in some cases we need to create the object from a context + manager, in which case our only way of retrieving a `Span` object is by + calling the `__enter__()` method on the context manager, which makes the + span active in the OpenTelemetry tracer; whereas in other cases we need to + accept a `SpanWrapper` object and wrap it in a `ScopeWrapper`. + """ + + def __init__(self, manager, span=None, span_cm=None): + super().__init__(manager, span) + self._span_cm = span_cm + def close(self): - self._span.finish() + self._span.unwrap().end() # TODO: Set active span on OpenTelemetry tracer. # https://github.com/open-telemetry/opentelemetry-python/issues/161#issuecomment-534136274 + def __enter__(self): + if self._span_cm is not None: + otel_span = self._span_cm.__enter__() + self._span = SpanWrapper( + self._manager.tracer, otel_span.get_context(), otel_span + ) + return self + + def __exit__(self, exc_type, exc_val, exc_tb): + # Span._on_error(self.span, exc_type, exc_val, exc_tb) + # TODO: Do we want this ^ ? It exists in the overridden method on the + # base class in the OpenTracing API. + if self._span_cm is not None: + self._span_cm.__exit__(exc_type, exc_val, exc_tb) + class ScopeManagerWrapper(opentracing.ScopeManager): def __init__(self, tracer): @@ -118,15 +161,11 @@ def __init__(self, tracer): # pylint: disable=super-init-not-called self._tracer = tracer - @contextmanager def activate(self, span, finish_on_close): - with self._tracer.unwrap().use_span( + span_cm = self._tracer.unwrap().use_span( span.unwrap(), end_on_exit=finish_on_close - ) as otel_span: - wrapped_span = SpanWrapper( - self._tracer, otel_span.get_context(), otel_span - ) - yield ScopeWrapper(self, wrapped_span) + ) + return ScopeWrapper(self, span_cm=span_cm) @property def active(self): @@ -135,7 +174,11 @@ def active(self): return None wrapped_span = SpanWrapper(self._tracer, span.get_context(), span) - return ScopeWrapper(self, wrapped_span) + return ScopeWrapper(self, span=wrapped_span) + + @property + def tracer(self): + return self._tracer class TracerWrapper(opentracing.Tracer): @@ -153,7 +196,6 @@ def unwrap(self): return self._otel_tracer - @contextmanager def start_active_span( self, operation_name, @@ -172,8 +214,7 @@ def start_active_span( start_time=start_time, ignore_active_span=ignore_active_span, ) - with self._scope_manager.activate(span, finish_on_close) as scope: - yield scope + return self._scope_manager.activate(span, finish_on_close) def start_span( self, From 9210853670e3bc3c7e68a2789fd3e699fd22d724 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 14 Oct 2019 13:45:25 +0200 Subject: [PATCH 087/103] Remove opentracing runtime dependency from tox.ini --- tox.ini | 2 -- 1 file changed, 2 deletions(-) diff --git a/tox.ini b/tox.ini index c13dd2f1fe..a6abe8e158 100644 --- a/tox.ini +++ b/tox.ini @@ -15,7 +15,6 @@ python = [testenv] deps = mypy,mypyinstalled: mypy~=0.740 - opentracing-shim: opentracing setenv = mypy: MYPYPATH={toxinidir}/opentelemetry-api/src/ @@ -68,7 +67,6 @@ deps = flake8~=3.7 isort~=4.3 black>=19.3b0,==19.* - opentracing~=2.2 commands_pre = pip install -e {toxinidir}/opentelemetry-api From cb41689566cb4479b095d178690ce739ef8aceca Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 14 Oct 2019 20:48:00 +0200 Subject: [PATCH 088/103] Clean up test setup --- ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py index 6a69992dc0..b961f86878 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -27,7 +27,8 @@ class TestShim(unittest.TestCase): # pylint: disable=too-many-public-methods def setUp(self): - # TODO: Review setup. + """Create an OpenTelemetry tracer and a shim before every test case.""" + self.tracer = trace.tracer() self.shim = opentracingshim.create_tracer(self.tracer) @@ -36,9 +37,7 @@ def setUpClass(cls): """Set preferred tracer implementation only once rather than before every test method. """ - # TODO: Do we need to call setUpClass() on super()? - # Seems to work fine without it. - super(TestShim, cls).setUpClass() + trace.set_preferred_tracer_implementation(lambda T: Tracer()) def test_shim_type(self): From 04d44e38d9b24a17cd9a253ab8d4f7966a2ba58c Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 14 Oct 2019 20:55:16 +0200 Subject: [PATCH 089/103] Rename package to opentracing_shim --- ext/opentelemetry-ext-opentracing-shim/setup.py | 2 +- .../ext/{opentracingshim => opentracing_shim}/__init__.py | 2 +- .../ext/{opentracingshim => opentracing_shim}/util.py | 0 .../ext/{opentracingshim => opentracing_shim}/version.py | 0 ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py | 4 ++-- ext/opentelemetry-ext-opentracing-shim/tests/test_util.py | 2 +- 6 files changed, 5 insertions(+), 5 deletions(-) rename ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/{opentracingshim => opentracing_shim}/__init__.py (99%) rename ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/{opentracingshim => opentracing_shim}/util.py (100%) rename ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/{opentracingshim => opentracing_shim}/version.py (100%) diff --git a/ext/opentelemetry-ext-opentracing-shim/setup.py b/ext/opentelemetry-ext-opentracing-shim/setup.py index 81c5b0ded9..bbec88b500 100644 --- a/ext/opentelemetry-ext-opentracing-shim/setup.py +++ b/ext/opentelemetry-ext-opentracing-shim/setup.py @@ -17,7 +17,7 @@ BASE_DIR = os.path.dirname(__file__) VERSION_FILENAME = os.path.join( - BASE_DIR, "src", "opentelemetry", "ext", "opentracingshim", "version.py" + BASE_DIR, "src", "opentelemetry", "ext", "opentracing_shim", "version.py" ) PACKAGE_INFO = {} with open(VERSION_FILENAME) as f: diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py similarity index 99% rename from ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py rename to ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index 3637da1184..3b928be878 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -16,7 +16,7 @@ import opentracing -from opentelemetry.ext.opentracingshim import util +from opentelemetry.ext.opentracing_shim import util logger = logging.getLogger(__name__) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/util.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/util.py similarity index 100% rename from ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/util.py rename to ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/util.py diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/version.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/version.py similarity index 100% rename from ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracingshim/version.py rename to ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/version.py diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py index b961f86878..9089a06e22 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -17,9 +17,9 @@ import opentracing -import opentelemetry.ext.opentracingshim as opentracingshim +import opentelemetry.ext.opentracing_shim as opentracingshim from opentelemetry import trace -from opentelemetry.ext.opentracingshim import util +from opentelemetry.ext.opentracing_shim import util from opentelemetry.sdk.trace import Tracer diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_util.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_util.py index 5c09981591..84bdc73a6a 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_util.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_util.py @@ -15,7 +15,7 @@ import time import unittest -from opentelemetry.ext.opentracingshim import util +from opentelemetry.ext.opentracing_shim import util from opentelemetry.util import time_ns From add02e73497cdfc5b9ba5889c4f7e206561c5a97 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 14 Oct 2019 21:02:03 +0200 Subject: [PATCH 090/103] Don't allow using arbitrary scope managers Until we figure out how to handle unsupported frameworks (e.g. gevent), we remove the `scope_manager` argument of `create_tracer` to avoid breaking the OpenTelemetry context propagation. --- .../opentelemetry/ext/opentracing_shim/__init__.py | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index 3b928be878..f90ed9f33e 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -21,8 +21,8 @@ logger = logging.getLogger(__name__) -def create_tracer(tracer, scope_manager=None): - return TracerWrapper(tracer, scope_manager) +def create_tracer(tracer): + return TracerWrapper(tracer) class SpanContextWrapper(opentracing.SpanContext): @@ -182,13 +182,8 @@ def tracer(self): class TracerWrapper(opentracing.Tracer): - def __init__(self, tracer, scope_manager=None): - # If a scope manager isn't provided by the user, create a - # `ScopeManagerWrapper` instance and use it to initialize the - # `TracerWrapper`. - if scope_manager is None: - scope_manager = ScopeManagerWrapper(self) - super().__init__(scope_manager=scope_manager) + def __init__(self, tracer): + super().__init__(scope_manager=ScopeManagerWrapper(self)) self._otel_tracer = tracer def unwrap(self): From c2b03a7e963ad7fae870271c704632658f65e796 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 14 Oct 2019 21:05:24 +0200 Subject: [PATCH 091/103] Remove unnecessary comments. These are obvious and therefore unnecessary. --- .../src/opentelemetry/ext/opentracing_shim/__init__.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index f90ed9f33e..c63e45a98a 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -55,8 +55,6 @@ def unwrap(self): def set_operation_name(self, operation_name): self._otel_span.update_name(operation_name) - - # Return self for call chaining. return self def finish(self, finish_time=None): @@ -67,8 +65,6 @@ def finish(self, finish_time=None): def set_tag(self, key, value): self._otel_span.set_attribute(key, value) - - # Return self for call chaining. return self def log_kv(self, key_values, timestamp=None): @@ -79,8 +75,6 @@ def log_kv(self, key_values, timestamp=None): event_name = util.event_name_from_kv(key_values) self._otel_span.add_event(event_name, event_timestamp, key_values) - - # Return self for call chaining. return self def set_baggage_item(self, key, value): From ecfd2b8701538c949eea0d0bb5be5f6c3af417f9 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Mon, 14 Oct 2019 21:17:38 +0200 Subject: [PATCH 092/103] Remove TODO --- .../src/opentelemetry/ext/opentracing_shim/__init__.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index c63e45a98a..36ef61ab44 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -217,8 +217,6 @@ def start_span( if child_of is None: parent = None else: - # TODO: Should we use the OpenTracing base classes for the type - # check? if isinstance(child_of, (SpanWrapper, SpanContextWrapper)): # The parent specified in `child_of` is valid and is either a # `SpanWrapper` or a `SpanContextWrapper`. Unwrap the `Span` or From 3a78b2cf8f4c5bc03b702ced2f7ebdca0da2c848 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Tue, 15 Oct 2019 13:55:02 +0200 Subject: [PATCH 093/103] Don't accept scalar values in references argument The API docs are clear about the `references` argument being a list. We don't try to compensate for wrong argument types elsewhere, so no reason to do that here. --- .../src/opentelemetry/ext/opentracing_shim/__init__.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index 36ef61ab44..6fa6931561 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -245,8 +245,6 @@ def start_span( span = self._otel_tracer.create_span(operation_name, parent) if references: - if not isinstance(references, list): - references = [references] for ref in references: span.add_link(ref.referenced_context.unwrap()) From 10f72aca08de0eedf4e939622a76ad3b844c242b Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Tue, 15 Oct 2019 15:26:43 +0200 Subject: [PATCH 094/103] Rename XWrapper to XShim Not all classes in the shim are actually wrappers, which makes the XWrapper naming convention confusing. XShim is more neutral. --- .../ext/opentracing_shim/__init__.py | 44 +++++++++---------- .../tests/test_shim.py | 14 +++--- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index 6fa6931561..9a3f0fd909 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -22,10 +22,10 @@ def create_tracer(tracer): - return TracerWrapper(tracer) + return TracerShim(tracer) -class SpanContextWrapper(opentracing.SpanContext): +class SpanContextShim(opentracing.SpanContext): def __init__(self, otel_context): self._otel_context = otel_context @@ -43,7 +43,7 @@ def baggage(self): # TODO: Implement. -class SpanWrapper(opentracing.Span): +class SpanShim(opentracing.Span): def __init__(self, tracer, context, span): super().__init__(tracer, context) self._otel_span = span @@ -96,30 +96,30 @@ def get_baggage_item(self, key): # `log_kv()`). -class ScopeWrapper(opentracing.Scope): - """A `ScopeWrapper` wraps the OpenTelemetry functionality related to span +class ScopeShim(opentracing.Scope): + """A `ScopeShim` wraps the OpenTelemetry functionality related to span activation/deactivation while using OpenTracing `Scope` objects for presentation. - There are two ways to construct a `ScopeWrapper` object: using the `span` + There are two ways to construct a `ScopeShim` object: using the `span` argument and using the `span_cm` argument. One and only one of `span` or `span_cm` must be specified. - When calling the initializer while passing a `SpanWrapper` object in the - `span` argument, the `ScopeWrapper` is initialized as a regular `Scope` + When calling the initializer while passing a `SpanShim` object in the + `span` argument, the `ScopeShim` is initialized as a regular `Scope` object. When calling the initializer while passing a context manager *generator* in the `span_cm` argument (as returned by the `use_span()` method of - OpenTelemetry `Tracer` objects, the resulting `ScopeWrapper` becomes + OpenTelemetry `Tracer` objects, the resulting `ScopeShim` becomes usable as a context manager (using `with` statements). - It is necessary to have both ways for constructing `ScopeWrapper` + It is necessary to have both ways for constructing `ScopeShim` objects because in some cases we need to create the object from a context manager, in which case our only way of retrieving a `Span` object is by calling the `__enter__()` method on the context manager, which makes the span active in the OpenTelemetry tracer; whereas in other cases we need to - accept a `SpanWrapper` object and wrap it in a `ScopeWrapper`. + accept a `SpanShim` object and wrap it in a `ScopeShim`. """ def __init__(self, manager, span=None, span_cm=None): @@ -134,7 +134,7 @@ def close(self): def __enter__(self): if self._span_cm is not None: otel_span = self._span_cm.__enter__() - self._span = SpanWrapper( + self._span = SpanShim( self._manager.tracer, otel_span.get_context(), otel_span ) return self @@ -147,7 +147,7 @@ def __exit__(self, exc_type, exc_val, exc_tb): self._span_cm.__exit__(exc_type, exc_val, exc_tb) -class ScopeManagerWrapper(opentracing.ScopeManager): +class ScopeManagerShim(opentracing.ScopeManager): def __init__(self, tracer): # The only thing the `__init__()` method on the base class does is # initialize `self._noop_span` and `self._noop_scope` with no-op @@ -159,7 +159,7 @@ def activate(self, span, finish_on_close): span_cm = self._tracer.unwrap().use_span( span.unwrap(), end_on_exit=finish_on_close ) - return ScopeWrapper(self, span_cm=span_cm) + return ScopeShim(self, span_cm=span_cm) @property def active(self): @@ -167,17 +167,17 @@ def active(self): if span is None: return None - wrapped_span = SpanWrapper(self._tracer, span.get_context(), span) - return ScopeWrapper(self, span=wrapped_span) + wrapped_span = SpanShim(self._tracer, span.get_context(), span) + return ScopeShim(self, span=wrapped_span) @property def tracer(self): return self._tracer -class TracerWrapper(opentracing.Tracer): +class TracerShim(opentracing.Tracer): def __init__(self, tracer): - super().__init__(scope_manager=ScopeManagerWrapper(self)) + super().__init__(scope_manager=ScopeManagerShim(self)) self._otel_tracer = tracer def unwrap(self): @@ -217,9 +217,9 @@ def start_span( if child_of is None: parent = None else: - if isinstance(child_of, (SpanWrapper, SpanContextWrapper)): + if isinstance(child_of, (SpanShim, SpanContextShim)): # The parent specified in `child_of` is valid and is either a - # `SpanWrapper` or a `SpanContextWrapper`. Unwrap the `Span` or + # `SpanShim` or a `SpanContextShim`. Unwrap the `Span` or # `SpanContext` to extract the OpenTracing object and use this # object as the parent of the created span. parent = child_of.unwrap() @@ -260,8 +260,8 @@ def start_span( start_time_ns = util.time_seconds_to_ns(start_time) span.start(start_time=start_time_ns) - context = SpanContextWrapper(span.get_context()) - return SpanWrapper(self, context, span) + context = SpanContextShim(span.get_context()) + return SpanShim(self, context, span) def inject(self, span_context, format, carrier): # pylint: disable=redefined-builtin diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py index 9089a06e22..540e9fc58a 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -59,7 +59,7 @@ def test_start_active_span(self): self.assertEqual(self.shim.active_span.context, scope.span.context) # TODO: We can't check for equality of self.shim.active_span and # scope.span because the same OpenTelemetry span is returned inside - # different SpanWrapper objects. A possible solution is described + # different SpanShim objects. A possible solution is described # here: # https://github.com/open-telemetry/opentelemetry-python/issues/161#issuecomment-534136274 @@ -205,10 +205,10 @@ def test_activate_finish_on_close(self): span.finish() def test_explicit_scope_close(self): - """Test `close()` method on `ScopeWrapper`.""" + """Test `close()` method on `ScopeShim`.""" span = self.shim.start_span("TestSpan") - scope = opentracingshim.ScopeWrapper(self.shim.scope_manager, span) + scope = opentracingshim.ScopeShim(self.shim.scope_manager, span) # Verify span hasn't ended. self.assertIsNone(span.unwrap().end_time) @@ -249,8 +249,8 @@ def test_parent_child_implicit(self): self.shim.active_span.context, parent.span.context # TODO: Check equality of the spans themselves rather than - # their context once the SpanWrapper reconstruction problem - # has been addressed (see previous TODO). + # their context once the SpanShim reconstruction problem has + # been addressed (see previous TODO). ) # Verify there is no active span. @@ -387,10 +387,10 @@ def test_log_kv(self): self.assertAlmostEqual(result, now, places=6) def test_span_context(self): - """Test construction of `SpanContextWrapper` objects.""" + """Test construction of `SpanContextShim` objects.""" otel_context = trace.SpanContext(1234, 5678) - context = opentracingshim.SpanContextWrapper(otel_context) + context = opentracingshim.SpanContextShim(otel_context) self.assertIsInstance(context, opentracing.SpanContext) self.assertEqual(context.unwrap().trace_id, 1234) From aee40a32eba680df5a30affec9b32cfc936adf0a Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Tue, 15 Oct 2019 15:53:30 +0200 Subject: [PATCH 095/103] Don't call active_span() when we have a parent --- .../src/opentelemetry/ext/opentracing_shim/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index 9a3f0fd909..520295e924 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -236,9 +236,9 @@ def start_span( # Use active span as parent when no explicit parent is specified. if ( - self.active_span is not None + not parent and not ignore_active_span - and not parent + and self.active_span is not None ): parent = self.active_span.unwrap() From f2e91c83fb9dcb54752e3f26dfc3b9cd40a3c574 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Tue, 15 Oct 2019 22:21:39 +0200 Subject: [PATCH 096/103] Append exception info to span In the OpenTracing API, when an exception occurs while a span is active, information about the exception is added to the span as logs and a tag is added as well. We re-add this functionality to the shim since we've overridden the `__exit__()` method in which it lives. --- .../ext/opentracing_shim/__init__.py | 4 +--- .../tests/test_shim.py | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index 520295e924..7116040a0a 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -140,9 +140,7 @@ def __enter__(self): return self def __exit__(self, exc_type, exc_val, exc_tb): - # Span._on_error(self.span, exc_type, exc_val, exc_tb) - # TODO: Do we want this ^ ? It exists in the overridden method on the - # base class in the OpenTracing API. + opentracing.Span._on_error(self.span, exc_type, exc_val, exc_tb) if self._span_cm is not None: self._span_cm.__exit__(exc_type, exc_val, exc_tb) diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py index 540e9fc58a..d550e075de 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -395,3 +395,19 @@ def test_span_context(self): self.assertIsInstance(context, opentracing.SpanContext) self.assertEqual(context.unwrap().trace_id, 1234) self.assertEqual(context.unwrap().span_id, 5678) + + def test_span_on_error(self): + """Verify error tag and logs are created on span when an exception is + raised. + """ + + # Raise an exception while a span is active. + with self.assertRaises(Exception): + with self.shim.start_active_span("TestName") as scope: + raise Exception + + # Verify exception details have been added to span. + self.assertEqual(scope.span.unwrap().attributes["error"], True) + self.assertEqual( + scope.span.unwrap().events[0].attributes["error.kind"], Exception + ) From f093ee1b7a8569cf0989eec4a7e16f4c06b4b17f Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Tue, 15 Oct 2019 23:04:57 +0200 Subject: [PATCH 097/103] Refactor ScopeShim --- .../ext/opentracing_shim/__init__.py | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index 7116040a0a..cc9d4094a8 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -126,23 +126,26 @@ def __init__(self, manager, span=None, span_cm=None): super().__init__(manager, span) self._span_cm = span_cm - def close(self): - self._span.unwrap().end() - # TODO: Set active span on OpenTelemetry tracer. - # https://github.com/open-telemetry/opentelemetry-python/issues/161#issuecomment-534136274 - - def __enter__(self): + # If a span context manager is provided, extract the `Span` object from + # it, wrap the extracted span and save it as an attribute. if self._span_cm is not None: otel_span = self._span_cm.__enter__() self._span = SpanShim( self._manager.tracer, otel_span.get_context(), otel_span ) - return self - def __exit__(self, exc_type, exc_val, exc_tb): - opentracing.Span._on_error(self.span, exc_type, exc_val, exc_tb) + def close(self): if self._span_cm is not None: - self._span_cm.__exit__(exc_type, exc_val, exc_tb) + # We don't have error information to pass to `__exit__()` so we + # pass `None` in all arguments. If the OpenTelemetry tracer + # implementation requires this information, the `__exit__()` method + # on `opentracing.Scope` should be overridden and modified to pass + # the relevant values to this `close()` method. + self._span_cm.__exit__(None, None, None) + else: + self._span.unwrap().end() + # TODO: Set active span on OpenTelemetry tracer. + # https://github.com/open-telemetry/opentelemetry-python/issues/161#issuecomment-534136274 class ScopeManagerShim(opentracing.ScopeManager): From 1f4cc3cde97a6c28e61e9fbb26ea9ce9f5128257 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 16 Oct 2019 16:03:23 +0200 Subject: [PATCH 098/103] Don't validate parent type Validating the parent type complicates the logic of `start_span()`. Since the OpenTracing API clearly specifies the valid types for a parent span, it's the user's responsibility to specify a valid parent. --- .../ext/opentracing_shim/__init__.py | 31 +++---------------- .../tests/test_shim.py | 12 ------- 2 files changed, 5 insertions(+), 38 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index cc9d4094a8..55f578fb90 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -215,34 +215,13 @@ def start_span( start_time=None, ignore_active_span=False, ): - if child_of is None: - parent = None - else: - if isinstance(child_of, (SpanShim, SpanContextShim)): - # The parent specified in `child_of` is valid and is either a - # `SpanShim` or a `SpanContextShim`. Unwrap the `Span` or - # `SpanContext` to extract the OpenTracing object and use this - # object as the parent of the created span. - parent = child_of.unwrap() - else: - logger.warning( - "Unknown class %s passed in child_of argument to start_span() method.", - type(child_of), - ) - parent = None - # TODO: Refuse to create a span and return `None` instead of - # proceeding with a `None` parent? This would cause the created - # span to become a child of the active span, if any, or create - # a new trace and make the span the root span of that trace. - # Use active span as parent when no explicit parent is specified. - if ( - not parent - and not ignore_active_span - and self.active_span is not None - ): - parent = self.active_span.unwrap() + if not ignore_active_span and not child_of: + child_of = self.active_span + # Use the specified parent or the active span if possible. Otherwise, + # use a `None` parent, which triggers the creation of a new trace. + parent = child_of.unwrap() if child_of else None span = self._otel_tracer.create_span(operation_name, parent) if references: diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py index d550e075de..d98de0195c 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -311,18 +311,6 @@ def test_parent_child_explicit_span_context(self): child.unwrap().parent, parent.context.unwrap() ) - def test_parent_child_explicit_invalid(self): - """Test span creation with an explicit parent of an invalid type.""" - - with self.shim.start_active_span("ParentSpan") as parent: - with self.shim.start_active_span( - "ChildSpan", child_of=object - ) as child: - # Verify span was created as a child of the active span. - self.assertEqual( - child.span.unwrap().parent, parent.span.unwrap() - ) - def test_references(self): """Test span creation using the `references` argument.""" From 970ce6a2895501de959c737e92c01280194e1ceb Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 16 Oct 2019 17:50:57 +0200 Subject: [PATCH 099/103] Verify ScopeShim.close() deactivates span Verify that calling `close()` on a `ScopeShim` deactivates the span in the OpenTelemetry tracer. --- .../ext/opentracing_shim/__init__.py | 2 -- .../tests/test_shim.py | 27 ++++++++++++++----- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index 55f578fb90..70adf15bb0 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -144,8 +144,6 @@ def close(self): self._span_cm.__exit__(None, None, None) else: self._span.unwrap().end() - # TODO: Set active span on OpenTelemetry tracer. - # https://github.com/open-telemetry/opentelemetry-python/issues/161#issuecomment-534136274 class ScopeManagerShim(opentracing.ScopeManager): diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py index d98de0195c..4845bd7fb2 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -207,16 +207,29 @@ def test_activate_finish_on_close(self): def test_explicit_scope_close(self): """Test `close()` method on `ScopeShim`.""" - span = self.shim.start_span("TestSpan") - scope = opentracingshim.ScopeShim(self.shim.scope_manager, span) + with self.shim.start_active_span("ParentSpan") as parent: + # Verify parent span is active. + self.assertEqual( + self.shim.active_span.context, parent.span.context + ) - # Verify span hasn't ended. - self.assertIsNone(span.unwrap().end_time) + child = self.shim.start_active_span("ChildSpan") - scope.close() + # Verify child span is active. + self.assertEqual(self.shim.active_span.context, child.span.context) - # Verify span has ended. - self.assertIsNotNone(span.unwrap().end_time) + # Verify child span hasn't ended. + self.assertIsNone(child.span.unwrap().end_time) + + child.close() + + # Verify child span has ended. + self.assertIsNotNone(child.span.unwrap().end_time) + + # Verify parent span becomes active again. + self.assertEqual( + self.shim.active_span.context, parent.span.context + ) def test_parent_child_implicit(self): """Test parent-child relationship and activation/deactivation of spans From fc12138d28007970b8873ff3e5429950b7787216 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Thu, 17 Oct 2019 14:56:00 +0200 Subject: [PATCH 100/103] Wrap span context properly We shouldn't return OpenTelemetry objects to the OpenTracing API. --- .../ext/opentracing_shim/__init__.py | 6 ++- .../tests/test_shim.py | 41 ++++++++++++++----- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index 70adf15bb0..d18cc753c6 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -130,8 +130,9 @@ def __init__(self, manager, span=None, span_cm=None): # it, wrap the extracted span and save it as an attribute. if self._span_cm is not None: otel_span = self._span_cm.__enter__() + span_context = SpanContextShim(otel_span.get_context()) self._span = SpanShim( - self._manager.tracer, otel_span.get_context(), otel_span + self._manager.tracer, span_context, otel_span ) def close(self): @@ -166,7 +167,8 @@ def active(self): if span is None: return None - wrapped_span = SpanShim(self._tracer, span.get_context(), span) + span_context = SpanContextShim(span.get_context()) + wrapped_span = SpanShim(self._tracer, span_context, span) return ScopeShim(self, span=wrapped_span) @property diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py index 4845bd7fb2..b6691911dd 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -56,7 +56,10 @@ def test_start_active_span(self): self.assertIsNotNone(scope.span.unwrap().start_time) # Verify span is active. - self.assertEqual(self.shim.active_span.context, scope.span.context) + self.assertEqual( + self.shim.active_span.context.unwrap(), + scope.span.context.unwrap(), + ) # TODO: We can't check for equality of self.shim.active_span and # scope.span because the same OpenTelemetry span is returned inside # different SpanShim objects. A possible solution is described @@ -149,7 +152,10 @@ def test_explicit_span_activation(self): span, finish_on_close=True ) as scope: # Verify span is active. - self.assertEqual(self.shim.active_span.context, scope.span.context) + self.assertEqual( + self.shim.active_span.context.unwrap(), + scope.span.context.unwrap(), + ) # Verify no span is active. self.assertIsNone(self.shim.active_span) @@ -186,7 +192,10 @@ def test_activate_finish_on_close(self): span, finish_on_close=True ) as scope: # Verify span is active. - self.assertEqual(self.shim.active_span.context, scope.span.context) + self.assertEqual( + self.shim.active_span.context.unwrap(), + scope.span.context.unwrap(), + ) # Verify span has ended. self.assertIsNotNone(span.unwrap().end_time) @@ -197,7 +206,10 @@ def test_activate_finish_on_close(self): span, finish_on_close=False ) as scope: # Verify span is active. - self.assertEqual(self.shim.active_span.context, scope.span.context) + self.assertEqual( + self.shim.active_span.context.unwrap(), + scope.span.context.unwrap(), + ) # Verify span hasn't ended. self.assertIsNone(span.unwrap().end_time) @@ -210,13 +222,17 @@ def test_explicit_scope_close(self): with self.shim.start_active_span("ParentSpan") as parent: # Verify parent span is active. self.assertEqual( - self.shim.active_span.context, parent.span.context + self.shim.active_span.context.unwrap(), + parent.span.context.unwrap(), ) child = self.shim.start_active_span("ChildSpan") # Verify child span is active. - self.assertEqual(self.shim.active_span.context, child.span.context) + self.assertEqual( + self.shim.active_span.context.unwrap(), + child.span.context.unwrap(), + ) # Verify child span hasn't ended. self.assertIsNone(child.span.unwrap().end_time) @@ -228,7 +244,8 @@ def test_explicit_scope_close(self): # Verify parent span becomes active again. self.assertEqual( - self.shim.active_span.context, parent.span.context + self.shim.active_span.context.unwrap(), + parent.span.context.unwrap(), ) def test_parent_child_implicit(self): @@ -239,13 +256,15 @@ def test_parent_child_implicit(self): with self.shim.start_active_span("ParentSpan") as parent: # Verify parent span is the active span. self.assertEqual( - self.shim.active_span.context, parent.span.context + self.shim.active_span.context.unwrap(), + parent.span.context.unwrap(), ) with self.shim.start_active_span("ChildSpan") as child: # Verify child span is the active span. self.assertEqual( - self.shim.active_span.context, child.span.context + self.shim.active_span.context.unwrap(), + child.span.context.unwrap(), ) # Verify parent-child relationship. @@ -259,8 +278,8 @@ def test_parent_child_implicit(self): # Verify parent span becomes the active span again. self.assertEqual( - self.shim.active_span.context, - parent.span.context + self.shim.active_span.context.unwrap(), + parent.span.context.unwrap() # TODO: Check equality of the spans themselves rather than # their context once the SpanShim reconstruction problem has # been addressed (see previous TODO). From fc6f34ea4b74be059c536617ad660c134030bb61 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 18 Oct 2019 22:11:57 +0200 Subject: [PATCH 101/103] Refactor ScopeShim initializers --- .../ext/opentracing_shim/__init__.py | 52 ++++++++----------- 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index d18cc753c6..d2e6c57422 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -101,39 +101,33 @@ class ScopeShim(opentracing.Scope): activation/deactivation while using OpenTracing `Scope` objects for presentation. - There are two ways to construct a `ScopeShim` object: using the `span` - argument and using the `span_cm` argument. One and only one of `span` or - `span_cm` must be specified. - - When calling the initializer while passing a `SpanShim` object in the - `span` argument, the `ScopeShim` is initialized as a regular `Scope` - object. - - When calling the initializer while passing a context manager *generator* in - the `span_cm` argument (as returned by the `use_span()` method of - OpenTelemetry `Tracer` objects, the resulting `ScopeShim` becomes - usable as a context manager (using `with` statements). - - It is necessary to have both ways for constructing `ScopeShim` - objects because in some cases we need to create the object from a context - manager, in which case our only way of retrieving a `Span` object is by - calling the `__enter__()` method on the context manager, which makes the - span active in the OpenTelemetry tracer; whereas in other cases we need to - accept a `SpanShim` object and wrap it in a `ScopeShim`. + There are two ways to construct a `ScopeShim` object: using the default + initializer and using the `from_context_manager()` class method. + + It is necessary to have both ways for constructing `ScopeShim` objects + because in some cases we need to create the object from a context manager, + in which case our only way of retrieving a `Span` object is by calling the + `__enter__()` method on the context manager, which makes the span active in + the OpenTelemetry tracer; whereas in other cases we need to accept a + `SpanShim` object and wrap it in a `ScopeShim`. """ - def __init__(self, manager, span=None, span_cm=None): + def __init__(self, manager, span, span_cm=None): super().__init__(manager, span) self._span_cm = span_cm - # If a span context manager is provided, extract the `Span` object from - # it, wrap the extracted span and save it as an attribute. - if self._span_cm is not None: - otel_span = self._span_cm.__enter__() - span_context = SpanContextShim(otel_span.get_context()) - self._span = SpanShim( - self._manager.tracer, span_context, otel_span - ) + # TODO: Change type of `manager` argument to `opentracing.ScopeManager`? We + # need to get rid of `manager.tracer` for this. + @classmethod + def from_context_manager(cls, manager, span_cm): + """Constructs a `ScopeShim` from an OpenTelemetry `Span` context + manager (as returned by `Tracer.use_span()`). + """ + + otel_span = span_cm.__enter__() + span_context = SpanContextShim(otel_span.get_context()) + span = SpanShim(manager.tracer, span_context, otel_span) + return cls(manager, span, span_cm) def close(self): if self._span_cm is not None: @@ -159,7 +153,7 @@ def activate(self, span, finish_on_close): span_cm = self._tracer.unwrap().use_span( span.unwrap(), end_on_exit=finish_on_close ) - return ScopeShim(self, span_cm=span_cm) + return ScopeShim.from_context_manager(self, span_cm=span_cm) @property def active(self): From 7860f550734c1c3825100fa0830f7e4bb81e4347 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 23 Oct 2019 18:36:07 +0200 Subject: [PATCH 102/103] Add TODO --- .../src/opentelemetry/ext/opentracing_shim/__init__.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index d2e6c57422..cfa01ce451 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -164,6 +164,11 @@ def active(self): span_context = SpanContextShim(span.get_context()) wrapped_span = SpanShim(self._tracer, span_context, span) return ScopeShim(self, span=wrapped_span) + # TODO: The returned `ScopeShim` instance here always ends the + # corresponding span, regardless of the `finish_on_close` value used + # when activating the span. This is because here we return a *new* + # `ScopeShim` rather than returning a saved instance of `ScopeShim`. + # https://github.com/open-telemetry/opentelemetry-python/pull/211/files#r335398792 @property def tracer(self): From 082183aa59e8e6e5496a3c6bb25ec01359d0f7f0 Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Wed, 23 Oct 2019 18:39:13 +0200 Subject: [PATCH 103/103] Rename tracer argument of create_tracer() Make it clearer that the provided tracer object should be an OpenTelemetry tracer, not an OpenTracing tracer. --- .../src/opentelemetry/ext/opentracing_shim/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py index cfa01ce451..1a6479fc58 100644 --- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py +++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py @@ -21,8 +21,8 @@ logger = logging.getLogger(__name__) -def create_tracer(tracer): - return TracerShim(tracer) +def create_tracer(otel_tracer): + return TracerShim(otel_tracer) class SpanContextShim(opentracing.SpanContext):