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

Jump to bottom

Add Link and Event classes #130

Merged
merged 4 commits into from
Sep 13, 2019
Merged

Add Link and Event classes #130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
985b62c
remove annotation for self
mauriciovasquezbernal Sep 7, 2019
8a369fd
sdk/tests/trace/span: reorganize by member
mauriciovasquezbernal Sep 6, 2019
96b7567
add Link class
mauriciovasquezbernal Sep 6, 2019
a339e7d
add Event class
mauriciovasquezbernal Sep 6, 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
69 changes: 60 additions & 9 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 @@ -70,6 +70,47 @@
ParentSpan = typing.Optional[typing.Union["Span", "SpanContext"]]


class Link:
Copy link
Member

Choose a reason for hiding this comment

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

Since these classes are so simple, why did you not keep using namedtuple?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC the idea is that users can extend these classes (hence the lazy prefix). And personally I'd prefer to keep them as very simple classes (otherwise, I'd totally be up for named tuples ;) )

Copy link
Member Author

Choose a reason for hiding this comment

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

Even if those classes are very simple, I think having a proper class allows more flexibility, for instance as mentioned by @carlosalberto, when users want to extend those.

Copy link
Member

Choose a reason for hiding this comment

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

namedtuples can be extended, they are full classes. In fact, a common pattern, that's even mentioned in the docs of namedtuple is something like:

class MyDataCls(namedtuple('MyDataCls', 'foo bar')):
  def baz(self):
    return self.foo + self.bar

Copy link
Contributor

Choose a reason for hiding this comment

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

namedtuples can be extended

Sure, but IHMO it's still better to have this as simple classes, as it's clearer for users who want to extend it, instead of having to go read the namedtuple documentation ;)

