Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add indicator if SpanContext was propagated from remote parent #516

Merged
merged 6 commits into from
Mar 26, 2020
Merged

Add indicator if SpanContext was propagated from remote parent #516

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
496283b
Add indicator if SpanContext was propagated from remote parent
mariojonke Mar 19, 2020
9b92b7a
Address review comments
mariojonke Mar 23, 2020
eb5be02
Merge branch 'master' into spancontext-isremote-flag
toumorokoshi Mar 24, 2020
1d3cad5
Merge branch 'master' into spancontext-isremote-flag
toumorokoshi Mar 24, 2020
5b42197
Make is_remote flag mandatory when creating a SpanContext
mariojonke Mar 25, 2020
e119212
Merge branch 'master' into spancontext-isremote-flag
toumorokoshi Mar 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion opentelemetry-api/src/opentelemetry/trace/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""

def __init__(
Expand All @@ -327,6 +328,7 @@ def __init__(
span_id: int,
trace_flags: "TraceFlags" = DEFAULT_TRACE_OPTIONS,
trace_state: "TraceState" = DEFAULT_TRACE_STATE,
is_remote: bool = False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

) -> None:
if trace_flags is None:
trace_flags = DEFAULT_TRACE_OPTIONS
Expand All @@ -336,13 +338,17 @@ def __init__(
self.span_id = span_id
self.trace_flags = trace_flags
self.trace_state = trace_state
self.is_remote = is_remote

def __repr__(self) -> str:
return "{}(trace_id={}, span_id={}, trace_state={!r})".format(
return (
"{}(trace_id={}, span_id={}, trace_state={!r}, is_remote={})"
).format(
type(self).__name__,
format_trace_id(self.trace_id),
format_span_id(self.span_id),
self.trace_state,
self.is_remote,
)

def is_valid(self) -> bool:
Expand Down
1 change: 1 addition & 0 deletions opentelemetry-api/src/opentelemetry/trace/propagation/tracecontexthttptextformat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ def extract(
span_id=int(span_id, 16),
trace_flags=trace.TraceFlags(trace_flags),
trace_state=tracestate,
is_remote=True,
)
return set_span_in_context(trace.DefaultSpan(span_context), context)

Expand Down
1 change: 1 addition & 0 deletions opentelemetry-api/tests/trace/propagation/test_tracecontexthttptextformat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ def test_headers_with_tracestate(self):
self.assertEqual(
span_context.trace_state, {"foo": "1", "bar": "2", "baz": "3"}
)
self.assertTrue(span_context.is_remote)
output = {} # type:typing.Dict[str, str]
span = trace.DefaultSpan(span_context)

Expand Down
1 change: 1 addition & 0 deletions opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ def extract(
span_id=int(span_id, 16),
trace_flags=trace.TraceFlags(options),
trace_state=trace.TraceState(),
is_remote=True,
)
)
)
Expand Down
6 changes: 6 additions & 0 deletions opentelemetry-sdk/tests/context/propagation/test_b3_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@toumorokoshi toumorokoshi Mar 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.assertEqual(
new_carrier[FORMAT.PARENT_SPAN_ID_KEY],
b3_format.format_span_id(parent.context.span_id),
)
self.assertTrue(parent.context.is_remote)
self.assertEqual(new_carrier[FORMAT.SAMPLED_KEY], "1")

def test_extract_single_header(self):
Expand All @@ -111,6 +113,8 @@ def test_extract_single_header(self):
b3_format.format_span_id(child.context.span_id),
)
self.assertEqual(new_carrier[FORMAT.SAMPLED_KEY], "1")
self.assertFalse(child.context.is_remote)
self.assertTrue(parent.context.is_remote)

child, parent, new_carrier = get_child_parent_new_carrier(
{
Expand All @@ -130,10 +134,12 @@ def test_extract_single_header(self):
new_carrier[FORMAT.SPAN_ID_KEY],
b3_format.format_span_id(child.context.span_id),
)
self.assertFalse(child.context.is_remote)
self.assertEqual(
new_carrier[FORMAT.PARENT_SPAN_ID_KEY],
b3_format.format_span_id(parent.context.span_id),
)
self.assertTrue(parent.context.is_remote)
self.assertEqual(new_carrier[FORMAT.SAMPLED_KEY], "1")

def test_extract_header_precedence(self):
Expand Down
6 changes: 6 additions & 0 deletions opentelemetry-sdk/tests/trace/test_trace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Oberon00 Oberon00 Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tracer = new_tracer()

span = tracer.start_span("foo")
self.assertFalse(span.context.is_remote)


class TestSpan(unittest.TestCase):
def setUp(self):
Expand Down