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 add_link and add_lazy_link from api/sdk #259

Merged
merged 15 commits into from
Nov 16, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,11 @@
time.sleep(0.1)
foo.set_attribute("my_atribbute", True)
foo.add_event("event in foo", {"name": "foo1"})
with tracer.start_as_current_span("bar") as bar:
with tracer.start_as_current_span(
"bar", links=[trace.Link(foo.get_context())]
) as bar:
time.sleep(0.2)
bar.set_attribute("speed", 100.0)
bar.add_link(foo.get_context())

with tracer.start_as_current_span("baz") as baz:
time.sleep(0.3)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import opentracing
from deprecated import deprecated

import opentelemetry.trace as trace_api
from opentelemetry import propagators
from opentelemetry.ext.opentracing_shim import util

Expand Down Expand Up @@ -231,11 +232,16 @@ 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
span = self._otel_tracer.create_span(operation_name, parent)

links = ()
if references:
links = []
Copy link
Member

Choose a reason for hiding this comment

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

Curious, is there a strong reason to create links as a tuple and then change to list upon modification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is not. Cleaned it up

for ref in references:
span.add_link(ref.referenced_context.unwrap())
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():
Expand Down
34 changes: 16 additions & 18 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,10 @@ def __init__(
self, context: "SpanContext", attributes: types.Attributes = None
) -> None:
self._context = context
self._attributes = attributes
if attributes is None:
self._attributes = {} # type: types.Attributes
else:
self._attributes = attributes

@property
def context(self) -> "SpanContext":
Expand Down Expand Up @@ -198,23 +201,6 @@ def add_lazy_event(self, event: Event) -> None:
Adds an `Event` that has previously been created.
"""

def add_link(
self,
link_target_context: "SpanContext",
attributes: types.Attributes = None,
) -> None:
"""Adds a `Link` to another span.

Adds a single `Link` from this Span to another Span identified by the
`SpanContext` passed as argument.
"""

def add_lazy_link(self, link: "Link") -> None:
"""Adds a `Link` to another span.

Adds a `Link` that has previously been created.
"""

def update_name(self, name: str) -> None:
"""Updates the `Span` name.

