Add indicator if SpanContext was propagated from remote parent #516

mariojonke · 2020-03-20T06:52:53Z

According to the spec a SpanContext should have an indicator if it was propagated from a remote parent.

This PR introduces an is_remote flag on the SpanContext which gets set when the SpanContext is extracted in a propagator.

Oberon00 · 2020-03-20T09:54:33Z

Looks like we have an unrelated flaky test (EDIT: Actually that was pip failing to download something. Maybe we can set it to retry once):

ERROR: Exception:

Traceback (most recent call last):

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_vendor/urllib3/response.py", line 360, in _error_catcher

    yield

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_vendor/urllib3/response.py", line 442, in read

    data = self._fp.read(amt)

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_vendor/cachecontrol/filewrapper.py", line 62, in read

    data = self.__fp.read(amt)

  File "/opt/python/3.4.8/lib/python3.4/http/client.py", line 537, in read

    return super(HTTPResponse, self).read(amt)

  File "/opt/python/3.4.8/lib/python3.4/http/client.py", line 576, in readinto

    n = self.fp.readinto(b)

  File "/opt/python/3.4.8/lib/python3.4/socket.py", line 378, in readinto

    return self._sock.recv_into(b)

  File "/opt/python/3.4.8/lib/python3.4/ssl.py", line 754, in recv_into

    return self.read(nbytes, buffer)

  File "/opt/python/3.4.8/lib/python3.4/ssl.py", line 626, in read

    v = self._sslobj.read(len, buffer)

ConnectionResetError: [Errno 104] Connection reset by peer

During handling of the above exception, another exception occurred:

Traceback (most recent call last):

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/cli/base_command.py", line 178, in main

    status = self.run(options, args)

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/commands/install.py", line 352, in run

    resolver.resolve(requirement_set)

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/resolve.py", line 131, in resolve

    self._resolve_one(requirement_set, req)

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/resolve.py", line 294, in _resolve_one

    abstract_dist = self._get_abstract_dist_for(req_to_install)

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/resolve.py", line 242, in _get_abstract_dist_for

    self.require_hashes

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/operations/prepare.py", line 347, in prepare_linked_requirement

    progress_bar=self.progress_bar

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/download.py", line 886, in unpack_url

    progress_bar=progress_bar

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/download.py", line 746, in unpack_http_url

    progress_bar)

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/download.py", line 954, in _download_http_url

    _download_url(resp, link, content_file, hashes, progress_bar)

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/download.py", line 683, in _download_url

    hashes.check_against_chunks(downloaded_chunks)

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/utils/hashes.py", line 62, in check_against_chunks

    for chunk in chunks:

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/download.py", line 651, in written_chunks

    for chunk in chunks:

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/utils/ui.py", line 156, in iter

    for x in it:

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_internal/download.py", line 640, in resp_read

    decode_content=False):

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_vendor/urllib3/response.py", line 494, in stream

    data = self.read(amt=amt, decode_content=decode_content)

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_vendor/urllib3/response.py", line 459, in read

    raise IncompleteRead(self._fp_bytes_read, self.length_remaining)

  File "/opt/python/3.4.8/lib/python3.4/contextlib.py", line 77, in __exit__

    self.gen.throw(type, value, traceback)

  File "/home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/lib/python3.4/site-packages/pip/_vendor/urllib3/response.py", line 378, in _error_catcher

    raise ProtocolError('Connection broken: %r' % e, e)

pip._vendor.urllib3.exceptions.ProtocolError: ("Connection broken: ConnectionResetError(104, 'Connection reset by peer')", ConnectionResetError(104, 'Connection reset by peer'))

ERROR: InvocationError for command /home/travis/build/open-telemetry/opentelemetry-python/.tox/py34-test-ext-otcollector/bin/pip install /home/travis/build/open-telemetry/opentelemetry-python/ext/opentelemetry-ext-otcollector (exited with code 2)

I re-triggered the build.

mauriciovasquezbernal

Two small -not blocking- nits. LGTM!

mauriciovasquezbernal · 2020-03-20T14:23:01Z

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

@@ -86,10 +86,12 @@ def test_extract_multi_header(self):
            new_carrier[FORMAT.SPAN_ID_KEY],
            b3_format.format_span_id(child.context.span_id),
        )
+        self.assertFalse(child.context.is_remote)


I don't think this check is needed. child is created from a SpanContext extracted from the carrier, so whether is_remote is False or True depends on the Span implementation (that you are already testing in test_span_context_remote_flag) and not on the propagator one.

Same consideration for all the cases where child.is_remote is tested.

