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

Parent is now always passed in via Context, intead of Span or SpanContext #1146

Merged
merged 25 commits into from
Oct 8, 2020
Merged
Show file tree
Hide file tree
Changes from 16 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 @@ -118,14 +118,13 @@ def _trace_prerun(self, *args, **kwargs):
return

request = task.request
tracectx = propagators.extract(carrier_extractor, request) or {}
parent = get_current_span(tracectx)
tracectx = propagators.extract(carrier_extractor, request) or None

logger.debug("prerun signal start task_id=%s", task_id)

operation_name = "{0}/{1}".format(_TASK_RUN, task.name)
span = self._tracer.start_span(
operation_name, parent=parent, kind=trace.SpanKind.CONSUMER
operation_name, context=tracectx, kind=trace.SpanKind.CONSUMER
)

activation = self._tracer.use_span(span, end_on_exit=True)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,10 @@ def start_span(
# Use the specified parent or the active span if possible. Otherwise,
# use a `None` parent, which triggers the creation of a new trace.
parent = child_of.unwrap() if child_of else None
if isinstance(parent, OtelSpanContext):
parent = DefaultSpan(parent)

parent_context = set_span_in_context(parent)

links = []
if references:
Expand All @@ -645,7 +649,7 @@ def start_span(

span = self._otel_tracer.start_span(
operation_name,
parent,
context=parent_context,
links=links,
attributes=tags,
start_time=start_time_ns,
Expand Down
2 changes: 2 additions & 0 deletions opentelemetry-api/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
([#1194](https://github.com/open-telemetry/opentelemetry-python/pull/1194))
- Make instances of SpanContext immutable
([#1134](https://github.com/open-telemetry/opentelemetry-python/pull/1134))
- Parent is now always passed in via Context, intead of Span or SpanContext
([#1146](https://github.com/open-telemetry/opentelemetry-python/pull/1146))

## Version 0.13b0

Expand Down
31 changes: 17 additions & 14 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,11 @@
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::

# Explicit parent span assignment
child = tracer.start_span("child", parent=parent)
# Explicit parent span assignment is done via the Context
from opentelemetry.trace import set_span_in_context

parent_context = set_span_in_context(parent)
codeboten marked this conversation as resolved.
Show resolved Hide resolved
child = tracer.start_span("child", context=parent_context)
codeboten marked this conversation as resolved.
Show resolved Hide resolved

try:
do_work(span=child)
Expand All @@ -77,6 +80,7 @@
from contextlib import contextmanager
from logging import getLogger

from opentelemetry.context.context import Context
from opentelemetry.trace.ids_generator import IdsGenerator, RandomIdsGenerator
from opentelemetry.trace.propagation import (
get_current_span,
Expand All @@ -102,9 +106,6 @@

logger = getLogger(__name__)

# TODO: quarantine
ParentSpan = typing.Optional[typing.Union["Span", "SpanContext"]]


class LinkBase(abc.ABC):
def __init__(self, context: "SpanContext") -> None:
Expand Down Expand Up @@ -228,7 +229,7 @@ class Tracer(abc.ABC):
def start_span(
self,
name: str,
parent: ParentSpan = CURRENT_SPAN,
context: typing.Optional[Context] = None,
kind: SpanKind = SpanKind.INTERNAL,
attributes: types.Attributes = None,
links: typing.Sequence[Link] = (),
Expand All @@ -242,8 +243,9 @@ def start_span(
method, see :meth:`start_as_current_span`.

By default the current span in the context will be used as parent, but an
explicit parent can also be specified, either a `Span` or a `opentelemetry.trace.SpanContext`.
If the specified value is `None`, the created span will be a root span.
explicit context can also be specified, by passing in a `Context` containing
a current `Span`. If there is no current span in the global `Context` or in
the specified context, the created span will be a root span.

The span can be used as a context manager. On exiting the context manager,
the span's end() method will be called.
Expand All @@ -257,7 +259,8 @@ def start_span(

Args:
name: The name of the span to be created.
parent: The span's parent. Defaults to the current span.
context: An optional Context containing the span's parent. Defaults to the
global context.
kind: The span's kind (relationship to parent). Note that is
meaningful even if there is no parent.
attributes: The span's attributes.
Expand All @@ -278,7 +281,7 @@ def start_span(
def start_as_current_span(
self,
name: str,
parent: ParentSpan = CURRENT_SPAN,
context: typing.Optional[Context] = None,
kind: SpanKind = SpanKind.INTERNAL,
attributes: types.Attributes = None,
links: typing.Sequence[Link] = (),
Expand Down Expand Up @@ -315,7 +318,8 @@ def start_as_current_span(

Args:
name: The name of the span to be created.
parent: The span's parent. Defaults to the current span.
context: An optional Context containing the span's parent. Defaults to the
global context.
kind: The span's kind (relationship to parent). Note that is
meaningful even if there is no parent.
attributes: The span's attributes.
Expand Down Expand Up @@ -357,7 +361,7 @@ class DefaultTracer(Tracer):
def start_span(
self,
name: str,
parent: ParentSpan = Tracer.CURRENT_SPAN,
context: typing.Optional[Context] = None,
kind: SpanKind = SpanKind.INTERNAL,
attributes: types.Attributes = None,
links: typing.Sequence[Link] = (),
Expand All @@ -371,7 +375,7 @@ def start_span(
def start_as_current_span(
self,
name: str,
parent: ParentSpan = Tracer.CURRENT_SPAN,
context: typing.Optional[Context] = None,
kind: SpanKind = SpanKind.INTERNAL,
attributes: types.Attributes = None,
links: typing.Sequence[Link] = (),
Expand Down Expand Up @@ -447,7 +451,6 @@ def get_tracer_provider() -> TracerProvider:
"DefaultTracerProvider",
"Link",
"LinkBase",
"ParentSpan",
"RandomIdsGenerator",
"Span",
"SpanContext",
Expand Down
2 changes: 2 additions & 0 deletions opentelemetry-sdk/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
([#1153](https://github.com/open-telemetry/opentelemetry-python/pull/1153))
- Renaming metrics Batcher to Processor
([#1203](https://github.com/open-telemetry/opentelemetry-python/pull/1203))
- Parent is now always passed in via Context, intead of Span or SpanContext
([#1146](https://github.com/open-telemetry/opentelemetry-python/pull/1146))

## Version 0.13b0

Expand Down
18 changes: 7 additions & 11 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ class Span(trace_api.Span):
name: The name of the operation this span represents
context: The immutable span context
parent: This span's parent's `opentelemetry.trace.SpanContext`, or
codeboten marked this conversation as resolved.
Show resolved Hide resolved
null if this is a root span
None if this is a root span
sampler: The sampler used to create this span
trace_config: TODO
resource: Entity producing telemetry
Expand Down Expand Up @@ -683,38 +683,34 @@ def __init__(
def start_as_current_span(
self,
name: str,
parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN,
context: Optional[context_api.Context] = None,
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
attributes: types.Attributes = None,
links: Sequence[trace_api.Link] = (),
) -> Iterator[trace_api.Span]:
span = self.start_span(name, parent, kind, attributes, links)
span = self.start_span(name, context, kind, attributes, links)
return self.use_span(span, end_on_exit=True)

def start_span( # pylint: disable=too-many-locals
self,
name: str,
parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN,
context: Optional[context_api.Context] = None,
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
attributes: types.Attributes = None,
links: Sequence[trace_api.Link] = (),
start_time: Optional[int] = None,
set_status_on_exception: bool = True,
) -> trace_api.Span:
if parent is Tracer.CURRENT_SPAN:
parent = trace_api.get_current_span()

parent_context = parent
if isinstance(parent_context, trace_api.Span):
parent_context = parent.get_context()
parent_context = trace_api.get_current_span(context).get_context()

if parent_context is not None and not isinstance(
parent_context, trace_api.SpanContext
):
raise TypeError("parent must be a Span, SpanContext or None.")
raise TypeError("parent_context must be a SpanContext or None.")
Copy link
Member

Choose a reason for hiding this comment

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

I guess this type validation is still valuable. But practically speaking, this would only be an issue if someone directly manipulates the context, and doesn't use the API functions.

Is that a common enough case to even catch here?

Alternatively I suppose this is cheap enough.


if parent_context is None or not parent_context.is_valid:
parent = parent_context = None
parent_context = None
trace_id = self.source.ids_generator.generate_trace_id()
trace_flags = None
trace_state = None
Expand Down
66 changes: 41 additions & 25 deletions opentelemetry-sdk/tests/trace/test_trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,10 @@ def test_default_sampler(self):

# Check that the default tracer creates real spans via the default
# sampler
root_span = tracer.start_span(name="root span", parent=None)
root_span = tracer.start_span(name="root span", context=None)
ctx = trace_api.set_span_in_context(root_span)
self.assertIsInstance(root_span, trace.Span)
child_span = tracer.start_span(name="child span", parent=root_span)
child_span = tracer.start_span(name="child span", context=ctx)
self.assertIsInstance(child_span, trace.Span)
self.assertTrue(root_span.context.trace_flags.sampled)
self.assertEqual(
Expand All @@ -153,9 +154,10 @@ def test_sampler_no_sampling(self):

# Check that the default tracer creates no-op spans if the sampler
# decides not to sampler
root_span = tracer.start_span(name="root span", parent=None)
root_span = tracer.start_span(name="root span", context=None)
ctx = trace_api.set_span_in_context(root_span)
self.assertIsInstance(root_span, trace_api.DefaultSpan)
child_span = tracer.start_span(name="child span", parent=root_span)
child_span = tracer.start_span(name="child span", context=ctx)
self.assertIsInstance(child_span, trace_api.DefaultSpan)
self.assertEqual(
root_span.get_context().trace_flags, trace_api.TraceFlags.DEFAULT
Expand All @@ -174,9 +176,10 @@ def test_start_span_invalid_spancontext(self):
eliminates redundant error handling logic in exporters.
"""
tracer = new_tracer()
new_span = tracer.start_span(
"root", parent=trace_api.INVALID_SPAN_CONTEXT
parent_context = trace_api.set_span_in_context(
trace_api.INVALID_SPAN_CONTEXT
)
new_span = tracer.start_span("root", context=parent_context)
self.assertTrue(new_span.context.is_valid)
self.assertIsNone(new_span.parent)

Expand Down Expand Up @@ -282,13 +285,18 @@ def test_start_span_implicit(self):
def test_start_span_explicit(self):
tracer = new_tracer()

other_parent = trace_api.SpanContext(
trace_id=0x000000000000000000000000DEADBEEF,
span_id=0x00000000DEADBEF0,
is_remote=False,
trace_flags=trace_api.TraceFlags(trace_api.TraceFlags.SAMPLED),
other_parent = trace.Span(
"name",
trace_api.SpanContext(
trace_id=0x000000000000000000000000DEADBEEF,
span_id=0x00000000DEADBEF0,
is_remote=False,
trace_flags=trace_api.TraceFlags(trace_api.TraceFlags.SAMPLED),
),
)

other_parent_context = trace_api.set_span_in_context(other_parent)

self.assertEqual(trace_api.get_current_span(), trace_api.INVALID_SPAN)

root = tracer.start_span("root")
Expand All @@ -299,27 +307,31 @@ def test_start_span_explicit(self):
with tracer.use_span(root, True):
self.assertIs(trace_api.get_current_span(), root)

with tracer.start_span("stepchild", other_parent) as child:
with tracer.start_span("stepchild", other_parent_context) as child:
# The child's parent should be the one passed in,
# not the current span.
self.assertNotEqual(child.parent, root)
self.assertIs(child.parent, other_parent)
self.assertIs(child.parent, other_parent.get_context())

self.assertIsNotNone(child.start_time)
self.assertIsNone(child.end_time)

# The child should inherit its context from the explicit
# parent, not the current span.
child_context = child.get_context()
self.assertEqual(other_parent.trace_id, child_context.trace_id)
self.assertEqual(
other_parent.get_context().trace_id, child_context.trace_id
)
self.assertNotEqual(
other_parent.span_id, child_context.span_id
other_parent.get_context().span_id, child_context.span_id
)
self.assertEqual(
other_parent.trace_state, child_context.trace_state
other_parent.get_context().trace_state,
child_context.trace_state,
)
self.assertEqual(
other_parent.trace_flags, child_context.trace_flags
other_parent.get_context().trace_flags,
child_context.trace_flags,
)

# Verify start_span() did not set the current span.
Expand Down Expand Up @@ -352,12 +364,16 @@ def test_start_as_current_span_implicit(self):
def test_start_as_current_span_explicit(self):
tracer = new_tracer()

other_parent = trace_api.SpanContext(
trace_id=0x000000000000000000000000DEADBEEF,
span_id=0x00000000DEADBEF0,
is_remote=False,
trace_flags=trace_api.TraceFlags(trace_api.TraceFlags.SAMPLED),
other_parent = trace.Span(
"name",
trace_api.SpanContext(
trace_id=0x000000000000000000000000DEADBEEF,
span_id=0x00000000DEADBEF0,
is_remote=False,
trace_flags=trace_api.TraceFlags(trace_api.TraceFlags.SAMPLED),
),
)
other_parent_ctx = trace_api.set_span_in_context(other_parent)

self.assertEqual(trace_api.get_current_span(), trace_api.INVALID_SPAN)

Expand All @@ -369,14 +385,14 @@ def test_start_as_current_span_explicit(self):
self.assertIsNone(root.end_time)

with tracer.start_as_current_span(
"stepchild", other_parent
"stepchild", other_parent_ctx
) as child:
# The child should become the current span as usual, but its
# parent should be the one passed in, not the
# previously-current span.
self.assertIs(trace_api.get_current_span(), child)
self.assertNotEqual(child.parent, root)
self.assertIs(child.parent, other_parent)
self.assertIs(child.parent, other_parent.get_context())

# After exiting the child's scope the last span on the stack should
# become current, not the child's parent.
Expand Down Expand Up @@ -538,7 +554,7 @@ def test_sampling_attributes(self):
"attr-in-both": "decision-attr",
}
tracer_provider = trace.TracerProvider(
sampling.StaticSampler(sampling.Decision.RECORD_AND_SAMPLE,)
sampling.StaticSampler(sampling.Decision.RECORD_AND_SAMPLE)
)

self.tracer = tracer_provider.get_tracer(__name__)
Expand Down