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.

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

Protect access to Span implementation #1188

Merged
merged 3 commits into from
Oct 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion .pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,8 @@ exclude-protected=_asdict,
_fields,
_replace,
_source,
_make
_make,
_Span

# List of valid names for the first argument in a class method.
valid-classmethod-first-arg=cls
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,22 +188,22 @@ def test_translate_to_datadog(self):
instrumentation_info = InstrumentationInfo(__name__, "0")

otel_spans = [
trace.Span(
trace._Span(
name=span_names[0],
context=span_context,
parent=parent_context,
kind=trace_api.SpanKind.CLIENT,
instrumentation_info=instrumentation_info,
resource=Resource({}),
),
trace.Span(
trace._Span(
name=span_names[1],
context=parent_context,
parent=None,
instrumentation_info=instrumentation_info,
resource=resource_without_service,
),
trace.Span(
trace._Span(
name=span_names[2],
context=other_context,
parent=None,
Expand Down Expand Up @@ -289,7 +289,7 @@ def test_export(self):
is_remote=False,
)

test_span = trace.Span("test_span", context=context)
test_span = trace._Span("test_span", context=context)
test_span.start()
test_span.end()

Expand Down Expand Up @@ -492,8 +492,8 @@ def test_origin(self):
),
)

root_span = trace.Span(name="root", context=context, parent=None)
child_span = trace.Span(
root_span = trace._Span(name="root", context=context, parent=None)
child_span = trace._Span(
name="child", context=context, parent=root_span
)
root_span.start()
Expand Down Expand Up @@ -528,7 +528,7 @@ def test_sampling_rate(self):
)
sampler = sampling.TraceIdRatioBased(0.5)

span = trace.Span(
span = trace._Span(
name="sampled", context=context, parent=None, sampler=sampler
)
span.start()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ def test_context_propagation(self):
)
self.assertTrue(parent_context.is_remote)

child = trace.Span(
child = trace._Span(
"child",
trace_api.SpanContext(
parent_context.trace_id,
Expand Down Expand Up @@ -149,7 +149,7 @@ def test_sampling_priority_auto_reject(self):

self.assertEqual(parent_context.trace_flags, constants.AUTO_REJECT)

child = trace.Span(
child = trace._Span(
"child",
trace_api.SpanContext(
parent_context.trace_id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def setUp(self):
is_remote=False,
)

self._test_span = trace.Span("test_span", context=context)
self._test_span = trace._Span("test_span", context=context)
self._test_span.start()
self._test_span.end()

Expand Down Expand Up @@ -187,18 +187,20 @@ def test_translate_to_jaeger(self):
]

otel_spans = [
trace.Span(
trace._Span(
name=span_names[0],
context=span_context,
parent=parent_context,
events=(event,),
links=(link,),
kind=trace_api.SpanKind.CLIENT,
),
trace.Span(
trace._Span(
name=span_names[1], context=parent_context, parent=None
),
trace.Span(name=span_names[2], context=other_context, parent=None),
trace._Span(
name=span_names[2], context=other_context, parent=None
),
]

otel_spans[0].start(start_time=start_times[0])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,21 +120,21 @@ def test_translate_to_collector(self):
link_2 = trace_api.Link(
context=parent_context, attributes=link_attributes
)
span_1 = trace.Span(
span_1 = trace._Span(
name="test1",
context=span_context,
parent=parent_context,
events=(event,),
links=(link_1,),
kind=trace_api.SpanKind.CLIENT,
)
span_2 = trace.Span(
span_2 = trace._Span(
name="test2",
context=parent_context,
parent=None,
kind=trace_api.SpanKind.SERVER,
)
span_3 = trace.Span(
span_3 = trace._Span(
name="test3",
context=other_context,
links=(link_2,),
Expand Down Expand Up @@ -302,7 +302,7 @@ def test_export(self):
trace_flags=TraceFlags(TraceFlags.SAMPLED),
)
otel_spans = [
trace.Span(
trace._Span(
name="test1",
context=span_context,
kind=trace_api.SpanKind.CLIENT,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
from opentelemetry.proto.trace.v1.trace_pb2 import Span as OTLPSpan
from opentelemetry.proto.trace.v1.trace_pb2 import Status
from opentelemetry.sdk.resources import Resource as SDKResource
from opentelemetry.sdk.trace import Span, TracerProvider
from opentelemetry.sdk.trace import TracerProvider, _Span
from opentelemetry.sdk.trace.export import (
SimpleExportSpanProcessor,
SpanExportResult,
Expand Down Expand Up @@ -123,7 +123,7 @@ def setUp(self):

type(event_mock).name = PropertyMock(return_value="a")

self.span = Span(
self.span = _Span(
"a",
context=Mock(
**{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def setUp(self):
is_remote=False,
)

self._test_span = trace.Span("test_span", context=context)
self._test_span = trace._Span("test_span", context=context)
self._test_span.start()
self._test_span.end()

Expand Down Expand Up @@ -154,18 +154,22 @@ def test_export(self):
)

otel_spans = [
trace.Span(
trace._Span(
name=span_names[0],
context=span_context,
parent=parent_context,
events=(event,),
links=(link,),
),
trace.Span(
trace._Span(
name=span_names[1], context=parent_context, parent=None
),
trace.Span(name=span_names[2], context=other_context, parent=None),
trace.Span(name=span_names[3], context=other_context, parent=None),
trace._Span(
name=span_names[2], context=other_context, parent=None
),
trace._Span(
name=span_names[3], context=other_context, parent=None
),
]

otel_spans[0].start(start_time=start_times[0])
Expand Down Expand Up @@ -328,7 +332,7 @@ def test_zero_padding(self):
trace_id, parent_id, is_remote=False
)

otel_span = trace.Span(
otel_span = trace._Span(
name=span_names[0], context=span_context, parent=parent_context,
)

Expand Down Expand Up @@ -387,7 +391,7 @@ def test_max_tag_length(self):
trace_flags=TraceFlags(TraceFlags.SAMPLED),
)

span = trace.Span(name="test-span", context=span_context,)
span = trace._Span(name="test-span", context=span_context,)

span.start()
span.resource = Resource({})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def test_set_attributes_from_context(self):
"routing_key": "celery",
}

span = trace.Span("name", mock.Mock(spec=trace_api.SpanContext))
span = trace._Span("name", mock.Mock(spec=trace_api.SpanContext))
utils.set_attributes_from_context(span, context)

self.assertEqual(
Expand Down Expand Up @@ -78,7 +78,7 @@ def test_set_attributes_from_context_empty_keys(self):
"retries": 0,
}

span = trace.Span("name", mock.Mock(spec=trace_api.SpanContext))
span = trace._Span("name", mock.Mock(spec=trace_api.SpanContext))
utils.set_attributes_from_context(span, context)

self.assertEqual(len(span.attributes), 0)
Expand All @@ -99,7 +99,7 @@ def fn_task():

# propagate and retrieve a Span
task_id = "7c6731af-9533-40c3-83a9-25b58f0d837f"
span = trace.Span("name", mock.Mock(spec=trace_api.SpanContext))
span = trace._Span("name", mock.Mock(spec=trace_api.SpanContext))
utils.attach_span(fn_task, task_id, span)
span_after = utils.retrieve_span(fn_task, task_id)
self.assertIs(span, span_after)
Expand All @@ -112,7 +112,7 @@ def fn_task():

# propagate a Span
task_id = "7c6731af-9533-40c3-83a9-25b58f0d837f"
span = trace.Span("name", mock.Mock(spec=trace_api.SpanContext))
span = trace._Span("name", mock.Mock(spec=trace_api.SpanContext))
utils.attach_span(fn_task, task_id, span)
# delete the Span
utils.detach_span(fn_task, task_id)
Expand Down
2 changes: 2 additions & 0 deletions opentelemetry-sdk/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
([#1153](https://github.com/open-telemetry/opentelemetry-python/pull/1153))
- Renaming metrics Batcher to Processor
([#1203](https://github.com/open-telemetry/opentelemetry-python/pull/1203))
- Protect access to Span implementation
([#1188](https://github.com/open-telemetry/opentelemetry-python/pull/1188))

## Version 0.13b0

Expand Down
14 changes: 13 additions & 1 deletion opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,11 @@ class Span(trace_api.Span):
this `Span`.
"""

def __new__(cls, *args, **kwargs):
if cls is Span:
raise TypeError("Span must be instantiated via a tracer.")
return super().__new__(cls)

def __init__(
self,
name: str,
Expand Down Expand Up @@ -663,6 +668,13 @@ def record_exception(self, exception: Exception) -> None:
)


class _Span(Span):
"""Protected implementation of `opentelemetry.trace.Span`.

This constructor should only be used internally.
"""


class Tracer(trace_api.Tracer):
"""See `opentelemetry.trace.Tracer`.

Expand Down Expand Up @@ -748,7 +760,7 @@ def start_span( # pylint: disable=too-many-locals
# Only record if is_recording() is true
if sampling_result.decision.is_recording():
# pylint:disable=protected-access
span = Span(
span = _Span(
name=name,
context=context,
parent=parent_context,
Expand Down
6 changes: 3 additions & 3 deletions opentelemetry-sdk/tests/trace/export/test_export.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ def test_simple_span_processor_not_sampled(self):


def _create_start_and_end_span(name, span_processor):
span = trace.Span(
span = trace._Span(
name,
trace_api.SpanContext(
0xDEADBEEF,
Expand Down Expand Up @@ -403,7 +403,7 @@ def test_export(self): # pylint: disable=no-self-use

# Mocking stdout interferes with debugging and test reporting, mock on
# the exporter instance instead.
span = trace.Span("span name", trace_api.INVALID_SPAN_CONTEXT)
span = trace._Span("span name", trace_api.INVALID_SPAN_CONTEXT)
with mock.patch.object(exporter, "out") as mock_stdout:
exporter.export([span])
mock_stdout.write.assert_called_once_with(span.to_json() + os.linesep)
Expand All @@ -421,5 +421,5 @@ def formatter(span): # pylint: disable=unused-argument
exporter = export.ConsoleSpanExporter(
out=mock_stdout, formatter=formatter
)
exporter.export([trace.Span("span name", mock.Mock())])
exporter.export([trace._Span("span name", mock.Mock())])
mock_stdout.write.assert_called_once_with(mock_span_str)
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def test_shutdown(self):
self.assertEqual(len(span_list), 3)

def test_return_code(self):
span = trace.Span("name", mock.Mock(spec=trace_api.SpanContext))
span = trace._Span("name", mock.Mock(spec=trace_api.SpanContext))
span_list = (span,)
memory_exporter = InMemorySpanExporter()

Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-sdk/tests/trace/propagation/test_b3_format.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ def get_child_parent_new_carrier(old_carrier):
ctx = FORMAT.extract(get_as_list, old_carrier)
parent_context = trace_api.get_current_span(ctx).get_context()

parent = trace.Span("parent", parent_context)
child = trace.Span(
parent = trace._Span("parent", parent_context)
child = trace._Span(
"child",
trace_api.SpanContext(
parent_context.trace_id,
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-sdk/tests/trace/test_implementation.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ def test_tracer(self):
def test_span(self):
with self.assertRaises(Exception):
# pylint: disable=no-value-for-parameter
span = trace.Span()
span = trace._Span()

span = trace.Span("name", INVALID_SPAN_CONTEXT)
span = trace._Span("name", INVALID_SPAN_CONTEXT)
self.assertEqual(span.get_context(), INVALID_SPAN_CONTEXT)
self.assertIs(span.is_recording(), True)
Loading