That's an interesting nuance. Are we expecting that different Span SDKs will handle this behavior differently? I guess I understood it as even different Span implementations SHOULD still adhere to the span propagation behavior.

Even the link this PR is referring to lives in the "API" section: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#spancontext. So theoretically different implementations should still provide similar behavior.

I agree that a test checking that the is_remote flag is not propagated to children is useful, however this is not the place to such test, that behavior depends on the tracer implementation and not in the propagator one.

mauriciovasquezbernal · 2020-03-20T14:36:09Z

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

@@ -319,6 +319,7 @@ class SpanContext:
        span_id: This span's ID.
        trace_flags: Trace options to propagate.
        trace_state: Tracing-system-specific info to propagate.
+        is_remote: Indicator if propagated from a remote parent


Suggested change

is_remote: Indicator if propagated from a remote parent

is_remote: Indicator if propagated from a remote parent.

Suggested change

is_remote: Indicator if propagated from a remote parent

is_remote: True if propagated from a remote parent.

* remove redundant assertions in tests * adapt doc string for is_remote flag in SpanContext

toumorokoshi · 2020-03-23T16:29:56Z

opentelemetry-sdk/tests/trace/test_trace.py

@@ -378,6 +378,12 @@ def test_default_span_resource(self):
        # pylint: disable=protected-access
        self.assertIs(span.resource, resources._EMPTY_RESOURCE)

+    def test_span_context_remote_flag(self):


maybe add a docstring saying that the expected behavior is is_remote is False by default? I guess it's implicit, I just like having some human description so I can verify the intended behavior.

Actually, I think there should not be a default for is_remote. And if any, the default should be true. This flag might have security implications, e.g. samplers may trust a sampled=True flag on a is_remote=False SpanContext. If the is_remote flag is wrongly False, this might lead to a Denial of Service vulnerability.

I can definitely see that argument, although I'm wondering if denial of service is a concern, there isn't a more robust way of dealing with that. This also assumes that the adapters (include internal/custom ones) also properly use the is_remote flag, which is impossible to enforce.

on the other hand, I feel like is_remote=True as the default is unusual behavior, as the remote span is much more of a special case, and currently users are creating/starting spans with no arguments, which would then make then remote spans by default unless one is aware of that flag, and sets it everywhere.

We may need more opinions here, but I think the default being false ensures minimal friction for usage of the opentelemetry APIs.

Sorry, this was a misunderstanding on my side! 🙈
I thought we were talking about manually creating a SpanContext. For starting a new span, having is_remote=False on its SpanContext is the only sane thing to do (and this should not be configurable).

toumorokoshi · 2020-03-23T16:35:05Z

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

@@ -86,10 +86,12 @@ def test_extract_multi_header(self):
            new_carrier[FORMAT.SPAN_ID_KEY],
            b3_format.format_span_id(child.context.span_id),
        )
+        self.assertFalse(child.context.is_remote)


That's an interesting nuance. Are we expecting that different Span SDKs will handle this behavior differently? I guess I understood it as even different Span implementations SHOULD still adhere to the span propagation behavior.

Even the link this PR is referring to lives in the "API" section: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#spancontext. So theoretically different implementations should still provide similar behavior.

toumorokoshi

Thanks for the hard work! I think there's a little bit of clarification in the Spec I'd like to verify before merging, but I think this does the current intended behavior.

@mauriciovasquezbernal I'd argue the test that was removed should be re-added, but maybe that needs clarification in the spec. issue filed: open-telemetry/opentelemetry-specification#523

codecov-io · 2020-03-24T05:36:24Z

Codecov Report

Merging #516 into master will increase coverage by 0.00%.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##           master     #516   +/-   ##
=======================================
  Coverage   89.47%   89.47%           
=======================================
  Files          43       43           
  Lines        2213     2214    +1     
  Branches      250      250           
=======================================
+ Hits         1980     1981    +1     
  Misses        161      161           
  Partials       72       72

Impacted Files	Coverage Δ
...ry/trace/propagation/tracecontexthttptextformat.py	`84.28% <ø> (ø)`
...opentelemetry/sdk/context/propagation/b3_format.py	`87.27% <ø> (ø)`
...ntelemetry-api/src/opentelemetry/trace/__init__.py	`83.12% <50.00%> (+0.10%)`	⬆️

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 fbfafa4...eb5be02. Read the comment docs.

Oberon00 · 2020-03-25T10:54:29Z

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

        span_id: int,
        trace_flags: "TraceFlags" = DEFAULT_TRACE_OPTIONS,
        trace_state: "TraceState" = DEFAULT_TRACE_STATE,
