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

Added set_status to span, updated pymongo integration #738

Merged
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
([#696](https://github.com/census-instrumentation/opencensus-python/pull/696))
- Fix cloud format propagator to use decimal span_id encoding instead of hex
([#719](https://github.com/census-instrumentation/opencensus-python/pull/719))
- Added `set_status` to `span`
([#738](https://github.com/census-instrumentation/opencensus-python/pull/738))


## 0.6.0
Expand Down
12 changes: 8 additions & 4 deletions contrib/opencensus-ext-flask/tests/test_flask_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -316,8 +316,10 @@ def test_teardown_include_exception(self):
exported_spandata = mock_exporter.export.call_args[0][0][0]
self.assertIsInstance(exported_spandata, span_data.SpanData)
self.assertIsInstance(exported_spandata.status, status.Status)
self.assertEqual(exported_spandata.status.code, code_pb2.UNKNOWN)
self.assertEqual(exported_spandata.status.message, 'error')
self.assertEqual(
exported_spandata.status.canonical_code, code_pb2.UNKNOWN
)
self.assertEqual(exported_spandata.status.description, 'error')

def test_teardown_include_exception_and_traceback(self):
mock_exporter = mock.MagicMock()
Expand All @@ -331,8 +333,10 @@ def test_teardown_include_exception_and_traceback(self):
exported_spandata = mock_exporter.export.call_args[0][0][0]
self.assertIsInstance(exported_spandata, span_data.SpanData)
self.assertIsInstance(exported_spandata.status, status.Status)
self.assertEqual(exported_spandata.status.code, code_pb2.UNKNOWN)
self.assertEqual(exported_spandata.status.message, 'error')
self.assertEqual(
exported_spandata.status.canonical_code, code_pb2.UNKNOWN
)
self.assertEqual(exported_spandata.status.description, 'error')
self.assertIsInstance(
exported_spandata.stack_trace, stack_trace.StackTrace
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,10 @@ def test_intercept_handler_exception(self):

# check that the status obj is attached to the current span
self.assertIsNotNone(current_span.status)
self.assertEqual(current_span.status.code, code_pb2.UNKNOWN)
self.assertEqual(current_span.status.message, 'Test')
self.assertEqual(
current_span.status.canonical_code, code_pb2.UNKNOWN
)
self.assertEqual(current_span.status.description, 'Test')

@mock.patch(
'opencensus.trace.execution_context.get_opencensus_tracer')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,12 +184,12 @@ def translate_to_jaeger(self, span_datas):
tags.append(jaeger.Tag(
key='status.code',
vType=jaeger.TagType.LONG,
vLong=status.code))
vLong=status.canonical_code))

tags.append(jaeger.Tag(
key='status.message',
vType=jaeger.TagType.STRING,
vStr=status.message))
vStr=status.description))

refs = _extract_refs_from_span(span)
logs = _extract_logs_from_span(span)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ def translate_to_trace_proto(span_data):
span_data.start_time),
end_time=ocagent_utils.proto_ts_from_datetime_str(span_data.end_time),
status=trace_pb2.Status(
code=span_data.status.code,
message=span_data.status.message)
code=span_data.status.canonical_code,
message=span_data.status.description,
)
if span_data.status is not None else None,
same_process_as_parent_span=BoolValue(
value=span_data.same_process_as_parent_span)
Expand Down
2 changes: 2 additions & 0 deletions contrib/opencensus-ext-pymongo/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# Changelog

## Unreleased
- Changed attributes names to match [specs](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md)
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't link to the OT spec here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have slightly different opinion on this.
I agree that OpenCensus Python should follow OpenCensus spec. Following OpenTelemetry spec is not the goal here, but it is nice to have if:

  1. It doesn't go against OpenCensus spec.
  2. It doesn't introduce major breaking changes (given we're moving to OpenTelemetry soon) without a good rationale.
  3. It helps us to form better understanding on our path to OpenTelemetry, or simply make the job easier when we try to steal some code and morph them into OpenTelemetry.

With these, I think having a link to OT spec is similar like saying "we are supporting W3C CorrelationContext version 2" - not something required in OC spec, but a nice to have feature.

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 with all that, I'm just concerned about putting a link to the OT spec in something as public as the release notes. There's already a lot of confusion around OC/OT, linking the OT spec here might invite more.

