From b5a2508726aab18351ea268e3746b7f8b4514984 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Tue, 14 Jul 2020 12:02:30 -0600 Subject: [PATCH 1/8] Implement get_attribute and set_attribute Fixes #239 --- .../ext/opentracing_shim/__init__.py | 26 +++---------------- .../src/opentelemetry/trace/span.py | 10 +++++++ .../src/opentelemetry/sdk/trace/__init__.py | 4 +++ 3 files changed, 18 insertions(+), 22 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 0b948216ff3..37ba2668bd3 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 @@ -270,31 +270,13 @@ def log(self, **kwargs): def log_event(self, event, payload=None): super().log_event(event, payload=payload) - def set_baggage_item(self, key, value): # pylint:disable=unused-argument - """Implements the ``set_baggage_item()`` method from the base class. + def set_baggage_item(self, key, value): - Warning: - Not implemented yet. - """ - - logger.warning( - "Calling unimplemented method set_baggage_item() on class %s", - self.__class__.__name__, - ) - # TODO: Implement. + return self._otel_span.set_attribute(key, value) - def get_baggage_item(self, key): # pylint:disable=unused-argument - """Implements the ``get_baggage_item()`` method from the base class. + def get_baggage_item(self, key): - Warning: - Not implemented yet. - """ - - logger.warning( - "Calling unimplemented method get_baggage_item() on class %s", - self.__class__.__name__, - ) - # TODO: Implement. + return self._otel_span.get_attribute(key) class ScopeShim(opentracing.Scope): diff --git a/opentelemetry-api/src/opentelemetry/trace/span.py b/opentelemetry-api/src/opentelemetry/trace/span.py index baea6670f63..53db6220c1b 100644 --- a/opentelemetry-api/src/opentelemetry/trace/span.py +++ b/opentelemetry-api/src/opentelemetry/trace/span.py @@ -37,6 +37,13 @@ def set_attribute(self, key: str, value: types.AttributeValue) -> None: Sets a single Attribute with the key and value passed as arguments. """ + @abc.abstractmethod + def get_attribute(self, key: str) -> types.AttributeValue: + """Gets an Attribute. + + Gets a single Attribute with the key passed as argument. + """ + @abc.abstractmethod def add_event( self, @@ -235,6 +242,9 @@ def end(self, end_time: typing.Optional[int] = None) -> None: def set_attribute(self, key: str, value: types.AttributeValue) -> None: pass + def get_attribute(self, key: str) -> types.AttributeValue: + pass + def add_event( self, name: str, diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 1118b424888..dadaf2f40cf 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -553,6 +553,10 @@ def set_attribute(self, key: str, value: types.AttributeValue) -> None: with self._lock: self.attributes[key] = value + def get_attribute(self, key: str) -> types.AttributeValue: + + return self.attributes[key] + @staticmethod def _filter_attribute_values(attributes: types.Attributes): if attributes: From 29725de7fe7280d14e1f8b0ec3e02ce12f79ede4 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 16 Jul 2020 18:47:56 -0600 Subject: [PATCH 2/8] Revert "Implement get_attribute and set_attribute" This reverts commit de3417252cea82f089e49bd681c35dff07463222. --- .../ext/opentracing_shim/__init__.py | 26 ++++++++++++++++--- .../src/opentelemetry/trace/span.py | 10 ------- .../src/opentelemetry/sdk/trace/__init__.py | 4 --- 3 files changed, 22 insertions(+), 18 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 37ba2668bd3..0b948216ff3 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 @@ -270,13 +270,31 @@ def log(self, **kwargs): def log_event(self, event, payload=None): super().log_event(event, payload=payload) - def set_baggage_item(self, key, value): + def set_baggage_item(self, key, value): # pylint:disable=unused-argument + """Implements the ``set_baggage_item()`` method from the base class. - return self._otel_span.set_attribute(key, value) + Warning: + Not implemented yet. + """ + + logger.warning( + "Calling unimplemented method set_baggage_item() on class %s", + self.__class__.__name__, + ) + # TODO: Implement. - def get_baggage_item(self, key): + def get_baggage_item(self, key): # pylint:disable=unused-argument + """Implements the ``get_baggage_item()`` method from the base class. - return self._otel_span.get_attribute(key) + Warning: + Not implemented yet. + """ + + logger.warning( + "Calling unimplemented method get_baggage_item() on class %s", + self.__class__.__name__, + ) + # TODO: Implement. class ScopeShim(opentracing.Scope): diff --git a/opentelemetry-api/src/opentelemetry/trace/span.py b/opentelemetry-api/src/opentelemetry/trace/span.py index 53db6220c1b..baea6670f63 100644 --- a/opentelemetry-api/src/opentelemetry/trace/span.py +++ b/opentelemetry-api/src/opentelemetry/trace/span.py @@ -37,13 +37,6 @@ def set_attribute(self, key: str, value: types.AttributeValue) -> None: Sets a single Attribute with the key and value passed as arguments. """ - @abc.abstractmethod - def get_attribute(self, key: str) -> types.AttributeValue: - """Gets an Attribute. - - Gets a single Attribute with the key passed as argument. - """ - @abc.abstractmethod def add_event( self, @@ -242,9 +235,6 @@ def end(self, end_time: typing.Optional[int] = None) -> None: def set_attribute(self, key: str, value: types.AttributeValue) -> None: pass - def get_attribute(self, key: str) -> types.AttributeValue: - pass - def add_event( self, name: str, diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index dadaf2f40cf..1118b424888 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -553,10 +553,6 @@ def set_attribute(self, key: str, value: types.AttributeValue) -> None: with self._lock: self.attributes[key] = value - def get_attribute(self, key: str) -> types.AttributeValue: - - return self.attributes[key] - @staticmethod def _filter_attribute_values(attributes: types.Attributes): if attributes: From d66b63d435b4189ef1f45eec8750e64f8744daf7 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 16 Jul 2020 20:13:45 -0600 Subject: [PATCH 3/8] Add tests --- .../ext/opentracing_shim/__init__.py | 55 ++++++------------- .../tests/test_shim.py | 35 ++++++++++-- 2 files changed, 45 insertions(+), 45 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 0b948216ff3..9a1e6f5c696 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 @@ -90,8 +90,9 @@ import opentracing from deprecated import deprecated -import opentelemetry.trace as trace_api +from opentelemetry.trace import get_current_span, INVALID_SPAN_CONTEXT, Link from opentelemetry import propagators +from opentelemetry.context import Context from opentelemetry.ext.opentracing_shim import util from opentelemetry.ext.opentracing_shim.version import __version__ from opentelemetry.trace import DefaultSpan, set_span_in_context @@ -130,6 +131,8 @@ class SpanContextShim(opentracing.SpanContext): def __init__(self, otel_context): self._otel_context = otel_context + # Context is being used here since it must be immutable. + self._baggage = Context() def unwrap(self): """Returns the wrapped :class:`opentelemetry.trace.SpanContext` @@ -144,17 +147,8 @@ def unwrap(self): @property def baggage(self): - """Implements the ``baggage`` property from the base class. - Warning: - Not implemented yet. - """ - - logger.warning( - "Using unimplemented property baggage on class %s.", - self.__class__.__name__, - ) - # TODO: Implement. + return self._baggage class SpanShim(opentracing.Span): @@ -270,31 +264,14 @@ def log(self, **kwargs): def log_event(self, event, payload=None): super().log_event(event, payload=payload) - def set_baggage_item(self, key, value): # pylint:disable=unused-argument - """Implements the ``set_baggage_item()`` method from the base class. + def set_baggage_item(self, key, value): + copy = {key_: value_ for key_, value_ in self._context._baggage.items()} + copy.update({key: value}) - Warning: - Not implemented yet. - """ + self._context._baggage = Context(copy) - logger.warning( - "Calling unimplemented method set_baggage_item() on class %s", - self.__class__.__name__, - ) - # TODO: Implement. - - def get_baggage_item(self, key): # pylint:disable=unused-argument - """Implements the ``get_baggage_item()`` method from the base class. - - Warning: - Not implemented yet. - """ - - logger.warning( - "Calling unimplemented method get_baggage_item() on class %s", - self.__class__.__name__, - ) - # TODO: Implement. + def get_baggage_item(self, key): + return self._context._baggage[key] class ScopeShim(opentracing.Scope): @@ -469,8 +446,8 @@ def active(self): shim and is likely to be handled in future versions. """ - span = trace_api.get_current_span() - if span.get_context() == trace_api.INVALID_SPAN_CONTEXT: + span = get_current_span() + if span.get_context() == INVALID_SPAN_CONTEXT: return None span_context = SpanContextShim(span.get_context()) @@ -643,7 +620,7 @@ def start_span( links = [] if references: for ref in references: - links.append(trace_api.Link(ref.referenced_context.unwrap())) + links.append(Link(ref.referenced_context.unwrap())) # The OpenTracing API expects time values to be `float` values which # represent the number of seconds since the epoch. OpenTelemetry @@ -699,10 +676,10 @@ def get_as_list(dict_object, key): propagator = propagators.get_global_httptextformat() ctx = propagator.extract(get_as_list, carrier) - span = trace_api.get_current_span(ctx) + span = get_current_span(ctx) if span is not None: otel_context = span.get_context() else: - otel_context = trace_api.INVALID_SPAN_CONTEXT + otel_context = INVALID_SPAN_CONTEXT return SpanContextShim(otel_context) diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py index 941d4280690..e9cf2b03995 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -17,10 +17,13 @@ import time from unittest import TestCase +from unittest.mock import Mock import opentracing -import opentelemetry.ext.opentracing_shim as opentracingshim +from opentelemetry.ext.opentracing_shim import ( + create_tracer, SpanContextShim, SpanShim +) from opentelemetry import propagators, trace from opentelemetry.ext.opentracing_shim import util from opentelemetry.sdk.trace import TracerProvider @@ -36,7 +39,7 @@ class TestShim(TestCase): def setUp(self): """Create an OpenTelemetry tracer and a shim before every test case.""" trace.set_tracer_provider(TracerProvider()) - self.shim = opentracingshim.create_tracer(trace.get_tracer_provider()) + self.shim = create_tracer(trace.get_tracer_provider()) @classmethod def setUpClass(cls): @@ -448,7 +451,7 @@ def test_span_context(self): """Test construction of `SpanContextShim` objects.""" otel_context = trace.SpanContext(1234, 5678, is_remote=False) - context = opentracingshim.SpanContextShim(otel_context) + context = SpanContextShim(otel_context) self.assertIsInstance(context, opentracing.SpanContext) self.assertEqual(context.unwrap().trace_id, 1234) @@ -473,7 +476,7 @@ def test_inject_http_headers(self): otel_context = trace.SpanContext( trace_id=1220, span_id=7478, is_remote=False ) - context = opentracingshim.SpanContextShim(otel_context) + context = SpanContextShim(otel_context) headers = {} self.shim.inject(context, opentracing.Format.HTTP_HEADERS, headers) @@ -486,7 +489,7 @@ def test_inject_text_map(self): otel_context = trace.SpanContext( trace_id=1220, span_id=7478, is_remote=False ) - context = opentracingshim.SpanContextShim(otel_context) + context = SpanContextShim(otel_context) # Verify Format.TEXT_MAP text_map = {} @@ -500,7 +503,7 @@ def test_inject_binary(self): otel_context = trace.SpanContext( trace_id=1220, span_id=7478, is_remote=False ) - context = opentracingshim.SpanContextShim(otel_context) + context = SpanContextShim(otel_context) # Verify exception for non supported binary format. with self.assertRaises(opentracing.UnsupportedFormatException): @@ -550,3 +553,23 @@ def test_extract_binary(self): # Verify exception for non supported binary format. with self.assertRaises(opentracing.UnsupportedFormatException): self.shim.extract(opentracing.Format.BINARY, bytearray()) + + def test_baggage(self): + + span_context_shim = SpanContextShim( + trace.SpanContext(1234, 5678, is_remote=False) + ) + + baggage = span_context_shim.baggage + + with self.assertRaises(ValueError): + baggage[1] = 3 + + with self.assertRaises(ValueError): + baggage[2] = 3 + + span_shim = SpanShim(Mock(), span_context_shim, Mock()) + + span_shim.set_baggage_item(1, 2) + + self.assertTrue(span_shim.get_baggage_item(1), 2) From 12e258effd0657f4ee5afae59bb299b09bddb3ba Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 16 Jul 2020 20:37:06 -0600 Subject: [PATCH 4/8] Fix lint --- .../opentelemetry/ext/opentracing_shim/__init__.py | 13 ++++++++++--- .../tests/test_shim.py | 8 +++++--- 2 files changed, 15 insertions(+), 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 9a1e6f5c696..033998e8d16 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 @@ -90,12 +90,17 @@ import opentracing from deprecated import deprecated -from opentelemetry.trace import get_current_span, INVALID_SPAN_CONTEXT, Link from opentelemetry import propagators from opentelemetry.context import Context from opentelemetry.ext.opentracing_shim import util from opentelemetry.ext.opentracing_shim.version import __version__ -from opentelemetry.trace import DefaultSpan, set_span_in_context +from opentelemetry.trace import ( + INVALID_SPAN_CONTEXT, + DefaultSpan, + Link, + get_current_span, + set_span_in_context, +) logger = logging.getLogger(__name__) @@ -265,12 +270,14 @@ def log_event(self, event, payload=None): super().log_event(event, payload=payload) def set_baggage_item(self, key, value): - copy = {key_: value_ for key_, value_ in self._context._baggage.items()} + # pylint: disable=protected-access + copy = self._context._baggage.copy() copy.update({key: value}) self._context._baggage = Context(copy) def get_baggage_item(self, key): + # pylint: disable=protected-access return self._context._baggage[key] diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py index e9cf2b03995..fb1a36fac0c 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -21,11 +21,13 @@ import opentracing +from opentelemetry import propagators, trace from opentelemetry.ext.opentracing_shim import ( - create_tracer, SpanContextShim, SpanShim + SpanContextShim, + SpanShim, + create_tracer, + util, ) -from opentelemetry import propagators, trace -from opentelemetry.ext.opentracing_shim import util from opentelemetry.sdk.trace import TracerProvider from opentelemetry.test.mock_httptextformat import ( MockHTTPTextFormat, From de8a06528e4efe09dd96efb0f6fd9150120ce278 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 16 Jul 2020 22:38:57 -0600 Subject: [PATCH 5/8] Fix docs --- .../src/opentelemetry/ext/opentracing_shim/__init__.py | 3 +++ 1 file changed, 3 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 033998e8d16..d35a65a1fa8 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 @@ -152,6 +152,7 @@ def unwrap(self): @property def baggage(self): + """Implements the ``baggage`` property from the base class.""" return self._baggage @@ -270,6 +271,7 @@ def log_event(self, event, payload=None): super().log_event(event, payload=payload) def set_baggage_item(self, key, value): + """Implements the ``set_baggage_item`` method from the base class.""" # pylint: disable=protected-access copy = self._context._baggage.copy() copy.update({key: value}) @@ -277,6 +279,7 @@ def set_baggage_item(self, key, value): self._context._baggage = Context(copy) def get_baggage_item(self, key): + """Implements the ``get_baggage_item`` method from the base class.""" # pylint: disable=protected-access return self._context._baggage[key] From 43422366f3ff95aea44c21c320cbeb493562a2f4 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Fri, 17 Jul 2020 15:14:34 -0600 Subject: [PATCH 6/8] Remove unnecessary test --- ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py index fb1a36fac0c..635907bc90b 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -567,9 +567,6 @@ def test_baggage(self): with self.assertRaises(ValueError): baggage[1] = 3 - with self.assertRaises(ValueError): - baggage[2] = 3 - span_shim = SpanShim(Mock(), span_context_shim, Mock()) span_shim.set_baggage_item(1, 2) From b72fd4290e49de370390b01da1ed28dfffc6dd49 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Fri, 17 Jul 2020 15:35:22 -0600 Subject: [PATCH 7/8] Use Correlations API --- .../src/opentelemetry/ext/opentracing_shim/__init__.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 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 d35a65a1fa8..73d180424dc 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 @@ -91,6 +91,7 @@ from deprecated import deprecated from opentelemetry import propagators +from opentelemetry.correlationcontext import set_correlation, get_correlation from opentelemetry.context import Context from opentelemetry.ext.opentracing_shim import util from opentelemetry.ext.opentracing_shim.version import __version__ @@ -273,15 +274,14 @@ def log_event(self, event, payload=None): def set_baggage_item(self, key, value): """Implements the ``set_baggage_item`` method from the base class.""" # pylint: disable=protected-access - copy = self._context._baggage.copy() - copy.update({key: value}) - - self._context._baggage = Context(copy) + self._context._baggage = set_correlation( + key, value, context=self._context._baggage + ) def get_baggage_item(self, key): """Implements the ``get_baggage_item`` method from the base class.""" # pylint: disable=protected-access - return self._context._baggage[key] + return get_correlation(key, context=self._context._baggage) class ScopeShim(opentracing.Scope): From 1424607404e43153444a60b6f6e3a56702c275ee Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Mon, 20 Jul 2020 15:51:20 -0600 Subject: [PATCH 8/8] Fix lint --- .../src/opentelemetry/ext/opentracing_shim/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 73d180424dc..8d8afca5cab 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 @@ -91,8 +91,8 @@ from deprecated import deprecated from opentelemetry import propagators -from opentelemetry.correlationcontext import set_correlation, get_correlation from opentelemetry.context import Context +from opentelemetry.correlationcontext import get_correlation, set_correlation from opentelemetry.ext.opentracing_shim import util from opentelemetry.ext.opentracing_shim.version import __version__ from opentelemetry.trace import (