sdk: span parents are now always spancontext #548

toumorokoshi · 2020-04-03T20:57:52Z

Exporter and span handling complexity was increasing due to the
Span.parent attribute being either a Span or a SpanContext.

As there is no requirement in the specification to have the parent
attribute be a Span, removing this complexity and handling.

Exporter and span handling complexity was increasing due to the Span.parent attribute being either a Span or a SpanContext. As there is no requirement in the specification to have the parent attribute be a Span, removing this complexity and handling.

c24t

LGTM. FWIW it looks like we got to the current design because the spec used to require different args for remote (SpanContext) and local (Span) parents: #33 (comment), open-telemetry/opentelemetry-specification#136 (comment).

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

toumorokoshi · 2020-04-04T05:01:26Z

LGTM. FWIW it looks like we got to the current design because the spec used to require different args for remote (SpanContext) and local (Span) parents: #33 (comment), open-telemetry/opentelemetry-specification#136 (comment).

Yeah, that makes sense: there's limited data to construct a full span in the remote scenario.

But just to confirm, there's no rationale for the Span inheriting that complexity, correct? The only caveat I can see here is that exporters will not be able to provide more information around the parent span if it's local, but considering the exporter should have also seen the parent span, I feel like that's not a big issue.

Oberon00 · 2020-04-04T08:58:49Z

The parent usually ends after the child, so that can be a big issue, if the exporter really relied on that feature.
The complexity can easily be reduced by introducing a getter parent_ctx too.

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

codeboten

This is a great cleanup! Just added some questions to make sure I understand what this change is doing. There seems to be some parts of the code that still expect either a Span or a SpanContext, but the change name specifically suggests removing that. Is the expectation that in the case of a remote span, the object will still be a Span and not a SpanContext?

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

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

Oberon00 · 2020-04-09T17:37:15Z

I'll ask again: Over a simple (API-implemented) method like:

def get_parent_context(self):
	if isinstance(self.parent, Span): return self.parent.get_context()
    else: return self.parent

does this add any significant improvements to combat

Exporter and span handling complexity was increasing due to the
Span.parent attribute being either a Span or a SpanContext.

? I feel not. There may still be a merit to this PR but IMHO it needs to be justified better.

