Add support for B3 parentspanid by ocelotl · Pull Request #286 · open-telemetry/opentelemetry-python

ocelotl · 2019-11-12T04:19:47Z

Fixes #236

ocelotl · 2019-11-12T04:38:55Z

Hello

I think the CI failure is related to lint, specifically E1137. It kinda looks like a false positive, it has been reported before for circumstances similar to this one:

opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py:106:12: E1137: 'trace_state' does not support item assignment (unsupported-assignment-operation)

Here is the offending line.

Does this looks like a false positive to you too?

ocelotl · 2019-11-12T17:14:59Z

I signed it

Oberon00 · 2019-11-12T17:25:37Z

This is arguably a true positive, since the trace state is supposed to be immutable. Of course it is implemented with a dict so it would work at runtime. EDIT: Not true, a TraceState should be mutable according to our type annotations.

EDIT2: Probably pylint-dev/pylint#2420 We might be able to workaround this by adding dict to the base class list for TraceState.

ocelotl · 2019-11-12T18:26:52Z

Ok! I'll try that, @Oberon00 👍

ocelotl · 2019-11-12T19:14:05Z

When I introduced this change:

diff --git a/opentelemetry-api/src/opentelemetry/trace/__init__.py b/opentelemetry-api/src/opentelemetry/trace/__init__.py
index 412a106..3c430b1 100644
--- a/opentelemetry-api/src/opentelemetry/trace/__init__.py
+++ b/opentelemetry-api/src/opentelemetry/trace/__init__.py
@@ -281,7 +281,9 @@ class TraceOptions(int):
 DEFAULT_TRACE_OPTIONS = TraceOptions.get_default()
 
 
