Improved Tag Validation & Reporting (#247)

* Use repr instead of str

* Add tests for sets and more json tests

* Use six for type checking

* Use regexp to validate set strings

* Remove redundant escape

Author: pglombardo

instana/span.py

@@ -1,18 +1,11 @@
+import six
 import sys
 from .log import logger
 from .util import DictionaryOfStan
 from basictracer.span import BasicSpan
 import opentracing.ext.tags as ot_tags
 
 
-PY3 = sys.version_info[0] == 3
-
-if PY3:
-    string_type = str
-else:
-    string_type = basestring
-
-
 class SpanContext():
     def __init__(
             self,
@@ -49,22 +42,32 @@ def finish(self, finish_time=None):
         super(InstanaSpan, self).finish(finish_time)
 
     def set_tag(self, key, value):
-        if not isinstance(key, string_type):
+        # Key validation
+        if not isinstance(key, six.text_type) and not isinstance(key, six.string_types) :
             logger.debug("(non-fatal) span.set_tag: tag names must be strings. tag discarded for %s", type(key))
             return self
 
         final_value = value
         value_type = type(value)
-        if value_type not in [bool, float, int, list, str]:
+
+        # Value validation
+        if value_type in [bool, float, int, list, str]:
+            return super(InstanaSpan, self).set_tag(key, final_value)
+
+        elif isinstance(value, six.text_type):
+            final_value = str(value)
+
+        else:
             try:
-                final_value = str(value)
+                final_value = repr(value)
             except:
-                final_value = "(non-fatal) span.set_tag: values must be one of these types: bool, float, int, list or str. tag discarded"
+                final_value = "(non-fatal) span.set_tag: values must be one of these types: bool, float, int, list, " \
+                              "set, str or alternatively support 'repr'. tag discarded"
                 logger.debug(final_value, exc_info=True)
+                return self
 
         return super(InstanaSpan, self).set_tag(key, final_value)
 
-
     def mark_as_errored(self, tags = None):
         """
         Mark this span as errored.

setup.py

@@ -57,6 +57,7 @@ def check_setuptools():
                         'fysom>=2.1.2',
                         'opentracing>=2.0.0',
                         'requests>=2.8.0',
+                        'six>=1.12.0',
                         'urllib3>=1.18.1'],
       entry_points={
                     'instana':  ['string = instana:load'],

tests/opentracing/test_ot_span.py

@@ -1,10 +1,15 @@
+import re
+import sys
+import json
 import time
 import unittest
 import opentracing
 from uuid import UUID
 from instana.util import to_json
 from instana.singletons import tracer
 
+PY2 = sys.version_info[0] == 2
+PY3 = sys.version_info[0] == 3
 
 class TestOTSpan(unittest.TestCase):
     def setUp(self):
@@ -146,7 +151,7 @@ def test_span_kind(self):
         span = spans[4]
         self.assertEqual(3, span.k)
 
-    def test_bad_tag_values(self):
+    def test_tag_values(self):
         with tracer.start_active_span('test') as scope:
             # Set a UUID class as a tag
             # If unchecked, this causes a json.dumps error: "ValueError: Circular reference detected"
@@ -155,33 +160,57 @@ def test_bad_tag_values(self):
             scope.span.set_tag('tracer', tracer)
             scope.span.set_tag('none', None)
             scope.span.set_tag('mylist', [1, 2, 3])
-
+            scope.span.set_tag('myset', {"one", 2})
 
         spans = tracer.recorder.queued_spans()
         assert len(spans) == 1
 
         test_span = spans[0]
         assert(test_span)
-        assert(len(test_span.data['sdk']['custom']['tags']) == 4)
-        assert(test_span.data['sdk']['custom']['tags']['uuid'] == '12345678-1234-5678-1234-567812345678')
+        assert(len(test_span.data['sdk']['custom']['tags']) == 5)
+        assert(test_span.data['sdk']['custom']['tags']['uuid'] == "UUID('12345678-1234-5678-1234-567812345678')")
         assert(test_span.data['sdk']['custom']['tags']['tracer'])
         assert(test_span.data['sdk']['custom']['tags']['none'] == 'None')
         assert(test_span.data['sdk']['custom']['tags']['mylist'] == [1, 2, 3])
-
+        if PY2:
+            set_regexp = re.compile(r"set\(\[.*,.*\]\)")
+            assert(set_regexp.search(test_span.data['sdk']['custom']['tags']['myset']))
+        else:
+            set_regexp = re.compile(r"\{.*,.*\}")
+            assert(set_regexp.search(test_span.data['sdk']['custom']['tags']['myset']))
+
+        # Convert to JSON
         json_data = to_json(test_span)
         assert(json_data)
 
-    def test_bad_tag_names(self):
+        # And back
+        span_dict = json.loads(json_data)
+        assert(len(span_dict['data']['sdk']['custom']['tags']) == 5)
+        assert(span_dict['data']['sdk']['custom']['tags']['uuid'] == "UUID('12345678-1234-5678-1234-567812345678')")
+        assert(span_dict['data']['sdk']['custom']['tags']['tracer'])
+        assert(span_dict['data']['sdk']['custom']['tags']['none'] == 'None')
+        assert(span_dict['data']['sdk']['custom']['tags']['mylist'] == [1, 2, 3])
+        if PY2:
+            set_regexp = re.compile(r"set\(\[.*,.*\]\)")
+            assert(set_regexp.search(test_span.data['sdk']['custom']['tags']['myset']))
+        else:
+            set_regexp = re.compile(r"{.*,.*}")
+            assert(set_regexp.search(test_span.data['sdk']['custom']['tags']['myset']))
+
+    def test_tag_names(self):
         with tracer.start_active_span('test') as scope:
             # Tag names (keys) must be strings
             scope.span.set_tag(1234567890, 'This should not get set')
+            # Unicode key name
+            scope.span.set_tag(u'asdf', 'This should be ok')
 
         spans = tracer.recorder.queued_spans()
         assert len(spans) == 1
 
         test_span = spans[0]
         assert(test_span)
-        assert(len(test_span.data['sdk']['custom']['tags']) == 0)
+        assert(len(test_span.data['sdk']['custom']['tags']) == 1)
+        assert(test_span.data['sdk']['custom']['tags']['asdf'] == 'This should be ok')
 
         json_data = to_json(test_span)
         assert(json_data)