Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing create_span from the api #290

Merged
merged 9 commits into from
Nov 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 7 additions & 6 deletions ext/opentelemetry-ext-flask/tests/test_flask_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.

import unittest
from unittest import mock

from flask import Flask
from werkzeug.test import Client
Expand Down Expand Up @@ -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

Expand All @@ -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)
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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):
Expand Down
3 changes: 1 addition & 2 deletions ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
72 changes: 8 additions & 64 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand All @@ -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.
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
4 changes: 0 additions & 4 deletions opentelemetry-api/tests/trace/test_tracer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
38 changes: 10 additions & 28 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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()
Expand All @@ -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
Expand All @@ -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,
Expand All @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love some feedback on whether or not this is the right thing to do here, since a DefaultSpan no longer has a start() method, I'm just returning it after creation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, a DefaultSpan doesn't even know whether it was started anyway.

return span

@contextmanager
def use_span(
Expand Down
Loading