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

Add support for lazy events and links #474

Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion ext/opentelemetry-ext-jaeger/tests/test_jaeger_exporter.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def test_translate_to_jaeger(self):
}

event_timestamp = base_time + 50 * 10 ** 6
event = trace_api.Event(
event = trace.Event(
name="event0",
timestamp=event_timestamp,
attributes=event_attributes,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ def test_translate_to_collector(self):
"key_float": 0.3,
}
event_timestamp = base_time + 50 * 10 ** 6
event = trace_api.Event(
event = trace.Event(
name="event0",
timestamp=event_timestamp,
attributes=event_attributes,
Expand Down
2 changes: 1 addition & 1 deletion ext/opentelemetry-ext-zipkin/tests/test_zipkin_exporter.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ def test_export(self):
}

event_timestamp = base_time + 50 * 10 ** 6
event = trace_api.Event(
event = trace.Event(
name="event0",
timestamp=event_timestamp,
attributes=event_attributes,
Expand Down
73 changes: 48 additions & 25 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,48 +87,59 @@
ParentSpan = typing.Optional[typing.Union["Span", "SpanContext"]]


class Link:
"""A link to a `Span`."""

def __init__(
self, context: "SpanContext", attributes: types.Attributes = None
) -> None:
class LinkBase(abc.ABC):
def __init__(self, context: "SpanContext") -> None:
self._context = context
if attributes is None:
self._attributes = {} # type: types.Attributes
else:
self._attributes = attributes

@property
def context(self) -> "SpanContext":
return self._context

@property
ocelotl marked this conversation as resolved.
Show resolved Hide resolved
@abc.abstractmethod
def attributes(self) -> types.Attributes:
return self._attributes
pass


class Event:
"""A text annotation with a set of attributes."""
class Link(LinkBase):
"""A link to a `Span`.

Args:
context: `SpanContext` of the `Span` to link to.
attributes: Link's attributes.
"""

def __init__(
self, name: str, attributes: types.Attributes, timestamp: int
self, context: "SpanContext", attributes: types.Attributes = None,
) -> None:
self._name = name
super().__init__(context)
self._attributes = attributes
self._timestamp = timestamp

@property
def name(self) -> str:
return self._name

@property
def attributes(self) -> types.Attributes:
return self._attributes


class LazyLink(LinkBase):
"""A link to a `Span`.
mauriciovasquezbernal marked this conversation as resolved.
Show resolved Hide resolved

Args:
context: `SpanContext` of the `Span` to link to.
link_formatter: Callable object that returns the attributes of the
Link.
"""

def __init__(
self,
context: "SpanContext",
link_formatter: types.AttributesFormatter,
Copy link
Member

Choose a reason for hiding this comment

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

I know the spec says this "SHOULD be named LinkFormatter", but I think that assumes it's returning a Link. attribute_formatter sounds like a better name in both classes because this is responsible for returning Attributes, and not specific to either links or events.

) -> None:
super().__init__(context)
self._link_formatter = link_formatter

@property
def timestamp(self) -> int:
return self._timestamp
def attributes(self) -> types.Attributes:
return self._link_formatter()
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to call this every time the user accesses attributes? It seem like it would be better to cache them after the first call.



class SpanKind(enum.Enum):
Expand Down Expand Up @@ -206,10 +217,17 @@ def add_event(
"""

@abc.abstractmethod
def add_lazy_event(self, event: Event) -> None:
def add_lazy_event(
self,
name: str,
event_formatter: types.AttributesFormatter,
timestamp: typing.Optional[int] = None,
) -> None:
"""Adds an `Event`.

Adds an `Event` that has previously been created.
Adds a single `Event` with the name, an event formatter that calculates
the attributes lazily and, optionally, a timestamp. Implementations
should generate a timestamp if the `timestamp` argument is omitted.
"""

@abc.abstractmethod
Expand Down Expand Up @@ -389,7 +407,12 @@ def add_event(
) -> None:
pass

def add_lazy_event(self, event: Event) -> None:
def add_lazy_event(
self,
name: str,
event_formatter: types.AttributesFormatter,
timestamp: typing.Optional[int] = None,
) -> None:
pass

lzchen marked this conversation as resolved.
Show resolved Hide resolved
def update_name(self, name: str) -> None:
Expand Down
3 changes: 2 additions & 1 deletion opentelemetry-api/src/opentelemetry/util/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.


from typing import Dict, Optional, Sequence, Union
from typing import Callable, Dict, Optional, Sequence, Union

AttributeValue = Union[
str,
Expand All @@ -26,3 +26,4 @@
Sequence[float],
]
Attributes = Optional[Dict[str, AttributeValue]]
AttributesFormatter = Callable[[], Optional[Dict[str, AttributeValue]]]
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
AttributesFormatter = Callable[[], Optional[Dict[str, AttributeValue]]]
AttributesFormatter = Callable[[], Attributes]

Or am I missing something about these types?

120 changes: 104 additions & 16 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.


import abc
import atexit
import logging
import random
Expand Down Expand Up @@ -114,6 +115,77 @@ def shutdown(self) -> None:
sp.shutdown()


class EventBase(abc.ABC):
def __init__(self, name: str, timestamp: Optional[int] = None) -> None:
self._name = name
if timestamp is None:
self._timestamp = time_ns()
else:
self._timestamp = timestamp

@property
Copy link
Member

Choose a reason for hiding this comment

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

Using properties in the base class means implementations can't use regular attributes. Not a big deal if we don't expect users to extend EventBase and LinkBase though.

def name(self) -> str:
return self._name

@property
def timestamp(self) -> int:
return self._timestamp

@property
@abc.abstractmethod
def attributes(self) -> types.Attributes:
pass


class Event(EventBase):
"""A text annotation with a set of attributes.

Args:
name: Name of the event.
attributes: Attributes of the event.
timestamp: Timestamp of the event. If `None` it will filled
automatically.
"""

def __init__(
self,
name: str,
attributes: types.Attributes = None,
timestamp: Optional[int] = None,
) -> None:
super().__init__(name, timestamp)
self._attributes = attributes

@property
def attributes(self) -> types.Attributes:
return self._attributes


class LazyEvent(EventBase):
"""A text annotation with a set of attributes.

Args:
name: Name of the event.
event_formatter: Callable object that returns the attributes of the
event.
timestamp: Timestamp of the event. If `None` it will filled
automatically.
"""

def __init__(
self,
name: str,
event_formatter: types.AttributesFormatter,
timestamp: Optional[int] = None,
) -> None:
super().__init__(name, timestamp)
self._event_formatter = event_formatter

@property
def attributes(self) -> types.Attributes:
return self._event_formatter()


class Span(trace_api.Span):
"""See `opentelemetry.trace.Span`.

Expand Down Expand Up @@ -149,7 +221,7 @@ def __init__(
trace_config: None = None, # TODO
resource: None = None,
attributes: types.Attributes = None, # TODO
events: Sequence[trace_api.Event] = None, # TODO
events: Sequence[Event] = None, # TODO
Copy link
Contributor

@lzchen lzchen Mar 26, 2020

Choose a reason for hiding this comment

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

Shouldn't Tracer.start_span and Tracer.start_as_current_span take in events as well in the api, since you can pass in events into the constructor of a span? I think if this is the case, you have to move the Event` definition back from the sdk to the api.

Copy link
Member Author

Choose a reason for hiding this comment

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

According to https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#span-creation the events cannot be passed at start creation time, infact we should remove them from the constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Curious, what does the # TODO refer to in the code? Can that be removed with these changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what the TODO was about. I'll remove that in a followup PR in next days.

links: Sequence[trace_api.Link] = (),
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
span_processor: SpanProcessor = SpanProcessor(),
Expand Down Expand Up @@ -266,21 +338,7 @@ def _check_attribute_value_sequence(sequence: Sequence) -> Optional[str]:
return "different type"
return None

def add_event(
self,
name: str,
attributes: types.Attributes = None,
timestamp: Optional[int] = None,
) -> None:
self.add_lazy_event(
trace_api.Event(
name,
Span._empty_attributes if attributes is None else attributes,
time_ns() if timestamp is None else timestamp,
)
)

def add_lazy_event(self, event: trace_api.Event) -> None:
def _add_event(self, event: EventBase) -> None:
with self._lock:
if not self.is_recording_events():
return
Expand All @@ -293,6 +351,36 @@ def add_lazy_event(self, event: trace_api.Event) -> None:
return
self.events.append(event)

def add_event(
self,
name: str,
attributes: types.Attributes = None,
timestamp: Optional[int] = None,
) -> None:
if attributes is None:
attributes = Span._empty_attributes
self._add_event(
Event(
name=name,
attributes=attributes,
timestamp=time_ns() if timestamp is None else timestamp,
Copy link
Member

Choose a reason for hiding this comment

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

Why do this here if EventBase constructor does the same thing?

)
)

def add_lazy_event(
self,
name: str,
event_formatter: types.AttributesFormatter,
timestamp: Optional[int] = None,
) -> None:
self._add_event(
LazyEvent(
name=name,
event_formatter=event_formatter,
timestamp=time_ns() if timestamp is None else timestamp,
)
)

def start(self, start_time: Optional[int] = None) -> None:
with self._lock:
if not self.is_recording_events():
Expand Down
19 changes: 12 additions & 7 deletions opentelemetry-sdk/tests/trace/test_trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -539,10 +539,11 @@ def test_events(self):
now = time_ns()
root.add_event("event2", {"name": "birthday"}, now)

def event_formatter():
return {"name": "hello"}

# lazy event
root.add_lazy_event(
trace_api.Event("event3", {"name": "hello"}, now)
)
root.add_lazy_event("event3", event_formatter, now)

self.assertEqual(len(root.events), 4)

Expand Down Expand Up @@ -573,11 +574,15 @@ def test_links(self):
trace_id=trace.generate_trace_id(),
span_id=trace.generate_span_id(),
)
links = [

def get_link_attributes():
return {"component": "http"}

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

self.assertEqual(len(root.links), 3)
Expand All @@ -587,7 +592,7 @@ def test_links(self):
self.assertEqual(
root.links[0].context.span_id, other_context1.span_id
)
self.assertEqual(root.links[0].attributes, {})
self.assertEqual(root.links[0].attributes, None)
self.assertEqual(
root.links[1].context.trace_id, other_context2.trace_id
)
Expand Down