From 243fc19540c8d09ecf883a3b839c480d24f6e5ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20V=C3=A1squez?= Date: Tue, 5 Nov 2019 08:14:53 -0500 Subject: [PATCH 1/2] api: change order of arguments in add_event e4d89490e870 ("OpenTracing Bridge - Initial implementation (#211)") introduced a new timestamp argument to the add_event method. This commit moves that argument to be the last one because it is more common to have event attributes than an explicitly timestamp. --- .../ext/opentracing_shim/__init__.py | 2 +- .../src/opentelemetry/trace/__init__.py | 4 +-- .../src/opentelemetry/sdk/trace/__init__.py | 4 +-- opentelemetry-sdk/tests/trace/test_trace.py | 26 +++++++++++++------ 4 files changed, 23 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 7ee8ad71e0..7271d79ea9 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 @@ -75,7 +75,7 @@ def log_kv(self, key_values, timestamp=None): event_timestamp = None event_name = util.event_name_from_kv(key_values) - self._otel_span.add_event(event_name, event_timestamp, key_values) + self._otel_span.add_event(event_name, key_values, event_timestamp) return self @deprecated(reason="This method is deprecated in favor of log_kv") diff --git a/opentelemetry-api/src/opentelemetry/trace/__init__.py b/opentelemetry-api/src/opentelemetry/trace/__init__.py index 1ac7d73d49..412a106229 100644 --- a/opentelemetry-api/src/opentelemetry/trace/__init__.py +++ b/opentelemetry-api/src/opentelemetry/trace/__init__.py @@ -95,7 +95,7 @@ class Event: """A text annotation with a set of attributes.""" def __init__( - self, name: str, timestamp: int, attributes: types.Attributes = None + self, name: str, attributes: types.Attributes, timestamp: int ) -> None: self._name = name self._attributes = attributes @@ -182,8 +182,8 @@ def set_attribute(self, key: str, value: types.AttributeValue) -> None: def add_event( self, name: str, - timestamp: int = None, attributes: types.Attributes = None, + timestamp: int = None, ) -> None: """Adds an `Event`. diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 0909b9b434..d06d34d93b 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -202,14 +202,14 @@ def set_attribute(self, key: str, value: types.AttributeValue) -> None: def add_event( self, name: str, - timestamp: int = None, attributes: types.Attributes = None, + timestamp: int = None, ) -> None: self.add_lazy_event( trace_api.Event( name, - time_ns() if timestamp is None else timestamp, Span.empty_attributes if attributes is None else attributes, + time_ns() if timestamp is None else timestamp, ) ) diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index 626a5499ec..8417bd5b42 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -264,28 +264,38 @@ def test_span_members(self): self.assertEqual(root.attributes["attr-key"], "attr-value2") # events + # only event name root.add_event("event0") + + # event name and attributes now = time_ns() - root.add_event( - "event1", timestamp=now, attributes={"name": "birthday"} - ) + root.add_event("event1", {"name": "pluto"}) + + # event name, attributes and timestamp + now = time_ns() + root.add_event("event2", {"name": "birthday"}, now) + + # lazy event root.add_lazy_event( - trace_api.Event("event2", now, {"name": "hello"}) + trace_api.Event("event3", {"name": "hello"}, now) ) - self.assertEqual(len(root.events), 3) + self.assertEqual(len(root.events), 4) self.assertEqual(root.events[0].name, "event0") self.assertEqual(root.events[0].attributes, {}) 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[1].attributes, {"name": "pluto"}) self.assertEqual(root.events[2].name, "event2") - self.assertEqual(root.events[2].attributes, {"name": "hello"}) + self.assertEqual(root.events[2].attributes, {"name": "birthday"}) self.assertEqual(root.events[2].timestamp, now) + self.assertEqual(root.events[3].name, "event3") + self.assertEqual(root.events[3].attributes, {"name": "hello"}) + self.assertEqual(root.events[3].timestamp, now) + # links root.add_link(other_context1) root.add_link(other_context2, {"name": "neighbor"}) From fad73692f7f607410c936095eeca1a04f9d2dba4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20V=C3=A1squez?= Date: Tue, 5 Nov 2019 14:48:00 -0500 Subject: [PATCH 2/2] split tests to avoid lint error on max number of statements --- opentelemetry-sdk/tests/trace/test_trace.py | 51 +++++++++++---------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index 8417bd5b42..50249479a7 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -213,30 +213,15 @@ def test_start_as_current_span_explicit(self): class TestSpan(unittest.TestCase): + def setUp(self): + self.tracer = trace.Tracer("test_span") + def test_basic_span(self): span = trace.Span("name", mock.Mock(spec=trace_api.SpanContext)) self.assertEqual(span.name, "name") - def test_span_members(self): - tracer = trace.Tracer("test_span_members") - - other_context1 = trace_api.SpanContext( - trace_id=trace.generate_trace_id(), - span_id=trace.generate_span_id(), - ) - other_context2 = trace_api.SpanContext( - trace_id=trace.generate_trace_id(), - span_id=trace.generate_span_id(), - ) - other_context3 = trace_api.SpanContext( - trace_id=trace.generate_trace_id(), - span_id=trace.generate_span_id(), - ) - - self.assertIsNone(tracer.get_current_span()) - - with tracer.start_as_current_span("root") as root: - # attributes + def test_attributes(self): + with self.tracer.start_as_current_span("root") as root: root.set_attribute("component", "http") root.set_attribute("http.method", "GET") root.set_attribute( @@ -263,7 +248,10 @@ def test_span_members(self): self.assertEqual(root.attributes["misc.pi"], 3.14) self.assertEqual(root.attributes["attr-key"], "attr-value2") - # events + def test_events(self): + self.assertIsNone(self.tracer.get_current_span()) + + with self.tracer.start_as_current_span("root") as root: # only event name root.add_event("event0") @@ -296,7 +284,21 @@ def test_span_members(self): self.assertEqual(root.events[3].attributes, {"name": "hello"}) self.assertEqual(root.events[3].timestamp, now) - # links + def test_links(self): + other_context1 = trace_api.SpanContext( + trace_id=trace.generate_trace_id(), + span_id=trace.generate_span_id(), + ) + other_context2 = trace_api.SpanContext( + trace_id=trace.generate_trace_id(), + span_id=trace.generate_span_id(), + ) + other_context3 = trace_api.SpanContext( + trace_id=trace.generate_trace_id(), + span_id=trace.generate_span_id(), + ) + + with self.tracer.start_as_current_span("root") as root: root.add_link(other_context1) root.add_link(other_context2, {"name": "neighbor"}) root.add_lazy_link( @@ -323,6 +325,8 @@ def test_span_members(self): ) self.assertEqual(root.links[2].attributes, {"component": "http"}) + def test_update_name(self): + with self.tracer.start_as_current_span("root") as root: # name root.update_name("toor") self.assertEqual(root.name, "toor") @@ -369,14 +373,13 @@ def test_span_override_start_and_end_time(self): def test_ended_span(self): """"Events, attributes are not allowed after span is ended""" - tracer = trace.Tracer("test_ended_span") other_context1 = trace_api.SpanContext( trace_id=trace.generate_trace_id(), span_id=trace.generate_span_id(), ) - with tracer.start_as_current_span("root") as root: + with self.tracer.start_as_current_span("root") as root: # everything should be empty at the beginning self.assertEqual(len(root.attributes), 0) self.assertEqual(len(root.events), 0)