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.

Sign up for GitHub

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

Refactor current span handling for newly created spans. #198

Merged
merged 6 commits into from
Oct 15, 2019
Merged

Refactor current span handling for newly created spans. #198

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bf80341
Refactor current span handling for newly created spans.
carlosalberto Sep 30, 2019
6f7fdd3
Update opentelemetry-api/src/opentelemetry/trace/__init__.py
carlosalberto Oct 3, 2019
a9dd24e
Apply feedback.
carlosalberto Oct 10, 2019
635f1b9
Merge branch 'master' into start_span_update
carlosalberto Oct 10, 2019
feb9334
Restore the contextmanager behavior for API's start_span().
carlosalberto Oct 10, 2019
aaad6c3
Apply more feedback.
carlosalberto Oct 10, 2019
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
88 changes: 71 additions & 17 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,16 @@
to use the API package alone without a supporting implementation.

The tracer supports creating spans that are "attached" or "detached" from the
context. By default, new spans are "attached" to the context in that they are
context. New spans are "attached" to the context in that they are
created as children of the currently active span, and the newly-created span
becomes the new active span::
can optionally become the new active span::

from opentelemetry.trace import tracer

# Create a new root span, set it as the current span in context
with tracer.start_span("parent"):
with tracer.start_as_current_span("parent"):
# Attach a new child and update the current span
with tracer.start_span("child"):
with tracer.start_as_current_span("child"):
do_work():
# Close child span, set parent as current
# Close parent span, set default span as current
Expand All @@ -62,6 +62,7 @@
"""

import enum
import types as python_types
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
import typing
from contextlib import contextmanager

Expand Down Expand Up @@ -226,6 +227,26 @@ def is_recording_events(self) -> bool:
events with the add_event operation and attributes using set_attribute.
"""

def __enter__(self) -> "Span":
Copy link
Member

Choose a reason for hiding this comment

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

👍

"""Invoked when `Span` is used as a context manager.

Returns the `Span` itself.
"""
return self

def __exit__(
self,
exc_type: typing.Optional[typing.Type[BaseException]],
exc_val: typing.Optional[BaseException],
exc_tb: typing.Optional[python_types.TracebackType],
) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
) -> bool:
) -> Optional[bool]:

According to https://github.com/python/mypy/blob/master/docs/source/protocols.rst#context-manager-protocols, and IIRC most context managers like this return null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was wondering about this one later on (specially about the null case - I'm wondering why there's nothing for this already available in the typing module ;) )

"""Ends context manager and calls ``end()`` on the `Span`.

Returns False.
"""
self.end()
return False


class TraceOptions(int):
"""A bitmask that represents options specific to the trace.
Expand Down Expand Up @@ -376,46 +397,78 @@ def get_current_span(self) -> "Span":
# pylint: disable=no-self-use
return INVALID_SPAN

@contextmanager # type: ignore
def start_span(
self,
name: str,
parent: ParentSpan = CURRENT_SPAN,
kind: SpanKind = SpanKind.INTERNAL,
) -> typing.Iterator["Span"]:
"""Context manager for span creation.
) -> "Span":
"""Starts a span.

Create a new span. Start the span and set it as the current span in
this tracer's context.
Create a new span. Start the span without setting it as the current
span in this tracer's context.

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.

On exiting the context manager stop the span and set its parent as the
On exiting the context manager ends the span.
Copy link
Member

Choose a reason for hiding this comment

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

This is kind of confusing since Span is the context manager now, not start_span.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right, this needs to be cleared up.


Example::

# tracer.get_current_span() will be used as the implicit parent.
# If none is found, the created span will be a root instance.
with tracer.start_span("two") as child:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with tracer.start_span("two") as child:
with tracer.start_span("one") as child:

If you want to make this consistent with the other example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will update.

child.add_event("child's event")

Applications that need to set the newly created span as the current
instance should use :meth:`start_as_current_span` instead.

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
Copy link
Member

Choose a reason for hiding this comment

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

Why keep this one a context manager instead of returning the span like the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't try that out. I will give it a spin and see how it looks ;)

def start_as_current_span(
Copy link
Member

@Oberon00 Oberon00 Oct 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I think we should implement start_as_current_span in the API. Other API-implementations that need special behavior here can still override. EDIT: But this is not why I "requested changes".

self,
name: str,
parent: ParentSpan = CURRENT_SPAN,
kind: SpanKind = SpanKind.INTERNAL,
) -> typing.Iterator["Span"]:
Copy link
Member

Choose a reason for hiding this comment

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

Iterator is the wrong type here. Correct would be ContextManger[Span] but that does not exist in Python 3.5. So I guess this is a case where we have to use something like -> SpanContextManager and above

MYPY = False
if MYPY:
    SpanContextManager = typing.ContextManager[Span]
else:
    SpanContextManager = typing.Any

https://mypy.readthedocs.io/en/latest/common_issues.html#import-cycles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I'd leave this out for now, as it makes the code ugly ;)

(Else, I'd add it in another PR)

Copy link
Member

@Oberon00 Oberon00 Oct 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't want to hold this PR up and I don't know why mypy accepts it and I agree that this (my suggested) code is very ugly. But: If I'm reading this correctly, the annotation is in fact wrong. If it were correct, you'd use this function as for span in tracer().start_as_current_span(). An iterator has no set_attribute, but it has __next__. A context manager has neither.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly feel like this is not the place to discuss this - this change it's even now, as we speak, in master - check out https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-api/src/opentelemetry/trace/__init__.py#L385

-> It's still defined as start_span instead of start_as_current_span and in both cases, a Span is yielded.

I have no problem adjusting things that were changed in this PR, but I honestly see no reason to change things that were there before, unless they are fatal. Otherwise, we will in my next review something else that could be improved, and then we will have to discuss them and have yet another iteration ;)

Copy link
Member

Choose a reason for hiding this comment

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

The point is that your removed the @contextmanager and consequently changed yield to return so it keeps working fine at runtime, but I think it messes up the type annotation. Unless the interaction between @contextmanager and the function return type is not taken into account by mypy, then it would already have been wrong before.

Copy link
Member

Choose a reason for hiding this comment

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

--> #219

"""Context manager for creating a new span and set it
as the current span in this tracer's context.

