Removing add_link and add_lazy_link from api/sdk #259

codeboten · 2019-11-01T23:25:16Z

Definitely still a work in progress. Removing add_link and add_lazy_link from api/sdk. From the open-telemetry/oteps#28, it appeared the only place where it made sense to add links was in the start_span, create_span and start_as_current_span if we want to be able to add them to the Span before it's constructed. Would love some feedback on this.

Still need to address the other part of the OTEP regarding Attributes

Signed-off-by: Alex Boten [email protected]

Signed-off-by: Alex Boten <[email protected]>

mauriciovasquezbernal · 2019-11-02T03:41:10Z

ext/opentelemetry-ext-jaeger/examples/jaeger_exporter_example.py

@@ -1,6 +1,6 @@
 import time

-from opentelemetry import trace
+from opentelemetry import trace, trace_api


trace_api doesn't exist on opentelemetry, just trace will work fine.

i see, in other places it gets imported as trace_api. Fixed.

Signed-off-by: Alex Boten <[email protected]>

…ation

c24t

LGTM besides the mutable default. This makes the API for span creation a bit uglier, but there's no obvious nicer solution following the OTEP.

c24t · 2019-11-05T02:00:33Z

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

@@ -76,11 +76,15 @@
 class Link:
    """A link to a `Span`."""

+    empty_attributes = {}  # type: typing.Dict[str, types.AttributeValue]


This can't be mutable since we expose it below, changing one (initially empty-attributed) link's attributes would affect others'.

Signed-off-by: Alex Boten <[email protected]>

…ation

Signed-off-by: Alex Boten <[email protected]>

c24t

LGTM!

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

Signed-off-by: Alex Boten <[email protected]>

codecov-io · 2019-11-07T01:08:42Z

Codecov Report

Merging #259 into master will decrease coverage by 0.08%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
- Coverage   85.74%   85.65%   -0.09%     
==========================================
  Files          33       33              
  Lines        1620     1610      -10     
  Branches      183      180       -3     
==========================================
- Hits         1389     1379      -10     
- Misses        184      185       +1     
+ Partials       47       46       -1

