Remove create_span from the API (open-telemetry#290)

Simplify the API by removing create_span.

Resolves open-telemetry#152

Author: alrex

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py

@@ -62,10 +62,12 @@ def _before_flask_request():
 
     tracer = trace.tracer()
 
-    span = tracer.create_span(
-        span_name, parent_span, kind=trace.SpanKind.SERVER
+    span = tracer.start_span(
+        span_name,
+        parent_span,
+        kind=trace.SpanKind.SERVER,
+        start_time=environ.get(_ENVIRON_STARTTIME_KEY),
     )
-    span.start(environ.get(_ENVIRON_STARTTIME_KEY))
     activation = tracer.use_span(span, end_on_exit=True)
     activation.__enter__()
     environ[_ENVIRON_ACTIVATION_KEY] = activation

ext/opentelemetry-ext-flask/tests/test_flask_integration.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 import unittest
+from unittest import mock
 
 from flask import Flask
 from werkzeug.test import Client
@@ -52,12 +53,12 @@ def test_simple(self):
         self.assertEqual(200, resp.status_code)
         self.assertEqual([b"Hello: 123"], list(resp.response))
 
-        self.create_span.assert_called_with(
+        self.start_span.assert_called_with(
             "hello_endpoint",
             trace_api.INVALID_SPAN_CONTEXT,
             kind=trace_api.SpanKind.SERVER,
+            start_time=mock.ANY,
         )
-        self.assertEqual(1, self.span.start.call_count)
 
         # TODO: Change this test to use the SDK, as mocking becomes painful
 
@@ -79,12 +80,12 @@ def test_404(self):
         self.assertEqual(404, resp.status_code)
         resp.close()
 
-        self.create_span.assert_called_with(
+        self.start_span.assert_called_with(
             "/bye",
             trace_api.INVALID_SPAN_CONTEXT,
             kind=trace_api.SpanKind.SERVER,
+            start_time=mock.ANY,
         )
-        self.assertEqual(1, self.span.start.call_count)
 
         # Nope, this uses Tracer.use_span(end_on_exit)
         #  self.assertEqual(1, self.span.end.call_count)
@@ -107,12 +108,12 @@ def test_internal_error(self):
         self.assertEqual(500, resp.status_code)
         resp.close()
 
-        self.create_span.assert_called_with(
+        self.start_span.assert_called_with(
             "hello_endpoint",
             trace_api.INVALID_SPAN_CONTEXT,
             kind=trace_api.SpanKind.SERVER,
+            start_time=mock.ANY,
         )
-        self.assertEqual(1, self.span.start.call_count)
 
         # Nope, this uses Tracer.use_span(end_on_exit)
         #  self.assertEqual(1, self.span.end.call_count)

ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py

@@ -238,22 +238,21 @@ def start_span(
             for ref in references:
                 links.append(trace_api.Link(ref.referenced_context.unwrap()))
 
-        span = self._otel_tracer.create_span(
-            operation_name, parent, links=links
-        )
-
-        if tags:
-            for key, value in tags.items():
-                span.set_attribute(key, value)
-
         # 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)
+        span = self._otel_tracer.start_span(
+            operation_name,
+            parent,
+            links=links,
+            attributes=tags,
+            start_time=start_time_ns,
+        )
+
         context = SpanContextShim(span.get_context())
         return SpanShim(self, context, span)
 

ext/opentelemetry-ext-testutil/src/opentelemetry/ext/testutil/wsgitestutil.py

@@ -10,14 +10,14 @@ class WsgiTestBase(unittest.TestCase):
     def setUp(self):
         tracer = trace_api.tracer()
         self.span = mock.create_autospec(trace_api.Span, spec_set=True)
-        self.create_span_patcher = mock.patch.object(
+        self.start_span_patcher = mock.patch.object(
             tracer,
-            "create_span",
+            "start_span",
             autospec=True,
             spec_set=True,
             return_value=self.span,
         )
-        self.create_span = self.create_span_patcher.start()
+        self.start_span = self.start_span_patcher.start()
         self.write_buffer = io.BytesIO()
         self.write = self.write_buffer.write
 
@@ -29,12 +29,9 @@ def setUp(self):
         self.exc_info = None
 
     def tearDown(self):
-        self.create_span_patcher.stop()
+        self.start_span_patcher.stop()
 
     def start_response(self, status, response_headers, exc_info=None):
-        # The span should have started already
-        self.assertEqual(1, self.span.start.call_count)
-
         self.status = status
         self.response_headers = response_headers
         self.exc_info = exc_info

ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py

@@ -24,7 +24,6 @@
 
 from opentelemetry import propagators, trace
 from opentelemetry.ext.wsgi.version import __version__  # noqa
-from opentelemetry.util import time_ns
 
 
 def get_header_from_environ(
@@ -138,16 +137,13 @@ def __call__(self, environ, start_response):
             start_response: The WSGI start_response callable.
         """
 
-        start_timestamp = time_ns()
-
         tracer = trace.tracer()
         parent_span = propagators.extract(get_header_from_environ, environ)
         span_name = get_default_span_name(environ)
 
-        span = tracer.create_span(
+        span = tracer.start_span(
             span_name, parent_span, kind=trace.SpanKind.SERVER
         )
-        span.start(start_timestamp)
 
         try:
             with tracer.use_span(span):

ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py

@@ -96,10 +96,9 @@ def validate_response(self, response, error=None):
             self.assertIsNone(self.exc_info)
 
         # Verify that start_span has been called
-        self.create_span.assert_called_with(
+        self.start_span.assert_called_with(
             "/", trace_api.INVALID_SPAN_CONTEXT, kind=trace_api.SpanKind.SERVER
         )
-        self.assertEqual(1, self.span.start.call_count)
 
     def test_basic_wsgi_call(self):
         app = otel_wsgi.OpenTelemetryMiddleware(simple_wsgi)

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

@@ -30,10 +30,10 @@
 created as children of the currently active span, and the newly-created span
 can optionally become the new active span::
 
-    from opentelemetry.trace import tracer
+    from opentelemetry import trace
 
     # Create a new root span, set it as the current span in context
-    with tracer.start_as_current_span("parent"):
+    with trace.tracer().start_as_current_span("parent"):
         # Attach a new child and update the current span
         with tracer.start_as_current_span("child"):
             do_work():
@@ -43,17 +43,15 @@
 When creating a span that's "detached" from the context the active span doesn't
 change, and the caller is responsible for managing the span's lifetime::
 
-    from opentelemetry.api.trace import tracer
+    from opentelemetry import trace
 
     # Explicit parent span assignment
-    span = tracer.create_span("child", parent=parent) as child:
+    child = trace.tracer().start_span("child", parent=parent)
 
-    # The caller is responsible for starting and ending the span
-    span.start()
     try:
         do_work(span=child)
     finally:
-        span.end()
+        child.end()
 
 Applications should generally use a single global tracer, and use either
 implicit or explicit context propagation consistently throughout.
@@ -147,16 +145,6 @@ class SpanKind(enum.Enum):
 class Span:
     """A span represents a single operation within a trace."""
 
-    def start(self, start_time: typing.Optional[int] = None) -> None:
-        """Sets the current time as the span's start time.
-
-        Each span represents a single operation. The span's start time is the
-        wall time at which the operation started.
-
-        Only the first call to `start` should modify the span, and
-        implementations are free to ignore or raise on further calls.
-        """
-
     def end(self, end_time: int = None) -> None:
         """Sets the current time as the span's end time.
 
@@ -204,8 +192,7 @@ def add_lazy_event(self, event: Event) -> None:
     def update_name(self, name: str) -> None:
         """Updates the `Span` name.
 
-        This will override the name provided via :func:`Tracer.create_span`
-        or :func:`Tracer.start_span`.
+        This will override the name provided via :func:`Tracer.start_span`.
 
         Upon this update, any sampling behavior based on Span name will depend
         on the implementation.
@@ -404,6 +391,7 @@ def start_span(
         kind: SpanKind = SpanKind.INTERNAL,
         attributes: typing.Optional[types.Attributes] = None,
         links: typing.Sequence[Link] = (),
+        start_time: typing.Optional[int] = None,
     ) -> "Span":
         """Starts a span.
 
@@ -434,6 +422,7 @@ def start_span(
                 meaningful even if there is no parent.
             attributes: The span's attributes.
             links: Links span to other spans
+            start_time: Sets the start time of a span
 
         Returns:
             The newly-created span.
@@ -494,51 +483,6 @@ def start_as_current_span(
         # pylint: disable=unused-argument,no-self-use
         yield INVALID_SPAN
 
-    def create_span(
-        self,
-        name: str,
-        parent: ParentSpan = CURRENT_SPAN,
-        kind: SpanKind = SpanKind.INTERNAL,
-        attributes: typing.Optional[types.Attributes] = None,
-        links: typing.Sequence[Link] = (),
-    ) -> "Span":
-        """Creates a span.
-
-        Creating the span does not start it, and should not affect the tracer's
-        context. To start the span and update the tracer's context to make it
-        the currently active span, see :meth:`use_span`.
-
-        By default the current span will be used as parent, but an explicit
-        parent can also be specified, either a Span or a SpanContext.
-        If the specified value is `None`, the created span will be a root
-        span.
-
-        Applications that need to create spans detached from the tracer's
-        context should use this method.
-
-            with tracer.start_as_current_span(name) as span:
-                do_work()
-
-        This is equivalent to::
-
-            span = tracer.create_span(name)
-            with tracer.use_span(span):
-                do_work()
-
-        Args:
-            name: The name of the span to be created.
-            parent: The span's parent. Defaults to the current span.
-            kind: The span's kind (relationship to parent). Note that is
-                meaningful even if there is no parent.
-            attributes: The span's attributes.
-            links: Links span to other spans
-
-        Returns:
-            The newly-created span.
-        """
-        # pylint: disable=unused-argument,no-self-use
-        return INVALID_SPAN
-
     @contextmanager  # type: ignore
     def use_span(
         self, span: "Span", end_on_exit: bool = False

opentelemetry-api/tests/trace/test_tracer.py

@@ -33,10 +33,6 @@ def test_start_as_current_span(self):
         with self.tracer.start_as_current_span("") as span:
             self.assertIsInstance(span, trace.Span)
 
-    def test_create_span(self):
-        span = self.tracer.create_span("")
-        self.assertIsInstance(span, trace.Span)
-
     def test_use_span(self):
         span = trace.Span()
         with self.tracer.use_span(span):

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

@@ -314,20 +314,6 @@ def get_current_span(self):
         """See `opentelemetry.trace.Tracer.get_current_span`."""
         return self._current_span_slot.get()
 
-    def start_span(
-        self,
-        name: str,
-        parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN,
-        kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
-        attributes: Optional[types.Attributes] = None,
-        links: Sequence[trace_api.Link] = (),
-    ) -> "Span":
-        """See `opentelemetry.trace.Tracer.start_span`."""
-
-        span = self.create_span(name, parent, kind, attributes, links)
-        span.start()
-        return span
-
     def start_as_current_span(
         self,
         name: str,
@@ -341,22 +327,16 @@ def start_as_current_span(
         span = self.start_span(name, parent, kind, attributes, links)
         return self.use_span(span, end_on_exit=True)
 
-    def create_span(
+    def start_span(
         self,
         name: str,
         parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN,
         kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
         attributes: Optional[types.Attributes] = None,
         links: Sequence[trace_api.Link] = (),
-    ) -> "trace_api.Span":
-        """See `opentelemetry.trace.Tracer.create_span`.
-
-        If `parent` is null the new span will be created as a root span, i.e. a
-        span with no parent context. By default, the new span will be created
-        as a child of the current span in this tracer's context, or as a root
-        span if no current span exists.
-        """
-        span_id = generate_span_id()
+        start_time: Optional[int] = None,
+    ) -> "Span":
+        """See `opentelemetry.trace.Tracer.start_span`."""
 
         if parent is Tracer.CURRENT_SPAN:
             parent = self.get_current_span()
@@ -381,7 +361,7 @@ def create_span(
             trace_state = parent_context.trace_state
 
         context = trace_api.SpanContext(
-            trace_id, span_id, trace_options, trace_state
+            trace_id, generate_span_id(), trace_options, trace_state
         )
 
         # The sampler decides whether to create a real or no-op span at the
@@ -405,7 +385,7 @@ def create_span(
                 # apply sampling decision attributes after initial attributes
                 span_attributes = attributes.copy()
                 span_attributes.update(sampling_decision.attributes)
-            return Span(
+            span = Span(
                 name=name,
                 context=context,
                 parent=parent,
@@ -415,8 +395,10 @@ def create_span(
                 kind=kind,
                 links=links,
             )
-
-        return trace_api.DefaultSpan(context=context)
+            span.start(start_time=start_time)
+        else:
+            span = trace_api.DefaultSpan(context=context)
+        return span
 
     @contextmanager
     def use_span(