([#738](https://github.com/census-instrumentation/opencensus-python/pull/738))

## 0.1.3
Released 2019-05-31
Expand Down
45 changes: 32 additions & 13 deletions contrib/opencensus-ext-pymongo/opencensus/ext/pymongo/trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,11 @@

from pymongo import monitoring

from google.rpc import code_pb2

from opencensus.trace import execution_context
from opencensus.trace import span as span_module
from opencensus.trace import status as status_module


log = logging.getLogger(__name__)
Expand All @@ -34,7 +37,6 @@ def trace_integration(tracer=None):


class MongoCommandListener(monitoring.CommandListener):

def __init__(self, tracer=None):
self._tracer = tracer

Expand All @@ -44,30 +46,47 @@ def tracer(self):

def started(self, event):
span = self.tracer.start_span(
name='{}.{}.{}.{}'.format(MODULE_NAME,
event.database_name,
event.command.get(event.command_name),
event.command_name))
name='{}.{}.{}.{}'.format(
MODULE_NAME,
event.database_name,
event.command.get(event.command_name),
event.command_name,
)
)
span.span_kind = span_module.SpanKind.CLIENT

self.tracer.add_attribute_to_current_span('component', 'mongodb')
self.tracer.add_attribute_to_current_span('db.type', 'mongodb')
self.tracer.add_attribute_to_current_span(
'db.instance', event.database_name
)
self.tracer.add_attribute_to_current_span(
'db.statement', event.command.get(event.command_name)
victoraugustolls marked this conversation as resolved.
Show resolved Hide resolved
)

for attr in COMMAND_ATTRIBUTES:
_attr = event.command.get(attr)
if _attr is not None:
self.tracer.add_attribute_to_current_span(attr, str(_attr))

self.tracer.add_attribute_to_current_span(
'request_id', event.request_id)
'request_id', event.request_id
)

self.tracer.add_attribute_to_current_span(
'connection_id', str(event.connection_id))
'connection_id', str(event.connection_id)
)

def succeeded(self, event):
self._stop('succeeded')
self._stop(code_pb2.OK)

def failed(self, event):
self._stop('failed')

def _stop(self, status):
self.tracer.add_attribute_to_current_span('status', status)

self._stop(code_pb2.UNKNOWN, 'MongoDB error', event.failure)

def _stop(self, code, message='', details=None):
span = self.tracer.current_span()
status = status_module.Status(
code=code, message=message, details=details
)
span.set_status(status)
self.tracer.end_span()
52 changes: 39 additions & 13 deletions contrib/opencensus-ext-pymongo/tests/test_pymongo_trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ def test_started(self):
}

expected_attrs = {
'component': 'mongodb',
'db.type': 'mongodb',
'db.instance': 'database_name',
'db.statement': 'find',
'filter': 'filter',
'sort': 'sort',
'limit': 'limit',
Expand All @@ -63,8 +67,8 @@ def test_started(self):
trace.MongoCommandListener().started(
event=MockEvent(command_attrs))

self.assertEqual(mock_tracer.current_span.attributes, expected_attrs)
self.assertEqual(mock_tracer.current_span.name, expected_name)
self.assertEqual(mock_tracer.span.attributes, expected_attrs)
self.assertEqual(mock_tracer.span.name, expected_name)

def test_succeed(self):
mock_tracer = MockTracer()
Expand All @@ -74,12 +78,16 @@ def test_succeed(self):
'opencensus.trace.execution_context.get_opencensus_tracer',
return_value=mock_tracer)

expected_attrs = {'status': 'succeeded'}
expected_status = {
'code': 0,
'message': '',
'details': None
}

with patch:
trace.MongoCommandListener().succeeded(event=MockEvent(None))

self.assertEqual(mock_tracer.current_span.attributes, expected_attrs)
self.assertEqual(mock_tracer.span.status, expected_status)
mock_tracer.end_span.assert_called_with()

def test_failed(self):
Expand All @@ -90,12 +98,16 @@ def test_failed(self):
'opencensus.trace.execution_context.get_opencensus_tracer',
return_value=mock_tracer)

expected_attrs = {'status': 'failed'}
expected_status = {
'code': 2,
'message': 'MongoDB error',
'details': 'failure'
}

with patch:
trace.MongoCommandListener().failed(event=MockEvent(None))

