-
Notifications
You must be signed in to change notification settings - Fork 657
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
Changes from 10 commits
ce8b47b
7ff2dde
98de769
01f2fd3
63d0236
ac9d6f6
2e5c06c
f888308
544be12
47dfa13
8d95e12
b641ba4
e136ad2
d95151d
4e0c328
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
import opentracing | ||
from deprecated import deprecated | ||
|
||
import opentelemetry.trace as trace_api | ||
from opentelemetry.ext.opentracing_shim import util | ||
|
||
logger = logging.getLogger(__name__) | ||
|
@@ -226,11 +227,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 = None | ||
if references: | ||
links = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious, is there a strong reason to create There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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": | ||
|
@@ -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. | ||
|
||
|
@@ -416,6 +402,7 @@ def start_span( | |
name: str, | ||
parent: ParentSpan = CURRENT_SPAN, | ||
kind: SpanKind = SpanKind.INTERNAL, | ||
links: typing.Sequence[Link] = (), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good point. If we're not using |
||
) -> "Span": | ||
"""Starts a span. | ||
|
||
|
@@ -444,6 +431,7 @@ 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. | ||
links: The span's links, to be used in sampling decisions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should call out sampling here. This is merely a detail, mostly the links will be used to, well, have the span linked to other spans, not to influence sampling. |
||
|
||
Returns: | ||
The newly-created span. | ||
|
@@ -457,6 +445,7 @@ def start_as_current_span( | |
name: str, | ||
parent: ParentSpan = CURRENT_SPAN, | ||
kind: SpanKind = SpanKind.INTERNAL, | ||
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. | ||
|
@@ -492,6 +481,7 @@ 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. | ||
links: The span's links, to be used in sampling decisions | ||
|
||
Yields: | ||
The newly-created span. | ||
|
@@ -505,6 +495,7 @@ def create_span( | |
name: str, | ||
parent: ParentSpan = CURRENT_SPAN, | ||
kind: SpanKind = SpanKind.INTERNAL, | ||
links: typing.Sequence[Link] = (), | ||
) -> "Span": | ||
"""Creates a span. | ||
|
||
|
@@ -534,6 +525,7 @@ 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. | ||
links: The span's links, to be used in sampling decisions | ||
|
||
Returns: | ||
The newly-created span. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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] = None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since |
||
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL, | ||
span_processor: SpanProcessor = SpanProcessor(), | ||
) -> None: | ||
|
@@ -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(): | ||
|
@@ -343,10 +319,11 @@ def start_span( | |
name: str, | ||
parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN, | ||
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL, | ||
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, links) | ||
span.start() | ||
return span | ||
|
||
|
@@ -355,17 +332,19 @@ def start_as_current_span( | |
name: str, | ||
parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN, | ||
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL, | ||
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, 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, | ||
links: Sequence[trace_api.Link] = (), | ||
) -> "trace_api.Span": | ||
"""See `opentelemetry.trace.Tracer.create_span`. | ||
|
||
|
@@ -408,11 +387,7 @@ def create_span( | |
# The sampler may also add attributes to the newly-created span, e.g. | ||
# to include information about the sampling decision. | ||
sampling_decision = self.sampler.should_sample( | ||
parent_context, | ||
context.trace_id, | ||
context.span_id, | ||
name, | ||
{}, # TODO: links | ||
parent_context, context.trace_id, context.span_id, name, links, | ||
) | ||
|
||
if sampling_decision.sampled: | ||
|
@@ -424,6 +399,7 @@ def create_span( | |
attributes=sampling_decision.attributes, | ||
span_processor=self._active_span_processor, | ||
kind=kind, | ||
links=links, | ||
) | ||
|
||
return trace_api.DefaultSpan(context=context) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty tuple should be a singleton constant, so no performance overhead over using
None
here.