Set status properly on end (#358)

In accordance with the specification, a trace status should always be set 
on an ended span.

Author: ocelotl

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -67,7 +67,8 @@ def on_end(self, span: "Span") -> None:
         """
 
     def shutdown(self) -> None:
-        """Called when a :class:`opentelemetry.sdk.trace.Tracer` is shutdown."""
+        """Called when a :class:`opentelemetry.sdk.trace.Tracer` is shutdown.
+        """
 
 
 class MultiSpanProcessor(SpanProcessor):
@@ -299,16 +300,17 @@ def end(self, end_time: Optional[int] = None) -> None:
                 raise RuntimeError("Calling end() on a not started span.")
             has_ended = self.end_time is not None
             if not has_ended:
+                if self.status is None:
+                    self.status = Status(canonical_code=StatusCanonicalCode.OK)
+
                 self._end_time = (
                     end_time if end_time is not None else time_ns()
                 )
+
         if has_ended:
             logger.warning("Calling end() on an ended span.")
             return
 
-        if self.status is None:
-            self.set_status(Status(canonical_code=StatusCanonicalCode.OK))
-
         self.span_processor.on_end(self)
 
     def update_name(self, name: str) -> None:

opentelemetry-sdk/tests/trace/test_trace.py

@@ -15,6 +15,7 @@
 import shutil
 import subprocess
 import unittest
+from logging import ERROR, WARNING
 from unittest import mock
 
 from opentelemetry import trace as trace_api
@@ -167,8 +168,10 @@ def test_instrumentation_info(self):
 
     def test_invalid_instrumentation_info(self):
         tracer_source = trace.TracerSource()
-        tracer1 = tracer_source.get_tracer("")
-        tracer2 = tracer_source.get_tracer(None)
+        with self.assertLogs(level=ERROR):
+            tracer1 = tracer_source.get_tracer("")
+        with self.assertLogs(level=ERROR):
+            tracer2 = tracer_source.get_tracer(None)
         self.assertEqual(
             tracer1.instrumentation_info, tracer2.instrumentation_info
         )
@@ -567,7 +570,8 @@ def test_start_span(self):
 
         span.start()
         start_time = span.start_time
-        span.start()
+        with self.assertLogs(level=WARNING):
+            span.start()
         self.assertEqual(start_time, span.start_time)
 
         self.assertIs(span.status, None)
@@ -596,36 +600,45 @@ def test_span_override_start_and_end_time(self):
     def test_ended_span(self):
         """"Events, attributes are not allowed after span is ended"""
 
-        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)
-            self.assertEqual(len(root.links), 0)
+        root = self.tracer.start_span("root")
 
-            # call end first time
-            root.end()
-            end_time0 = root.end_time
+        # everything should be empty at the beginning
+        self.assertEqual(len(root.attributes), 0)
+        self.assertEqual(len(root.events), 0)
+        self.assertEqual(len(root.links), 0)
+
+        # call end first time
+        root.end()
+        end_time0 = root.end_time
 
-            # call it a second time
+        # call it a second time
+        with self.assertLogs(level=WARNING):
             root.end()
-            # end time shouldn't be changed
-            self.assertEqual(end_time0, root.end_time)
+        # end time shouldn't be changed
+        self.assertEqual(end_time0, root.end_time)
 
+        with self.assertLogs(level=WARNING):
             root.set_attribute("component", "http")
-            self.assertEqual(len(root.attributes), 0)
+        self.assertEqual(len(root.attributes), 0)
 
+        with self.assertLogs(level=WARNING):
             root.add_event("event1")
-            self.assertEqual(len(root.events), 0)
+        self.assertEqual(len(root.events), 0)
 
+        with self.assertLogs(level=WARNING):
             root.update_name("xxx")
-            self.assertEqual(root.name, "root")
+        self.assertEqual(root.name, "root")
 
-            new_status = trace_api.status.Status(
-                trace_api.status.StatusCanonicalCode.CANCELLED,
-                "Test description",
-            )
+        new_status = trace_api.status.Status(
+            trace_api.status.StatusCanonicalCode.CANCELLED, "Test description",
+        )
+
+        with self.assertLogs(level=WARNING):
             root.set_status(new_status)
-            self.assertIs(root.status, None)
+        self.assertEqual(
+            root.status.canonical_code,
+            trace_api.status.StatusCanonicalCode.OK,
+        )
 
     def test_error_status(self):
         try:

pytest.ini

@@ -1,2 +1,4 @@
 [pytest]
 addopts = -rs -v
+log_cli = true
+log_cli_level = warning