-class TraceState(typing.Dict[str, str]):
+# dict is added here to the base classes of TraceState as a workaround for
+# pylint E1137 which is raised if dict is not present here.
+class TraceState(typing.Dict[str, str], dict):
     """A list of key-value pairs representing vendor-specific trace info.
 
     Keys and values are strings of up to 256 printable US-ASCII characters.

tox -e lint passed , but tox -e py37-mypy didn't:

py37-mypy installed: backcall==0.1.0,decorator==4.4.1,ipdb==0.12.2,ipython==7.9.0,ipython-genutils==0.2.0,jedi==0.15.1,mypy==0.740,mypy-extensions==0.4.3,parso==0.5.1,pexpect==4.7.0,pickleshare==0.7.5,prompt-toolkit==2.0.10,ptyprocess==0.6.0,Pygments==2.4.2,six==1.13.0,traitlets==4.3.3,typed-ast==1.4.0,typing-extensions==3.7.4.1,wcwidth==0.1.7
py37-mypy run-test-pre: PYTHONHASHSEED='1437297989'
py37-mypy run-test-pre: commands[0] | python -m pip install -U pip setuptools wheel
Requirement already up-to-date: pip in ./.tox/py37-mypy/lib/python3.7/site-packages (19.3.1)
Requirement already up-to-date: setuptools in ./.tox/py37-mypy/lib/python3.7/site-packages (41.6.0)
Requirement already up-to-date: wheel in ./.tox/py37-mypy/lib/python3.7/site-packages (0.33.6)
py37-mypy run-test: commands[0] | mypy --namespace-packages opentelemetry-api/src/opentelemetry/
opentelemetry-api/src/opentelemetry/trace/__init__.py:286: error: Duplicate base class "dict"
opentelemetry-api/src/opentelemetry/trace/__init__.py:286: error: Implicit generic "Any". Use "typing.Dict" and specify generic parameters
Found 2 errors in 1 file (checked 23 source files)
ERROR: InvocationError for command /home/ocelotl/repos/lightstep/opentelemetry-python/.tox/py37-mypy/bin/mypy --namespace-packages opentelemetry-api/src/opentelemetry/ (exited with code 2)
__________________________________________________________________________________ summary __________________________________________________________________________________
ERROR:   py37-mypy: commands failed

So, I updated this commit to simply ignore E1137 in that particular line. As reported by @Oberon00 before, this seems to be a Pylint issue and this approach just targets the very specific place where a workaround is needed.

codecov-io · 2019-11-12T19:19:35Z

Codecov Report

Merging #286 into master will increase coverage by 0.09%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   84.82%   84.92%   +0.09%     
==========================================
  Files          38       38              
  Lines        1839     1844       +5     
  Branches      217      218       +1     
==========================================
+ Hits         1560     1566       +6     
+ Misses        214      213       -1     
  Partials       65       65

Impacted Files	Coverage Δ
...pentelemetry/context/propagation/httptextformat.py	`100% <100%> (ø)`	⬆️
...src/opentelemetry/ext/opentracing_shim/__init__.py	`95.93% <100%> (+0.03%)`	⬆️
.../context/propagation/tracecontexthttptextformat.py	`84.72% <100%> (+0.21%)`	⬆️
...opentelemetry/sdk/context/propagation/b3_format.py	`87.27% <83.33%> (+2.65%)`	⬆️

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 a89bbc8...ba20093. Read the comment docs.

toumorokoshi

Sorry for the wild goosechase here. From the discussion in #236, the only new behavior we should handle is extracting the parent span id as ParentSpanId. To do so, this would require passing in spans vs span context to the injector interface, which should be fine.

ocelotl · 2019-12-04T08:08:36Z

Ok, implemented the fix, please let me know if this is the right approach.

It fails tox -e py3X-test-example-app because of this.

How should this be handled? That interface should also receive a Span? I tried making that line inject the span instead of its context but other test cases fail in that case. How should the interface be defined @toumorokoshi ?

toumorokoshi

Good start! I think the API interface change not being propagated through is the major blocker here. Otherwise there's a stylistic choice to spawn a child span to exercise propagation. I generally prefer simple so just use the existing Span, but creating a new one is ok.

toumorokoshi · 2019-12-06T17:25:10Z

opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py

+        set_in_carrier(
+            carrier,
+            cls.PARENT_SPAN_ID_KEY,
+            format_span_id(span.parent.context.span_id),


the parent can be empty, so there should be a conditional here to check that first.

Ok, added the check 👍

toumorokoshi · 2019-12-06T17:27:44Z

opentelemetry-sdk/tests/context/propagation/test_b3_format.py

+            trace.generate_span_id()
+        )

    def test_extract_multi_header(self):


it feels to me that this test was sufficient by itself (since it exercises the base case of serialization / de-serialization), and have a separate test that verifies the parent_id propagation.

This test as is would also have been a good test for an empty parent.

Sorry, I don't understand this comment, can you explain further, please?

Yep definitely. What I was trying to say was: this test already should just work, and actually exercises a valuable case where the span has no parent.

So I was arguing to not modify this test case at all, and instead add another one that verifies that a parent_id field is set if a span has a parent.

Sure, since you already approved these changes, @toumorokoshi, do you prefer if I open separate issues for your pending requests like this one or is if ok for you if I commit them directly in this PR again?

toumorokoshi · 2019-12-06T17:29:01Z

opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py

    @classmethod
-    def inject(cls, context, set_in_carrier, carrier):
-        sampled = (trace.TraceOptions.SAMPLED & context.trace_options) != 0
+    def inject(


this interface change will also need to be propagated up to the API layer as well. There are multiple propagators (tracecontext) that implement the previous API, and those will need to be changed as part of this.

fixed too 👍

toumorokoshi · 2019-12-06T17:31:06Z

opentelemetry-sdk/tests/context/propagation/test_b3_format.py

+        parent_span = trace.Span(
+            "parent", FORMAT.extract(get_as_list, carrier)
+        )
+        child_span = trace.Tracer().start_span("child", parent=parent_span)


I believe you could just instantiate a raw Span object yourself from the SDK, rather than use a whole trace to do so.

Yes, fixed!

toumorokoshi · 2019-12-06T17:33:30Z

opentelemetry-sdk/tests/context/propagation/test_b3_format.py

+            parent_span = trace.Span(
+                "parent", FORMAT.extract(get_as_list, carrier)
+            )
+            child_span = trace.Tracer().start_span("child", parent=parent_span)


there's a lot of boilerplate in these tests now. Could they be refactored into a few test helper methods?

Yes, refactored 👍

toumorokoshi

Looks really good!

I think before merging we should be adding a test case for b3 propagator if a parent does not exist. But everything else looks great. Thanks!

c24t

This looks like a sensible change, but it might be in violation of the spec depending on how you interpret "the value to be injected can be SpanContext or DistributedContext". It might be worth it to check that we're only violating the letter of the law here, and not the spirit.

The discussion in open-telemetry/opentelemetry-specification#359 didn't cover the details of the implementation, I'm interested to see how other languages decide to handle this.

My only blocking comment is that we shouldn't require the SDK in the opentracing shim.

c24t · 2019-12-20T06:59:07Z

opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py

-        sampled = (trace.TraceOptions.SAMPLED & context.trace_options) != 0
+    def inject(
+        cls, span, set_in_carrier, carrier
+    ):  # pylint: disable=arguments-differ


I think you can lose this.

opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py

c24t · 2019-12-20T07:56:06Z

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

+
        propagator.inject(
-            span_context.unwrap(), type(carrier).__setitem__, carrier
+            Span("", span_context.unwrap()), type(carrier).__setitem__, carrier


It seems like a problem to require the opentelemetry SDK in code that only previously required the opentracing API. You could use DefaultSpan here, but it's not a beautiful solution.

It's also pretty surprising to see bare span creation here when we usually create them via the tracer, but I guess that's an unavoidable consequence of the API change.

I changed Span for DefaultSpan. I'm just curious, why is that you don't find this to be beautiful?

Reviewing again, the reason I think this is a kludge is that we're only using the DefaultSpan to smuggle the span context to the propagator. We're taking advantage of the fact that DefaultSpans aren't exported here, but everywhere else they're used interchangeably with regular Spans, with the expectation that the export behavior is left up to the user.

If the signature hadn't changed to require this to be a span, we wouldn't normally create a span here. That looks to me like evidence that we shouldn't change the API in this way.

Fixes open-telemetry#236

…xtformat.py Co-Authored-By: Chris Kleinknecht <libc@google.com>

This is done in order to avoid using the SDK in opentracing_shim

ocelotl · 2020-01-07T19:16:24Z

@c24t yes, I think this is a violation of the spec since inject will receive now a Span. In a previous conversation, @toumorokoshi referred to an upcoming big change in the API (I think #325) that makes this change ok. Please @toumorokoshi, correct me if I am wrong.

c24t

No more complaints from me, but note that we'll have to change the inject signature again for #325.

toumorokoshi

re-approving

Supporting B3's technical definition of a parentspanid, by sourcing the span id of the parent span during injection from the propagator.

* chore: remove all references to SpanData * yarn fix

ocelotl requested review from Oberon00, a-feld, c24t, carlosalberto, lzchen, reyang and toumorokoshi as code owners November 12, 2019 04:19

ocelotl mentioned this pull request Nov 12, 2019

B3 propagation does not handle ParentSpanId #236

Closed

ocelotl force-pushed the issue_236 branch from 0cec30a to e727db1 Compare November 12, 2019 19:11

toumorokoshi suggested changes Dec 1, 2019

View reviewed changes

ocelotl force-pushed the issue_236 branch 2 times, most recently from 2bfe070 to 09f6c35 Compare December 4, 2019 07:58

ocelotl requested a review from toumorokoshi December 4, 2019 20:34

toumorokoshi suggested changes Dec 6, 2019

View reviewed changes

ocelotl force-pushed the issue_236 branch 2 times, most recently from b5e405b to db91e97 Compare December 7, 2019 00:32

a-feld removed their request for review December 10, 2019 00:55

ocelotl force-pushed the issue_236 branch from db91e97 to 6299dcd Compare December 11, 2019 19:43

ocelotl requested a review from a team December 11, 2019 19:43

ocelotl force-pushed the issue_236 branch 2 times, most recently from 7f54d56 to 352de67 Compare December 12, 2019 00:22

ocelotl requested review from toumorokoshi and removed request for a team December 12, 2019 00:23

c24t added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Dec 12, 2019

ocelotl force-pushed the issue_236 branch from 352de67 to cdd49fa Compare December 16, 2019 21:39

toumorokoshi approved these changes Dec 17, 2019

View reviewed changes

c24t reviewed Dec 20, 2019

View reviewed changes

Add support for B3 parentspanid

1d47828

Fixes open-telemetry#236

ocelotl force-pushed the issue_236 branch from cdd49fa to 1d47828 Compare January 7, 2020 18:04

ocelotl and others added 4 commits January 7, 2020 12:09

Update opentelemetry-api/src/opentelemetry/context/propagation/httpte…

433dc66

…xtformat.py Co-Authored-By: Chris Kleinknecht <libc@google.com>

Remove lint workaround

f2e4531

Use DefaultSpan instead of Span

56117c5

This is done in order to avoid using the SDK in opentracing_shim

Lint fixes

ba20093

ocelotl requested review from c24t and toumorokoshi January 8, 2020 16:44

ocelotl added api Affects the API package. sdk Affects the SDK package. shim OpenTracing or OpenCensus compatibility labels Jan 8, 2020

ocelotl self-assigned this Jan 8, 2020

c24t approved these changes Jan 23, 2020

View reviewed changes

toumorokoshi approved these changes Jan 23, 2020

View reviewed changes

toumorokoshi merged commit 7415679 into open-telemetry:master Jan 23, 2020

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

chore: remove all references to SpanData (open-telemetry#286)

d18c844

* chore: remove all references to SpanData * yarn fix

Conversation

ocelotl commented Nov 12, 2019

Uh oh!

ocelotl commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ocelotl commented Nov 12, 2019

Uh oh!

Oberon00 commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ocelotl commented Nov 12, 2019

Uh oh!

ocelotl commented Nov 12, 2019

Uh oh!

codecov-io commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

ocelotl commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocelotl commented Jan 7, 2020

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

ocelotl commented Nov 12, 2019 •

edited

Loading

Oberon00 commented Nov 12, 2019 •

edited

Loading

codecov-io commented Nov 12, 2019 •

edited

Loading

ocelotl commented Dec 4, 2019 •

edited

Loading