(If we had this classes as private or SDK specific, I'd be also more open to have them as namedtuples)

Copy link
Member

Choose a reason for hiding this comment

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

@Oberon00 what's the benefit of using namedtuples here? If the goal is to avoid boilerplate we might want to use the attrs library. The docs include some reasons not to use namedtuples for data classes. One big reason is that subclasses of namedtuples have a MRO that looks very different from plain old classes.

Copy link
Member

Choose a reason for hiding this comment

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

OK, these reasons sound valid enough. I retract my objections then 😃 I think since the goal of the API is to be a low level building block we should prefer a bit of boilerplate code over an additional dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, I think there's a stronger reason for using attrs in the SDK than the API, but we may not want it at all.

"""A link to a `Span`."""

def __init__(
self, context: "SpanContext", attributes: types.Attributes = None
Copy link
Member

Choose a reason for hiding this comment

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

should attributes be typing.Optional? since it defaults to none.

Copy link
Member Author

@mauriciovasquezbernal mauriciovasquezbernal Sep 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 directly changed the type of types.Attributes to be Attributes = typing.Optional[typing.Dict[str, AttributeValue]]

) -> None:
self._context = context
self._attributes = attributes

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

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


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

def __init__(
self, name: str, timestamp: int, attributes: types.Attributes = None
) -> None:
self._name = name
Copy link
Member

Choose a reason for hiding this comment

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

A general question - do we expect this to be implemented in the API package or the SDK?
I think it makes sense to have it the API package, and I don't expect people to override this in the SDK.
Just to put the question here to see if someone has a different understanding.

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 +1 for keeping it in 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.

By keeping it on the API we are providing a reference implementation and also allowing users to extend it if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to have it the API package

Agreed, that's how we do it in Java too, btw.

self._attributes = attributes
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.

Making these properties in the API means subclasses can't make them regular attributes. Is that intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I could say it is, subclasses would have to re-implement this property if needed.

Copy link
Member

@c24t c24t Sep 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

But they could never choose to make these regular attributes. That is, you can always replace attributes with properties in a subclass, but not vice-versa.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good point with the replacing! But if the derived class defines a read-only property, it would break the base class' __init__.

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 am not totally sure I am following this conversation.
I want to go back a step first, the specifications states that Link class SHOULD be immutable, that's the reason I decided to use properties. (Maybe I should use slots too). Do you think there is a better way to make it an immutable type?

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

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

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


class Span:
"""A span represents a single operation within a trace."""

Expand Down Expand Up @@ -102,34 +143,44 @@ def get_context(self) -> "SpanContext":
A :class:`.SpanContext` with a copy of this span's immutable state.
"""

def set_attribute(
self: "Span", key: str, value: types.AttributeValue
) -> None:
def set_attribute(self, key: str, value: types.AttributeValue) -> None:
"""Sets an Attribute.

Sets a single Attribute with the key and value passed as arguments.
"""

def add_event(
self: "Span", name: str, attributes: types.Attributes = None
self, name: str, attributes: types.Attributes = None
) -> None:
"""Adds an Event.
"""Adds an `Event`.

Adds a single Event with the name and, optionally, attributes passed
Adds a single `Event` with the name and, optionally, attributes passed
as arguments.
"""

def add_lazy_event(self, event: Event) -> None:
"""Adds an `Event`.

Adds an `Event` that has previously been created.
"""

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

Adds a single Link from this Span to another Span identified by the
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:
Copy link
Member

Choose a reason for hiding this comment

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

I would almost argue that add_link should just always take a link object. Is it that much harder to do:

span.add_link(context, [attribute_1])

instead of

span.add_link(Link(context, [attribute_1]))

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it seems a bit redundant as of now, though I'd vote for having only span.add_link(context, [attribute_1]). I don't think I fully grasp the design of these "lazy" method yet, I can't see what's lazy about them.

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 have a strong opinion on any of both options, I just followed the specs: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#add-links, there the two methods are required add_xxx that receives the parameters and add_lazy_xxx that receives an object.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could optionally have only the add_link(link: Link) method, to reduce API surface - but we won't easily be able in the future to add an overload that takes a SpanContext + attributes, because of the name (add_nonlazy_link()?)

For historical reference: we added these as in Java this is a usual thing to add there (overload with different parameters, that is). We could drop this for Python if we feel like that, I feel.

"""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
2 changes: 1 addition & 1 deletion opentelemetry-api/src/opentelemetry/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@
import typing

AttributeValue = typing.Union[str, bool, float]
Attributes = typing.Dict[str, AttributeValue]
Attributes = typing.Optional[typing.Dict[str, AttributeValue]]
39 changes: 19 additions & 20 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 @@ -16,7 +16,7 @@
import random
import threading
import typing
from collections import OrderedDict, deque, namedtuple
from collections import OrderedDict, deque
from contextlib import contextmanager

from opentelemetry import trace as trace_api
Expand Down Expand Up @@ -140,11 +140,6 @@ def from_map(cls, maxlen, mapping):
return bounded_dict


Event = namedtuple("Event", ("name", "attributes"))

Link = namedtuple("Link", ("context", "attributes"))


class SpanProcessor:
"""Interface which allows hooks for SDK's `Span`s start and end method
invocations.
Expand Down Expand Up @@ -233,16 +228,16 @@ class Span(trace_api.Span):
empty_links = BoundedList(MAX_NUM_LINKS)

def __init__(
self: "Span",
self,
name: str,
context: "trace_api.SpanContext",
parent: trace_api.ParentSpan = None,
sampler=None, # TODO
trace_config=None, # TODO
resource=None, # TODO
attributes: types.Attributes = None, # TODO
events: typing.Sequence[Event] = None, # TODO
links: typing.Sequence[Link] = None, # TODO
events: typing.Sequence[trace_api.Event] = None, # TODO
links: typing.Sequence[trace_api.Link] = None, # TODO
span_processor: SpanProcessor = SpanProcessor(),
) -> None:

Expand Down Expand Up @@ -283,32 +278,36 @@ def __repr__(self):
def get_context(self):
return self.context

def set_attribute(
self: "Span", key: str, value: types.AttributeValue
) -> None:
def set_attribute(self, key: str, value: types.AttributeValue) -> None:
if self.attributes is Span.empty_attributes:
self.attributes = BoundedDict(MAX_NUM_ATTRIBUTES)
self.attributes[key] = value

def add_event(
self: "Span", name: str, attributes: types.Attributes = None
self, name: str, attributes: types.Attributes = None
) -> None:
if self.events is Span.empty_events:
self.events = BoundedList(MAX_NUM_EVENTS)
if attributes is None:
attributes = Span.empty_attributes
self.events.append(Event(name, attributes))
self.add_lazy_event(trace_api.Event(name, util.time_ns(), attributes))

def add_lazy_event(self, event: trace_api.Event) -> None:
if self.events is Span.empty_events:
self.events = BoundedList(MAX_NUM_EVENTS)
self.events.append(event)

def add_link(
self: "Span",
self,
link_target_context: "trace_api.SpanContext",
attributes: types.Attributes = None,
) -> None:
if self.links is Span.empty_links:
self.links = BoundedList(MAX_NUM_LINKS)
if attributes is None:
attributes = Span.empty_attributes
self.links.append(Link(link_target_context, attributes))
self.add_lazy_link(trace_api.Link(link_target_context, attributes))

def add_lazy_link(self, link: "trace_api.Link") -> None:
if self.links is Span.empty_links:
self.links = BoundedList(MAX_NUM_LINKS)
self.links.append(link)

def start(self):
if self.start_time is None:
Expand Down
58 changes: 39 additions & 19 deletions opentelemetry-sdk/tests/trace/test_trace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

from opentelemetry import trace as trace_api
from opentelemetry.sdk import trace
from opentelemetry.sdk import util


class TestTracer(unittest.TestCase):
Expand Down Expand Up @@ -126,10 +127,15 @@ def test_span_members(self):
trace_id=trace.generate_trace_id(),
span_id=trace.generate_span_id(),
)
other_context3 = trace_api.SpanContext(
trace_id=trace.generate_trace_id(),
span_id=trace.generate_span_id(),
)

