Skip to content

Commit

Permalink
Protect access to Span implementation (#1188)
Browse files Browse the repository at this point in the history
  • Loading branch information
ahlaw authored Oct 6, 2020
1 parent 8ccc5cf commit 6fac795
Show file tree
Hide file tree
Showing 15 changed files with 71 additions and 45 deletions.
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

0 comments on commit 6fac795

Please sign in to comment.