Add support for lazy events and links #474

mauriciovasquezbernal · 2020-03-10T22:48:43Z

This PR fix the support for lazy events and links.

The idea behind this PR is to have two versions of each class, Link and LazyLink.
Link is constructed using the already calculated attributes, while LazyLink receives an link_formatter function that returns the attributes when invoked.

This PR also moves the class Event definition to the SDK because after the changes to add_lazy_event it is not used anywhere on the API.

Fixes #384.

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

The computation of attributes is expensive in some conditions, even in some cases those attributes are not used at all. This commit fixes the add_lazy_event method by providing a true solution that delays the attributes calculation until attributes are accessed. It also provides a new LazyLink class.

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

lzchen · 2020-03-26T16:44:20Z

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

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


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.

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.

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

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

Co-Authored-By: Leighton Chen <[email protected]>

c24t

A few comments, but it largely LGTM.

I was surprised to see the lazy classes don't eventually return Events and Links, but are instead copies of those classes that do lazy attribute creation. It's consistent with the spec, but I'm worried it complicates the API without much benefit.

And it's a topic for another issue, but I think we should be distinguish between abstract classes that are meant to be implemented as part of the SDK and those that are just there to prevent accidentally instantiating a base class. Having mixed base/abstract classes in the API muddies this.

c24t · 2020-03-27T20:00:24Z

opentelemetry-api/src/opentelemetry/util/types.py

@@ -26,3 +26,4 @@
    Sequence[float],
 ]
 Attributes = Optional[Dict[str, AttributeValue]]
+AttributesFormatter = Callable[[], Optional[Dict[str, AttributeValue]]]


Suggested change

AttributesFormatter = Callable[[], Optional[Dict[str, AttributeValue]]]

AttributesFormatter = Callable[[], Attributes]

Or am I missing something about these types?

c24t · 2020-03-27T20:15:30Z

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

+        else:
+            self._timestamp = timestamp
+
+    @property


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.

c24t · 2020-03-27T20:16:03Z

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

-    def timestamp(self) -> int:
-        return self._timestamp
+    def attributes(self) -> types.Attributes:
+        return self._link_formatter()


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.

c24t · 2020-03-27T20:18:42Z

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

+    def __init__(
+        self,
+        context: "SpanContext",
+        link_formatter: types.AttributesFormatter,


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.

c24t · 2020-03-27T20:23:17Z

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

+            Event(
+                name=name,
+                attributes=attributes,
+                timestamp=time_ns() if timestamp is None else timestamp,


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

toumorokoshi

I think the code looks functionally correct. And I may change to an approve based on the fact that this is technically following the spec.

But I think Event (or the ABC) should still be in the API. I feel like it's an oversight that the spec calls out an Event adder interface and even the fields of such an object, but falls short of the API actually requiring it.

If the Event ABC can be moved back to the API for now, I'll switch to approved.

Also an aside and not a blocker, but "Formatter" feels weird to me. So two related issues in spec:

open-telemetry/opentelemetry-specification#532
open-telemetry/opentelemetry-specification#533

toumorokoshi

Whelp, I've been schooled :) open-telemetry/opentelemetry-specification#532. Sorry to hold you up, I think, at the moment, moving events to the SDK is an appropriate one.

mauriciovasquezbernal · 2020-03-30T15:31:00Z

This PR didn't aim to solve all the issues with lazy events & links, I wanted to fix the support for those events that was badly broken.

I see that there are still discussions about whether lazy events and links are needed or not, and also how to implement it, it's not even clear for me if other SIGs had implemented it or not.

Probably it's something we'd like to keep an eye on, I think it's likely that we are going to change it in next releases.

@todo

* feat(metrics): add registerMetric and getMetric functionality * fix: getTimeSeries and add more tests * fix: minor * fix: add JSDoc * fix: minor * fix: remove * fix: replace String -> string * fix: avoid casting * fix: use GAUGE_DOUBLE and COUNTER_DOUBLE type * fix: add ValueType to indicate type of the metric * fix: move ValueType.DOUBLE to DEFAULT_METRIC_OPTIONS * fix: use Number.isInteger * fix: log an error instead of throw error * fix: add more test and @todo comment * fix: link open-telemetry#474 isssue

mauriciovasquezbernal added the tracing label Mar 10, 2020

mauriciovasquezbernal requested a review from a team March 10, 2020 22:48

ocelotl suggested changes Mar 14, 2020

View reviewed changes

opentelemetry-api/src/opentelemetry/trace/__init__.py Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/implement-lazy-events-and-links branch from ab3515d to 17aad29 Compare March 18, 2020 13:35

codeboten added this to the 3/31 Beta milestone Mar 19, 2020

mauriciovasquezbernal added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Mar 19, 2020

lzchen reviewed Mar 26, 2020

View reviewed changes

opentelemetry-api/src/opentelemetry/trace/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed Mar 26, 2020

View reviewed changes

opentelemetry-api/src/opentelemetry/trace/__init__.py Show resolved Hide resolved

lzchen reviewed Mar 26, 2020

View reviewed changes

mauriciovasquezbernal and others added 2 commits March 27, 2020 07:57

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

fbd7162

Co-Authored-By: Leighton Chen <[email protected]>

Merge branch 'master' into mauricio/implement-lazy-events-and-links

21ba2a9

mauriciovasquezbernal force-pushed the mauricio/implement-lazy-events-and-links branch from 149dd66 to 21ba2a9 Compare March 27, 2020 13:10

c24t approved these changes Mar 27, 2020

View reviewed changes

lzchen approved these changes Mar 27, 2020

View reviewed changes

toumorokoshi suggested changes Mar 29, 2020

View reviewed changes

toumorokoshi approved these changes Mar 30, 2020

View reviewed changes

ocelotl approved these changes Mar 30, 2020

View reviewed changes

toumorokoshi merged commit 7b1d866 into open-telemetry:master Mar 30, 2020

toumorokoshi mentioned this pull request Mar 30, 2020

docs: updating changelogs for 0.6 release #533

Merged

mauriciovasquezbernal deleted the mauricio/implement-lazy-events-and-links branch April 14, 2020 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for lazy events and links #474

Add support for lazy events and links #474

mauriciovasquezbernal commented Mar 10, 2020 •

edited

Loading

lzchen Mar 26, 2020 •

edited

Loading

mauriciovasquezbernal Mar 27, 2020

lzchen Mar 27, 2020

mauriciovasquezbernal Mar 30, 2020

c24t left a comment

c24t Mar 27, 2020

c24t Mar 27, 2020

c24t Mar 27, 2020

c24t Mar 27, 2020

c24t Mar 27, 2020

toumorokoshi left a comment •

edited

Loading

toumorokoshi left a comment

mauriciovasquezbernal commented Mar 30, 2020

	AttributesFormatter = Callable[[], Optional[Dict[str, AttributeValue]]]
	AttributesFormatter = Callable[[], Attributes]

Add support for lazy events and links #474

Add support for lazy events and links #474

Conversation

mauriciovasquezbernal commented Mar 10, 2020 • edited Loading

lzchen Mar 26, 2020 • 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

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

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 • edited Loading

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal commented Mar 30, 2020

mauriciovasquezbernal commented Mar 10, 2020 •

edited

Loading

lzchen Mar 26, 2020 •

edited

Loading

toumorokoshi left a comment •

edited

Loading