self.assertIsNone(tracer.get_current_span())

with tracer.start_span("root") as root:
# attributes
root.set_attribute("component", "http")
root.set_attribute("http.method", "GET")
root.set_attribute(
Expand All @@ -144,18 +150,6 @@ def test_span_members(self):
root.set_attribute("attr-key", "attr-value1")
root.set_attribute("attr-key", "attr-value2")

root.add_event("event0")
root.add_event("event1", {"name": "birthday"})

root.add_link(other_context1)
root.add_link(other_context2, {"name": "neighbor"})

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

# The public API does not expose getters.
# Checks by accessing the span members directly

self.assertEqual(len(root.attributes), 7)
self.assertEqual(root.attributes["component"], "http")
self.assertEqual(root.attributes["http.method"], "GET")
Expand All @@ -168,16 +162,34 @@ def test_span_members(self):
self.assertEqual(root.attributes["misc.pi"], 3.14)
self.assertEqual(root.attributes["attr-key"], "attr-value2")

self.assertEqual(len(root.events), 2)
self.assertEqual(
root.events[0], trace.Event(name="event0", attributes={})
# events
root.add_event("event0")
root.add_event("event1", {"name": "birthday"})
now = util.time_ns()
root.add_lazy_event(
trace_api.Event("event2", now, {"name": "hello"})
)
self.assertEqual(
root.events[1],
trace.Event(name="event1", attributes={"name": "birthday"}),

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

self.assertEqual(root.events[0].name, "event0")
self.assertEqual(root.events[0].attributes, {})

self.assertEqual(root.events[1].name, "event1")
self.assertEqual(root.events[1].attributes, {"name": "birthday"})

self.assertEqual(root.events[2].name, "event2")
self.assertEqual(root.events[2].attributes, {"name": "hello"})
self.assertEqual(root.events[2].timestamp, now)

# links
root.add_link(other_context1)
root.add_link(other_context2, {"name": "neighbor"})
root.add_lazy_link(
trace_api.Link(other_context3, {"component": "http"})
)

self.assertEqual(len(root.links), 2)
self.assertEqual(len(root.links), 3)
self.assertEqual(
root.links[0].context.trace_id, other_context1.trace_id
)
Expand All @@ -192,6 +204,14 @@ def test_span_members(self):
root.links[1].context.span_id, other_context2.span_id
)
self.assertEqual(root.links[1].attributes, {"name": "neighbor"})
self.assertEqual(
root.links[2].context.span_id, other_context3.span_id
)
self.assertEqual(root.links[2].attributes, {"component": "http"})

# name
root.update_name("toor")
self.assertEqual(root.name, "toor")


class TestSpan(unittest.TestCase):
Expand Down