self.assertEqual(mock_tracer.current_span.attributes, expected_attrs)
self.assertEqual(mock_tracer.span.status, expected_status)
mock_tracer.end_span.assert_called_with()


Expand All @@ -115,17 +127,31 @@ def __getattr__(self, item):
return item


class MockSpan(object):
def __init__(self):
self.status = None

def set_status(self, status):
self.status = {
'code': status.canonical_code,
'message': status.description,
'details': status.details,
}


class MockTracer(object):
def __init__(self):
self.current_span = None
self.span = MockSpan()
self.end_span = mock.Mock()

def start_span(self, name=None):
span = mock.Mock()
span.name = name
span.attributes = {}
self.current_span = span
return span
self.span.name = name
self.span.attributes = {}
self.span.status = {}
return self.span

def add_attribute_to_current_span(self, key, value):
self.current_span.attributes[key] = value
self.span.attributes[key] = value

def current_span(self):
return self.span
8 changes: 8 additions & 0 deletions opencensus/trace/base_span.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ def add_link(self, link):
"""
raise NotImplementedError

def set_status(self, status):
"""Sets span status.

:type code: :class: `~opencensus.trace.status.Status`
:param code: A Status object.
"""
raise NotImplementedError

def start(self):
"""Set the start time for a span."""
raise NotImplementedError
Expand Down
8 changes: 8 additions & 0 deletions opencensus/trace/blank_span.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,14 @@ def add_link(self, link):
"""
pass

def set_status(self, status):
"""No-op implementation of this method.

:type code: :class: `~opencensus.trace.status.Status`
:param code: A Status object.
"""
pass

def start(self):
"""No-op implementation of this method."""
pass
Expand Down
18 changes: 17 additions & 1 deletion opencensus/trace/span.py
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,13 @@ def __init__(
else:
self.links = BoundedList.from_seq(MAX_NUM_LINKS, links)

if status is None:
self.status = status_module.Status.as_ok()
else:
self.status = status

self.span_id = span_id
self.stack_trace = stack_trace
self.status = status
self.same_process_as_parent_span = same_process_as_parent_span
self._child_spans = []
self.context_tracer = context_tracer
Expand Down Expand Up @@ -346,6 +350,18 @@ def add_link(self, link):
raise TypeError("Type Error: received {}, but requires Link.".
format(type(link).__name__))

def set_status(self, status):
"""Sets span status.

:type code: :class: `~opencensus.trace.status.Status`
:param code: A Status object.
"""
if isinstance(status, status_module.Status):
self.status = status
else:
raise TypeError("Type Error: received {}, but requires Status.".
format(type(status).__name__))

def start(self):
"""Set the start time for a span."""
self.start_time = utils.to_iso_str()
Expand Down
30 changes: 25 additions & 5 deletions opencensus/trace/status.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,31 @@ class Status(object):
See: https://cloud.google.com/trace/docs/reference/v2/
rest/v2/Status#FIELDS.details
"""
def __init__(self, code, message, details=None):
self.code = code
self.message = message
def __init__(self, code, message=None, details=None):
self._code = code
self._message = message
Copy link
Member

Choose a reason for hiding this comment

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

The changes to add set_status look good, but this looks like a breaking change for the sake of matching the names in a different spec.

self.details = details

@property
def canonical_code(self):
return self._code

@property
def description(self):
return self._message

@property
def is_ok(self):
return self._code == code_pb2.OK

def format_status_json(self):
"""Convert a Status object to json format."""
status_json = {}

status_json['code'] = self.code
status_json['message'] = self.message
status_json['code'] = self._code

if self._message is not None:
status_json['message'] = self._message

if self.details is not None:
status_json['details'] = self.details
Expand All @@ -62,3 +76,9 @@ def from_exception(cls, exc):
code=code_pb2.UNKNOWN,
message=str(exc)
)

@classmethod
def as_ok(cls):
return cls(
code=code_pb2.OK,
)
6 changes: 6 additions & 0 deletions tests/unit/trace/test_base_span.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,12 @@ def test_add_link_abstract(self):
with self.assertRaises(NotImplementedError):
span.add_link(None)

def test_set_status_abstract(self):
span = BaseSpan()

with self.assertRaises(NotImplementedError):
span.set_status(None)

def test_iter_abstract(self):
span = BaseSpan()

Expand Down
Loading