Add Link and Event classes #130

mauriciovasquezbernal · 2019-09-09T15:11:52Z

This PR creates classes for Link and Event, also implements the add_lazy_{event,link} methods.

Having a proper class would make it easier to extend in the future. Those classes are implemented directly in the API as they are general enough and different implementations could reuse them.

Solves #76
Solves #77

Put all actions of the same member together so it is easier to update the tests.

Oberon00 · 2019-09-09T15:15:25Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

@@ -70,6 +70,47 @@
 ParentSpan = typing.Optional[typing.Union["Span", "SpanContext"]]


+class Link:


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

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 ;) )

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.

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

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)

@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.

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.

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.

lzchen

LGTM

reyang · 2019-09-09T23:07:00Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

+    """A link to a `Span`."""
+
+    def __init__(
+        self, context: "SpanContext", attributes: types.Attributes = {}


Suggested change

self, context: "SpanContext", attributes: types.Attributes = {}

self, context: "SpanContext", attributes: types.Attributes = None

I used that because mypy was complaining, but then lint complained about this one. I had to change types.Attributes to typing.Optional[typing.Dict[str, AttributeValue]] and it worked.

As to why mypy/lint complain: https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

Make Link a class, also implement addLazyLink

Make Event a class and implement add_lazy_event

reyang · 2019-09-10T01:28:11Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

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


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.

I'd +1 for keeping it in the API.

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

I think it makes sense to have it the API package

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

toumorokoshi

Nice!

toumorokoshi · 2019-09-10T03:38:31Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

+    """A link to a `Span`."""
+
+    def __init__(
+        self, context: "SpanContext", attributes: types.Attributes = None


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

I directly changed the type of types.Attributes to be Attributes = typing.Optional[typing.Dict[str, AttributeValue]]

toumorokoshi · 2019-09-10T03:40:47Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

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


I'd +1 for keeping it in the API.

toumorokoshi · 2019-09-10T03:42:20Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

        `SpanContext` passed as argument.
        """

+    def add_lazy_link(self, link: "Link") -> None:


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]))

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.

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.

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.

c24t

LGTM, but we could benefit from a better policy on which classes get implemented in the API vs. SDK. So far we've only implemented the classes needed to let applications pass along the trace context without taking a dependency on the SDK.

c24t · 2019-09-10T20:26:42Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

+        self._attributes = attributes
+        self._timestamp = timestamp
+
+    @property


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

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

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.

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

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?

* Refactor: rename tracer to node-tracer * style: fix typo

mauriciovasquezbernal added 2 commits September 6, 2019 19:45

remove annotation for self

985b62c

sdk/tests/trace/span: reorganize by member

8a369fd

Put all actions of the same member together so it is easier to update the tests.

mauriciovasquezbernal requested review from a-feld, c24t, carlosalberto, lzchen, Oberon00, reyang and toumorokoshi as code owners September 9, 2019 15:11

mauriciovasquezbernal force-pushed the mauricio/add_event_and_link_classes branch from 368e2cc to 4c0fb58 Compare September 9, 2019 15:19

Oberon00 reviewed Sep 9, 2019

View reviewed changes

lzchen approved these changes Sep 9, 2019

View reviewed changes

reyang reviewed Sep 9, 2019

View reviewed changes

mauriciovasquezbernal added 2 commits September 9, 2019 20:06

add Link class

96b7567

Make Link a class, also implement addLazyLink

add Event class

a339e7d

Make Event a class and implement add_lazy_event

mauriciovasquezbernal force-pushed the mauricio/add_event_and_link_classes branch from 4c0fb58 to a339e7d Compare September 10, 2019 01:06

reyang reviewed Sep 10, 2019

View reviewed changes

toumorokoshi approved these changes Sep 10, 2019

View reviewed changes

c24t approved these changes Sep 10, 2019

View reviewed changes

Oberon00 merged commit 731ed02 into open-telemetry:master Sep 13, 2019

This was referenced Sep 13, 2019

Add Event class. #76

Closed

Add Link class. #77

Closed

Fix lint build after merging #130. #135

Merged

Oberon00 added a commit to dynatrace-oss-contrib/opentelemetry-python that referenced this pull request Sep 13, 2019

Fix lint build after merging open-telemetry#130.

88f2d86

c24t pushed a commit that referenced this pull request Sep 13, 2019

Fix lint build after merging #130 (#135)

876f34c

GoPavel mentioned this pull request Jan 28, 2020

Add Lazy attributes for Events #384

Closed

mauriciovasquezbernal deleted the mauricio/add_event_and_link_classes branch April 14, 2020 21:50

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

Refactor: rename tracer to node-tracer (open-telemetry#130)

af8a7e3

* Refactor: rename tracer to node-tracer * style: fix typo

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Link and Event classes #130

Add Link and Event classes #130

mauriciovasquezbernal commented Sep 9, 2019

Oberon00 Sep 9, 2019

carlosalberto Sep 9, 2019

mauriciovasquezbernal Sep 9, 2019

Oberon00 Sep 10, 2019

carlosalberto Sep 10, 2019

c24t Sep 10, 2019

Oberon00 Sep 13, 2019

c24t Sep 13, 2019

lzchen left a comment

reyang Sep 9, 2019

mauriciovasquezbernal Sep 10, 2019

Oberon00 Sep 10, 2019

reyang Sep 10, 2019

toumorokoshi Sep 10, 2019

mauriciovasquezbernal Sep 10, 2019

carlosalberto Sep 10, 2019

toumorokoshi left a comment

toumorokoshi Sep 10, 2019

mauriciovasquezbernal Sep 10, 2019 •

edited

Loading

toumorokoshi Sep 10, 2019

toumorokoshi Sep 10, 2019

Oberon00 Sep 10, 2019

mauriciovasquezbernal Sep 10, 2019

carlosalberto Sep 10, 2019

c24t left a comment

c24t Sep 10, 2019

mauriciovasquezbernal Sep 10, 2019

c24t Sep 11, 2019 •

edited

Loading

Oberon00 Sep 12, 2019

mauriciovasquezbernal Sep 12, 2019

		@@ -70,6 +70,47 @@
		ParentSpan = typing.Optional[typing.Union["Span", "SpanContext"]]


		class Link:

	self, context: "SpanContext", attributes: types.Attributes = {}
	self, context: "SpanContext", attributes: types.Attributes = None

Add Link and Event classes #130

Add Link and Event classes #130

Conversation

mauriciovasquezbernal commented Sep 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c24t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c24t Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal Sep 10, 2019 •

edited

Loading

c24t Sep 11, 2019 •

edited

Loading