Expand Down Expand Up @@ -416,6 +402,8 @@ def start_span(
name: str,
parent: ParentSpan = CURRENT_SPAN,
kind: SpanKind = SpanKind.INTERNAL,
attributes: typing.Optional[types.Attributes] = None,
links: typing.Sequence[Link] = (),
Copy link
Member

Choose a reason for hiding this comment

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

Do we intend to support iterator (delayed expansion)? This might give perf gain if the span is not sampled (so we don't have to evaluate the iterator at all).

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 someone should read up on the motivation behind lazy links. I think lazily evaluated iterables may be a good fit for Python.

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 a good point. If we're not using __getitem__ -- and especially if we might not read the sequence at all, -- we should generally prefer Iterator over Sequence.

) -> "Span":
"""Starts a span.

Expand Down Expand Up @@ -444,6 +432,8 @@ def start_span(
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.
Expand All @@ -457,6 +447,8 @@ def start_as_current_span(
name: str,
parent: ParentSpan = CURRENT_SPAN,
kind: SpanKind = SpanKind.INTERNAL,
attributes: typing.Optional[types.Attributes] = None,
links: typing.Sequence[Link] = (),
) -> typing.Iterator["Span"]:
"""Context manager for creating a new span and set it
as the current span in this tracer's context.
Expand Down Expand Up @@ -492,6 +484,8 @@ def start_as_current_span(
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

Yields:
The newly-created span.
Expand All @@ -505,6 +499,8 @@ def create_span(
name: str,
parent: ParentSpan = CURRENT_SPAN,
kind: SpanKind = SpanKind.INTERNAL,
attributes: typing.Optional[types.Attributes] = None,
links: typing.Sequence[Link] = (),
) -> "Span":
"""Creates a span.

Expand Down Expand Up @@ -534,6 +530,8 @@ def create_span(
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.
Expand Down
5 changes: 4 additions & 1 deletion opentelemetry-api/src/opentelemetry/trace/sampling.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

# pylint: disable=unused-import
from opentelemetry.trace import Link, SpanContext
from opentelemetry.util.types import AttributeValue
from opentelemetry.util.types import Attributes, AttributeValue


class Decision:
Expand Down Expand Up @@ -53,6 +53,7 @@ def should_sample(
trace_id: int,
span_id: int,
name: str,
attributes: Optional[Attributes] = None,
links: Sequence["Link"] = (),
) -> "Decision":
pass
Expand All @@ -70,6 +71,7 @@ def should_sample(
trace_id: int,
span_id: int,
name: str,
attributes: Optional[Attributes] = None,
links: Sequence["Link"] = (),
) -> "Decision":
return self._decision
Expand Down Expand Up @@ -107,6 +109,7 @@ def should_sample(
trace_id: int,
span_id: int,
name: str,
attributes: Optional[Attributes] = None, # TODO
links: Sequence["Link"] = (),
) -> "Decision":
if parent_context is not None:
Expand Down
48 changes: 19 additions & 29 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ def __init__(
resource: None = None, # TODO
attributes: types.Attributes = None, # TODO
events: Sequence[trace_api.Event] = None, # TODO
links: Sequence[trace_api.Link] = None, # TODO
links: Sequence[trace_api.Link] = (),
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
span_processor: SpanProcessor = SpanProcessor(),
) -> None:
Expand Down Expand Up @@ -226,30 +226,6 @@ def add_lazy_event(self, event: trace_api.Event) -> None:
return
self.events.append(event)

def add_link(
self,
link_target_context: "trace_api.SpanContext",
attributes: types.Attributes = None,
) -> None:
if attributes is None:
attributes = (
Span.empty_attributes
) # TODO: empty_attributes is not a Dict. Use Mapping?
self.add_lazy_link(trace_api.Link(link_target_context, attributes))

def add_lazy_link(self, link: "trace_api.Link") -> None:
with self._lock:
if not self.is_recording_events():
return
has_ended = self.end_time is not None
if not has_ended:
if self.links is Span.empty_links:
self.links = BoundedList(MAX_NUM_LINKS)
if has_ended:
logger.warning("Calling add_link() on an ended span.")
return
self.links.append(link)

def start(self, start_time: Optional[int] = None) -> None:
with self._lock:
if not self.is_recording_events():
Expand Down Expand Up @@ -343,10 +319,12 @@ def start_span(
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)
span = self.create_span(name, parent, kind, attributes, links)
span.start()
return span

Expand All @@ -355,17 +333,21 @@ def start_as_current_span(
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] = (),
) -> Iterator[trace_api.Span]:
"""See `opentelemetry.trace.Tracer.start_as_current_span`."""

span = self.start_span(name, parent, kind)
span = self.start_span(name, parent, kind, attributes, links)
return self.use_span(span, end_on_exit=True)

def create_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`.

Expand Down Expand Up @@ -412,18 +394,26 @@ def create_span(
context.trace_id,
context.span_id,
name,
{}, # TODO: links
attributes,
links,
)

if sampling_decision.sampled:
if attributes is None:
span_attributes = sampling_decision.attributes
else:
# apply sampling decision attributes after initial attributes
span_attributes = attributes.copy()
span_attributes.update(sampling_decision.attributes)
return Span(
name=name,
context=context,
parent=parent,
sampler=self.sampler,
attributes=sampling_decision.attributes,
attributes=span_attributes,
span_processor=self._active_span_processor,
kind=kind,
links=links,
)

return trace_api.DefaultSpan(context=context)
Expand Down
61 changes: 46 additions & 15 deletions opentelemetry-sdk/tests/trace/test_trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,46 @@ def test_attributes(self):
self.assertEqual(root.attributes["misc.pi"], 3.14)
self.assertEqual(root.attributes["attr-key"], "attr-value2")

attributes = {
"attr-key": "val",
"attr-key2": "val2",
"attr-in-both": "span-attr",
}
with self.tracer.start_as_current_span(
"root2", attributes=attributes
) as root:
self.assertEqual(len(root.attributes), 3)
self.assertEqual(root.attributes["attr-key"], "val")
self.assertEqual(root.attributes["attr-key2"], "val2")
self.assertEqual(root.attributes["attr-in-both"], "span-attr")

decision_attributes = {
"sampler-attr": "sample-val",
"attr-in-both": "decision-attr",
}
self.tracer.sampler = sampling.StaticSampler(
sampling.Decision(sampled=True, attributes=decision_attributes)
)

with self.tracer.start_as_current_span("root2") as root:
self.assertEqual(len(root.attributes), 2)
self.assertEqual(root.attributes["sampler-attr"], "sample-val")
self.assertEqual(root.attributes["attr-in-both"], "decision-attr")

attributes = {
"attr-key": "val",
"attr-key2": "val2",
"attr-in-both": "span-attr",
}
with self.tracer.start_as_current_span(
"root2", attributes=attributes
) as root:
self.assertEqual(len(root.attributes), 4)
self.assertEqual(root.attributes["attr-key"], "val")
self.assertEqual(root.attributes["attr-key2"], "val2")
self.assertEqual(root.attributes["sampler-attr"], "sample-val")
self.assertEqual(root.attributes["attr-in-both"], "decision-attr")

def test_events(self):
self.assertIsNone(self.tracer.get_current_span())

Expand Down Expand Up @@ -297,13 +337,12 @@ def test_links(self):
trace_id=trace.generate_trace_id(),
span_id=trace.generate_span_id(),
)

with self.tracer.start_as_current_span("root") as root:
root.add_link(other_context1)
root.add_link(other_context2, {"name": "neighbor"})
root.add_lazy_link(
trace_api.Link(other_context3, {"component": "http"})
)
links = [
trace_api.Link(other_context1),
trace_api.Link(other_context2, {"name": "neighbor"}),
trace_api.Link(other_context3, {"component": "http"}),
]
with self.tracer.start_as_current_span("root", links=links) as root:

self.assertEqual(len(root.links), 3)
self.assertEqual(
Expand Down Expand Up @@ -374,11 +413,6 @@ def test_span_override_start_and_end_time(self):
def test_ended_span(self):
""""Events, attributes are not allowed after span is ended"""

other_context1 = trace_api.SpanContext(
trace_id=trace.generate_trace_id(),
span_id=trace.generate_span_id(),
)

with self.tracer.start_as_current_span("root") as root:
# everything should be empty at the beginning
self.assertEqual(len(root.attributes), 0)
Expand All @@ -400,9 +434,6 @@ def test_ended_span(self):
root.add_event("event1")
self.assertEqual(len(root.events), 0)

root.add_link(other_context1)
self.assertEqual(len(root.links), 0)

root.update_name("xxx")
self.assertEqual(root.name, "root")

Expand Down