Exporter and span handling complexity should decrease just the same, but we still give exporters more info about the parent if it is local (I thin we should also recommend passing a local : As said, exporters will often not have the parents available when they process the child. They may be in different batches, or even different export intervals if the child/parent end long before/after one another. There is no guarantee at all here.

This topic is a bit of a pet peeve for me because of open-telemetry/opentelemetry-specification#525 (comment).

codeboten

Thanks for clarifying my questions.

toumorokoshi · 2020-04-15T05:21:53Z

@Oberon00 regarding your comment about providing a way to extract parent information from the context: I do see the two as providing equal functionality to an exporter. The reason I currently favor this approach is it's still compliant with the current spec as written.

Reading through your links (and thank you for posting them!), I am convinced that your proposal would work fine, and that it would be good to eliminate either Span or SpanContext to do away with this complexity altogether.

Happy to comment and help with moving the spec, but for the time being I feel like this PR is a good way to eliminate some boilerplate complexity that exists now. Thoughts with accepting this as an acceptable current solution?

Oberon00

@toumorokoshi I think the approach of this PR is indeed acceptable, and removing the storage of Span as parent does indeed reduce complexity even more than just providing a getter/property while still storing both. I just wanted to be sure that this is well-considered, which I'm now convinced it is. 😃

ext/opentelemetry-ext-docker-tests/tests/pymongo/test_pymongo_functional.py

ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py

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

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

…ancontext

c24t · 2020-04-24T20:46:49Z

It looks like we might have a flaky integration test after #526?

docker-tests run-test-pre: commands[2] | python check_availability.py
2013: Lost connection to MySQL server during query
Traceback (most recent call last):
  File "check_availability.py", line 65, in check_mysql_connection
    connection = mysql.connector.connect(
  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/docker-tests/lib/python3.8/site-packages/mysql/connector/__init__.py", line 219, in connect
    return MySQLConnection(*args, **kwargs)
  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/docker-tests/lib/python3.8/site-packages/mysql/connector/connection.py", line 104, in __init__
    self.connect(**kwargs)
  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/docker-tests/lib/python3.8/site-packages/mysql/connector/abstracts.py", line 960, in connect
    self._open_connection()
  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/docker-tests/lib/python3.8/site-packages/mysql/connector/connection.py", line 289, in _open_connection
    self._do_handshake()
  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/docker-tests/lib/python3.8/site-packages/mysql/connector/connection.py", line 146, in _do_handshake
    packet = self._socket.recv()
  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/docker-tests/lib/python3.8/site-packages/mysql/connector/network.py", line 267, in recv_plain
    raise errors.InterfaceError(errno=2013)
mysql.connector.errors.InterfaceError: 2013: Lost connection to MySQL server during query
2013: Lost connection to MySQL server during query
Traceback (most recent call last):
  File "check_availability.py", line 65, in check_mysql_connection
    connection = mysql.connector.connect(
  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/docker-tests/lib/python3.8/site-packages/mysql/connector/__init__.py", line 219, in connect
    return MySQLConnection(*args, **kwargs)
  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/docker-tests/lib/python3.8/site-packages/mysql/connector/connection.py", line 104, in __init__
    self.connect(**kwargs)
  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/docker-tests/lib/python3.8/site-packages/mysql/connector/abstracts.py", line 960, in connect
    self._open_connection()
  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/docker-tests/lib/python3.8/site-packages/mysql/connector/connection.py", line 289, in _open_connection
    self._do_handshake()
  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/docker-tests/lib/python3.8/site-packages/mysql/connector/connection.py", line 146, in _do_handshake
    packet = self._socket.recv()
  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/docker-tests/lib/python3.8/site-packages/mysql/connector/network.py", line 267, in recv_plain
    raise errors.InterfaceError(errno=2013)
mysql.connector.errors.InterfaceError: 2013: Lost connection to MySQL server during query

codeboten · 2020-04-24T21:02:44Z

Those errors are showing up during the startup check of mysql. I made a change to check_availability.py as part of the redis PR to clean up those error messages.

The failing tests should have been fixed in this change: 0740696

c24t · 2020-04-24T23:08:29Z

But 0740696 is in this branch already?

codeboten · 2020-04-24T23:28:29Z

Ah i see, the pymysql tests have the same tests I fixed in the other sql related tests in that change.

toumorokoshi · 2020-04-25T04:27:04Z

Thanks @codeboten for fixing the branch!

Exporter and span handling complexity was increasing due to the Span.parent attribute being either a Span or a SpanContext. As there is no requirement in the specification to have the parent attribute be a Span, removing this complexity and handling. Co-authored-by: Chris Kleinknecht <[email protected]> Co-authored-by: Alex Boten <[email protected]>

toumorokoshi requested a review from a team April 3, 2020 20:57

Merge branch 'master' into feature/parent-spancontext

dc434a5

c24t approved these changes Apr 4, 2020

View reviewed changes

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

addressing feedback (clarifying docstring)

71684e8

mauriciovasquezbernal reviewed Apr 6, 2020

View reviewed changes

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

codeboten suggested changes Apr 9, 2020

View reviewed changes

lzchen reviewed Apr 9, 2020

View reviewed changes

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

codeboten approved these changes Apr 14, 2020

View reviewed changes

Oberon00 approved these changes Apr 15, 2020

View reviewed changes

mauriciovasquezbernal mentioned this pull request Apr 15, 2020

Instrumenting Redis open-telemetry/opentelemetry-python-contrib#21

Closed

toumorokoshi and others added 4 commits April 16, 2020 21:26

Addressing feedback

6ddd9b0

Merge remote-tracking branch 'upstream/master' into feature/parent-sp…

269156e

…ancontext

fix tests

0740696

Merge branch 'master' into feature/parent-spancontext

dcdc688

fix pymysql tests

959730d

toumorokoshi merged commit 5f188f7 into open-telemetry:master Apr 25, 2020

codeboten mentioned this pull request Apr 27, 2020

fix: b3 propagation needs to check if parent is SpanContext #621

Merged

mauriciovasquezbernal mentioned this pull request May 5, 2020

Span parent contexts should always be a Span or SpanContext #345

Closed

c24t mentioned this pull request May 21, 2020

Cloud Trace exporter #698

Merged

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

fix(plugin-http): adapt to current @types/node (open-telemetry#548)

b57951c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk: span parents are now always spancontext #548

sdk: span parents are now always spancontext #548

toumorokoshi commented Apr 3, 2020

c24t left a comment

toumorokoshi commented Apr 4, 2020 •

edited

Loading

Oberon00 commented Apr 4, 2020

codeboten left a comment

Oberon00 commented Apr 9, 2020 •

edited

Loading

codeboten left a comment

toumorokoshi commented Apr 15, 2020

Oberon00 left a comment •

edited

Loading

c24t commented Apr 24, 2020

codeboten commented Apr 24, 2020

c24t commented Apr 24, 2020

codeboten commented Apr 24, 2020

toumorokoshi commented Apr 25, 2020

sdk: span parents are now always spancontext #548

sdk: span parents are now always spancontext #548

Conversation

toumorokoshi commented Apr 3, 2020

c24t left a comment

Choose a reason for hiding this comment

toumorokoshi commented Apr 4, 2020 • edited Loading

Oberon00 commented Apr 4, 2020

codeboten left a comment

Choose a reason for hiding this comment

Oberon00 commented Apr 9, 2020 • edited Loading

codeboten left a comment

Choose a reason for hiding this comment

toumorokoshi commented Apr 15, 2020

Oberon00 left a comment • edited Loading

Choose a reason for hiding this comment

c24t commented Apr 24, 2020

codeboten commented Apr 24, 2020

c24t commented Apr 24, 2020

codeboten commented Apr 24, 2020

toumorokoshi commented Apr 25, 2020

toumorokoshi commented Apr 4, 2020 •

edited

Loading

Oberon00 commented Apr 9, 2020 •

edited

Loading

Oberon00 left a comment •

edited

Loading