+        is_remote: bool = False,


I suggest not having a default here, for the reasons outlined in https://github.com/open-telemetry/opentelemetry-python/pull/516/files/9b92b7aa1e08be55d309ff2b0de266df0ee5fb91#r396611236

Suggested change

span_id: int,

trace_flags: "TraceFlags" = DEFAULT_TRACE_OPTIONS,

trace_state: "TraceState" = DEFAULT_TRACE_STATE,

is_remote: bool = False,

span_id: int,

is_remote: bool,

trace_flags: "TraceFlags" = DEFAULT_TRACE_OPTIONS,

trace_state: "TraceState" = DEFAULT_TRACE_STATE,

…telemetry#516) According to the spec a SpanContext should have an indicator if it was propagated from a remote parent. Introduces an is_remote flag on the SpanContext which gets set when the SpanContext is extracted in a propagaton.

* fix: add missing dev dependencies * fix: lint

Add indicator if SpanContext was propagated from remote parent

496283b

mariojonke requested a review from a team March 20, 2020 06:52

mauriciovasquezbernal approved these changes Mar 20, 2020

View reviewed changes

Address review comments

9b92b7a

* remove redundant assertions in tests * adapt doc string for is_remote flag in SpanContext

toumorokoshi reviewed Mar 23, 2020

View reviewed changes

toumorokoshi mentioned this pull request Mar 23, 2020

Document IsRemote flag should not be propagated to children. open-telemetry/opentelemetry-specification#523

Closed

toumorokoshi approved these changes Mar 23, 2020

View reviewed changes

Merge branch 'master' into spancontext-isremote-flag

eb5be02

Merge branch 'master' into spancontext-isremote-flag

1d3cad5

Oberon00 reviewed Mar 25, 2020

View reviewed changes

Make is_remote flag mandatory when creating a SpanContext

5b42197

Oberon00 requested review from toumorokoshi and mauriciovasquezbernal March 26, 2020 09:15

Oberon00 approved these changes Mar 26, 2020

View reviewed changes

Merge branch 'master' into spancontext-isremote-flag

e119212

toumorokoshi merged commit 144ab39 into open-telemetry:master Mar 26, 2020

mariojonke deleted the spancontext-isremote-flag branch March 27, 2020 06:23

toumorokoshi mentioned this pull request Mar 30, 2020

docs: updating changelogs for 0.6 release #533

Merged

lzchen mentioned this pull request Apr 3, 2020

Remove label sets + add is_remote to spancontext microsoft/opentelemetry-azure-monitor-python#75

Merged

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

fix: add missing dev dependencies (open-telemetry#516)

fcb0f1a

* fix: add missing dev dependencies * fix: lint

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add indicator if SpanContext was propagated from remote parent #516

Add indicator if SpanContext was propagated from remote parent #516

mariojonke commented Mar 20, 2020

Oberon00 commented Mar 20, 2020 •

edited

Loading

mauriciovasquezbernal left a comment

mauriciovasquezbernal Mar 20, 2020

toumorokoshi Mar 23, 2020 •

edited

Loading

mauriciovasquezbernal Mar 23, 2020

mauriciovasquezbernal Mar 20, 2020

Oberon00 Mar 20, 2020

toumorokoshi Mar 23, 2020

Oberon00 Mar 23, 2020

toumorokoshi Mar 24, 2020

toumorokoshi Mar 24, 2020

Oberon00 Mar 25, 2020 •

edited

Loading

toumorokoshi Mar 23, 2020 •

edited

Loading

toumorokoshi left a comment

codecov-io commented Mar 24, 2020 •

edited

Loading

Oberon00 Mar 25, 2020

	is_remote: Indicator if propagated from a remote parent
	is_remote: Indicator if propagated from a remote parent.

	is_remote: Indicator if propagated from a remote parent
	is_remote: True if propagated from a remote parent.

Add indicator if SpanContext was propagated from remote parent #516

Add indicator if SpanContext was propagated from remote parent #516

Conversation

mariojonke commented Mar 20, 2020

Oberon00 commented Mar 20, 2020 • edited Loading

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toumorokoshi Mar 23, 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

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

Oberon00 Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

toumorokoshi Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 24, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Oberon00 commented Mar 20, 2020 •

edited

Loading

toumorokoshi Mar 23, 2020 •

edited

Loading

Oberon00 Mar 25, 2020 •

edited

Loading

toumorokoshi Mar 23, 2020 •

edited

Loading

codecov-io commented Mar 24, 2020 •

edited

Loading