Impacted Files	Coverage Δ
...ntelemetry-api/src/opentelemetry/trace/__init__.py	`86.29% <0%> (-1.62%)`	⬇️
...ntelemetry-api/src/opentelemetry/trace/sampling.py	`85.1% <100%> (ø)`	⬆️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	`89.11% <100%> (+0.33%)`	⬆️
...src/opentelemetry/ext/opentracing_shim/__init__.py	`95.96% <100%> (+0.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcdd9fe...4e0c328. Read the comment docs.

Signed-off-by: Alex Boten <[email protected]>

Oberon00 · 2019-11-08T09:14:20Z

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

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


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.

Oberon00 · 2019-11-08T09:18:49Z

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

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


Since start_span uses () as a default instead of None, maybe use () here too? Also we have a Sequence not an Optional[Sequence] anyway.

Oberon00 · 2019-11-08T09:27:37Z

ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py


+        links = None


Suggested change

links = None

links = ()

An empty tuple should be a singleton constant, so no performance overhead over using None here.

- updating comments - updating default value of links for Span constructor Signed-off-by: Alex Boten <[email protected]>

- adding `attributes` as optional parameter to `create_span`, `start_as_current_span`, `start_span` - adding `attributes` as optional parameter to Sampler's `should_sample` to allow them to be considered during sampling decision Signed-off-by: Alex Boten <[email protected]>

…ation Signed-off-by: Alex Boten <[email protected]>

codeboten · 2019-11-12T19:14:33Z

Added attributes as optional parameter to create_span, start_as_current_span, start_span. This should address the second portion of #132

Signed-off-by: Alex Boten <[email protected]>

reyang · 2019-11-12T21:53:36Z

ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py

        if references:
+            links = []


Curious, is there a strong reason to create links as a tuple and then change to list upon modification?

There is not. Cleaned it up

reyang · 2019-11-12T21:57:45Z

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

@@ -416,6 +402,8 @@ def start_span(
        name: str,
        parent: ParentSpan = CURRENT_SPAN,
        kind: SpanKind = SpanKind.INTERNAL,
+        attributes: typing.Optional[types.Attributes] = None,
+        links: typing.Sequence[Link] = (),


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

I think someone should read up on the motivation behind lazy links. I think lazily evaluated iterables may be a good fit for Python.

This is a good point. If we're not using __getitem__ -- and especially if we might not read the sequence at all, -- we should generally prefer Iterator over Sequence.

reyang

LGTM, I've left some non-blocking suggestions.

codeboten · 2019-11-14T19:03:15Z

Closes #132

Signed-off-by: Alex Boten <[email protected]>

c24t

LGTM, but FWIW the API changes only really make sense in the context of the new sampling API. It's not clear why we'd prevent users from adding links after the span is created unless links were involved in the sampling decision, which they're not right now.

* feat: metrics API revision based on specs and rfcs * fix: document default values

Removing add_link and add_lazy_link from api/sdk

ce8b47b

Signed-off-by: Alex Boten <[email protected]>

codeboten requested review from a-feld, c24t, carlosalberto, lzchen, Oberon00, reyang and toumorokoshi as code owners November 1, 2019 23:25

codeboten mentioned this pull request Nov 1, 2019

Apply breaking changes to Span API (from Sampling OTEP 0006) #132

Closed

c24t added the WIP Work in progress label Nov 1, 2019

mauriciovasquezbernal reviewed Nov 2, 2019

View reviewed changes

Alex Boten added 4 commits November 4, 2019 08:11

Fixing an import and typo

7ff2dde

Signed-off-by: Alex Boten <[email protected]>

Fix lint and add annotation

98de769

Signed-off-by: Alex Boten <[email protected]>

More linting fixes

01f2fd3

Signed-off-by: Alex Boten <[email protected]>

Merge remote-tracking branch 'origin/master' into otep-0006-implement…

63d0236

…ation

c24t reviewed Nov 5, 2019

View reviewed changes

Alex Boten added 3 commits November 5, 2019 16:55

Don't need empty_attributes here.

ac9d6f6

Signed-off-by: Alex Boten <[email protected]>

Merge remote-tracking branch 'origin/master' into otep-0006-implement…

2e5c06c

…ation

Don't modify _attributes

f888308

Signed-off-by: Alex Boten <[email protected]>

c24t approved these changes Nov 7, 2019

View reviewed changes

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

Use types.Attributes alias

544be12

Signed-off-by: Alex Boten <[email protected]>

Documenting the links argument

47dfa13

Signed-off-by: Alex Boten <[email protected]>

Oberon00 reviewed Nov 8, 2019

View reviewed changes

Oberon00 approved these changes Nov 8, 2019

View reviewed changes

Alex Boten added 3 commits November 8, 2019 13:33

Changes from review feedback

8d95e12

- updating comments - updating default value of links for Span constructor Signed-off-by: Alex Boten <[email protected]>

Merge remote-tracking branch 'origin/master' into otep-0006-implement…

e136ad2

…ation Signed-off-by: Alex Boten <[email protected]>

codeboten changed the title ~~[WIP] Removing add_link and add_lazy_link from api/sdk~~ Removing add_link and add_lazy_link from api/sdk Nov 12, 2019

codeboten added api Affects the API package. and removed WIP Work in progress labels Nov 12, 2019

Fixing for for python 3.4

d95151d

Signed-off-by: Alex Boten <[email protected]>

reyang reviewed Nov 12, 2019

View reviewed changes

reyang approved these changes Nov 12, 2019

View reviewed changes

clean up

4e0c328

Signed-off-by: Alex Boten <[email protected]>

c24t approved these changes Nov 16, 2019

View reviewed changes

c24t merged commit 30a1bb8 into open-telemetry:master Nov 16, 2019

mauriciovasquezbernal mentioned this pull request Feb 5, 2020

Add Lazy attributes for Events #384

Closed

mauriciovasquezbernal mentioned this pull request Mar 27, 2020

Add support for lazy events and links #474

Merged

codeboten deleted the otep-0006-implementation branch October 9, 2020 18:50

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

feat: Metrics API revision based on Specs and RFCs (open-telemetry#259)

12e0455

* feat: metrics API revision based on specs and rfcs * fix: document default values

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing add_link and add_lazy_link from api/sdk #259

Removing add_link and add_lazy_link from api/sdk #259

codeboten commented Nov 1, 2019

mauriciovasquezbernal Nov 2, 2019

codeboten Nov 4, 2019

c24t left a comment

c24t Nov 5, 2019

c24t left a comment

codecov-io commented Nov 7, 2019 •

edited

Loading

Oberon00 Nov 8, 2019

Oberon00 Nov 8, 2019

Oberon00 Nov 8, 2019

codeboten commented Nov 12, 2019

reyang Nov 12, 2019

codeboten Nov 15, 2019

reyang Nov 12, 2019

Oberon00 Nov 13, 2019

c24t Nov 16, 2019

reyang left a comment

codeboten commented Nov 14, 2019

c24t left a comment

Removing add_link and add_lazy_link from api/sdk #259

Removing add_link and add_lazy_link from api/sdk #259

Conversation

codeboten commented Nov 1, 2019

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

c24t left a comment

Choose a reason for hiding this comment

codecov-io commented Nov 7, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten commented Nov 12, 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

reyang left a comment

Choose a reason for hiding this comment

codeboten commented Nov 14, 2019

c24t left a comment

Choose a reason for hiding this comment

codecov-io commented Nov 7, 2019 •

edited

Loading