On exiting the context manager stops the span and set its parent as the
current span.

Example::

with tracer.start_span("one") as parent:
with tracer.start_as_current_span("one") as parent:
parent.add_event("parent's event")
with tracer.start_span("two") as child:
with tracer.start_as_current_span("two") as child:
child.add_event("child's event")
tracer.get_current_span() # returns child
tracer.get_current_span() # returns parent
tracer.get_current_span() # returns previously active span

This is a convenience method for creating spans attached to the
tracer's context. Applications that need more control over the span
lifetime should use :meth:`create_span` instead. For example::
lifetime should use :meth:`start_span` instead. For example::

with tracer.start_span(name) as span:
with tracer.start_as_current_span(name) as span:
do_work()

is equivalent to::

span = tracer.create_span(name)
span.start()
span = tracer.start_span(name)
with tracer.use_span(span, end_on_exit=True):
do_work()

Expand All @@ -428,6 +481,7 @@ def start_span(
Yields:
The newly-created span.
"""

# pylint: disable=unused-argument,no-self-use
yield INVALID_SPAN

Expand All @@ -451,7 +505,7 @@ def create_span(
Applications that need to create spans detached from the tracer's
context should use this method.

with tracer.start_span(name) as span:
with tracer.start_as_current_span(name) as span:
do_work()

This is equivalent to::
Expand Down
17 changes: 14 additions & 3 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,18 +321,29 @@ def get_current_span(self):
"""See `opentelemetry.trace.Tracer.get_current_span`."""
return self._current_span_slot.get()

@contextmanager
def start_span(
self,
name: str,
parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN,
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
) -> typing.Iterator["Span"]:
) -> "Span":
"""See `opentelemetry.trace.Tracer.start_span`."""

span = self.create_span(name, parent, kind)
span.start()
with self.use_span(span, end_on_exit=True):
return span

@contextmanager
def start_as_current_span(
Copy link
Member

Choose a reason for hiding this comment

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

I've always been struggling with naming.
Given this technically is start_span then use_span, maybe we should just call it start_and_use_span❓

Copy link
Member

Choose a reason for hiding this comment

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

I can add one more suggestion: use_new_span.

Copy link
Member

Choose a reason for hiding this comment

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

I'd actually argue that "start_span" should remain as is, and we should maybe provide separate terminology for the two components of span creation and use.

I don't imagine most consumers will really need to separate span creation and use, and may find it as needlessly verbose for "start_and_use_span".

Copy link
Member

Choose a reason for hiding this comment

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

This is also about consistency across languages.

self,
name: str,
parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN,
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
) -> typing.Iterator["Span"]:
"""See `opentelemetry.trace.Tracer.start_as_current_span`."""

span = self.start_span(name, parent, kind)
Copy link
Member

Choose a reason for hiding this comment

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

Actually the function can be implemented more simply and efficiently without @contextmanager and Iterator by replaing the with statement: return self.use_span(span, True).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah! Right. I remember this is how I envisioned it first, but I guess I was decaffeinated when I wrote it ;(

with self.use_span(span, True):
Copy link
Member

Choose a reason for hiding this comment

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

I'd re-add

Suggested change
with self.use_span(span, True):
with self.use_span(span, end_on_exit=True):

yield span
Copy link
Member

Choose a reason for hiding this comment

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

Unless I'm missing some use case, now that Span calls end on exit it looks like you could lose end_on_exit and just let use_span be responsible for setting the context:

with self.start_span(name, parent, kind) as span:
    with self.use_span(span):
        yield(span)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one didn't work - calling yield() on the Span didn't to the trick/chaining, so I had to keep the original one ;)


def create_span(
Expand Down
Loading