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 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,10 @@ def __call__(self, environ, start_response):
path_info = environ["PATH_INFO"] or "/"
parent_span = propagators.extract(_get_header_from_environ, environ)

span = tracer.create_span(
span = tracer.start_span(
path_info, parent_span, kind=trace.SpanKind.SERVER
)
span.start()

try:
with tracer.use_span(span):
self._add_request_attributes(span, environ)
Expand Down
12 changes: 5 additions & 7 deletions ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,14 @@ class TestWsgiApplication(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 @@ -97,11 +97,10 @@ 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.span.start.assert_called_once_with()

self.status = status
self.response_headers = response_headers
Expand Down Expand Up @@ -130,10 +129,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.span.start.assert_called_with()

def test_basic_wsgi_call(self):
app = OpenTelemetryMiddleware(simple_wsgi)
Expand Down
56 changes: 6 additions & 50 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 @@ -218,8 +216,7 @@ def add_lazy_link(self, link: "Link") -> 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 @@ -500,47 +497,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,
) -> "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.

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
13 changes: 12 additions & 1 deletion opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,18 @@ def create_span(
parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN,
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
) -> "trace_api.Span":
"""See `opentelemetry.trace.Tracer.create_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.
Copy link
Member

Choose a reason for hiding this comment

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

I would remove this paragraph, and maybe also rename the method to _create_span. Or we could commit to having this method be a "semi-public" interface of the SDK, to be overriden by any vendor-SDKs that need their own Span type but nevertheless want to reuse functionality from vendor SDKs.

In any case, applications (or any instrumentation use case really) must not use this method.


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
Expand Down