OpenTracing: Tag Validation (#261)

* Move SpanContext into own module

* Use isinstance instead of type

* Pylint

* Tag validation

* Change value type check

* Better OT log support

* Remove debug

Author: Peter Giacomo Lombardo

instana/binary_propagator.py

@@ -4,7 +4,7 @@
 
 from .log import logger
 from .util import header_to_id
-from .span import SpanContext
+from .span_context import SpanContext
 
 
 class BinaryPropagator():
@@ -21,15 +21,15 @@ def inject(self, span_context, carrier):
             span_id = str.encode(span_context.span_id)
             level = str.encode("1")
 
-            if type(carrier) is dict or hasattr(carrier, "__dict__"):
+            if isinstance(carrier, dict) or hasattr(carrier, "__dict__"):
                 carrier[self.HEADER_KEY_T] = trace_id
                 carrier[self.HEADER_KEY_S] = span_id
                 carrier[self.HEADER_KEY_L] = level
-            elif type(carrier) is list:
+            elif isinstance(carrier, list):
                 carrier.append((self.HEADER_KEY_T, trace_id))
                 carrier.append((self.HEADER_KEY_S, span_id))
                 carrier.append((self.HEADER_KEY_L, level))
-            elif type(carrier) is tuple:
+            elif isinstance(carrier, tuple):
                 carrier = carrier.__add__(((self.HEADER_KEY_T, trace_id),))
                 carrier = carrier.__add__(((self.HEADER_KEY_S, span_id),))
                 carrier = carrier.__add__(((self.HEADER_KEY_L, level),))
@@ -50,17 +50,17 @@ def extract(self, carrier):  # noqa
         level = None
 
         try:
-            if type(carrier) is dict or hasattr(carrier, "__getitem__"):
+            if isinstance(carrier, dict) or hasattr(carrier, "__getitem__"):
                 dc = carrier
             elif hasattr(carrier, "__dict__"):
                 dc = carrier.__dict__
-            elif type(carrier) is list:
+            elif isinstance(carrier, list):
                 dc = dict(carrier)
             else:
                 raise ot.SpanContextCorruptedException()
 
             for key, value in dc.items():
-                if type(key) is str:
+                if isinstance(key, str):
                     key = str.encode(key)
 
                 if self.HEADER_KEY_T == key:

instana/http_propagator.py

@@ -3,8 +3,8 @@
 import opentracing as ot
 
 from .log import logger
-from .span import SpanContext
 from .util import header_to_id
+from .span_context import SpanContext
 
 # The carrier can be a dict or a list.
 # Using the trace header as an example, it can be in the following forms
@@ -43,11 +43,11 @@ def inject(self, span_context, carrier):
             trace_id = span_context.trace_id
             span_id = span_context.span_id
 
-            if type(carrier) is dict or hasattr(carrier, "__dict__"):
+            if isinstance(carrier, dict) or hasattr(carrier, "__dict__"):
                 carrier[self.HEADER_KEY_T] = trace_id
                 carrier[self.HEADER_KEY_S] = span_id
                 carrier[self.HEADER_KEY_L] = "1"
-            elif type(carrier) is list:
+            elif isinstance(carrier, list):
                 carrier.append((self.HEADER_KEY_T, trace_id))
                 carrier.append((self.HEADER_KEY_S, span_id))
                 carrier.append((self.HEADER_KEY_L, "1"))
@@ -68,11 +68,11 @@ def extract(self, carrier):  # noqa
         synthetic = False
 
         try:
-            if type(carrier) is dict or hasattr(carrier, "__getitem__"):
+            if isinstance(carrier, dict) or hasattr(carrier, "__getitem__"):
                 dc = carrier
             elif hasattr(carrier, "__dict__"):
                 dc = carrier.__dict__
-            elif type(carrier) is list:
+            elif isinstance(carrier, list):
                 dc = dict(carrier)
             else:
                 raise ot.SpanContextCorruptedException()

instana/span.py

@@ -1,75 +1,102 @@
+"""
+This module contains the classes that represents spans.
+
+InstanaSpan - the OpenTracing based span used during tracing
+
+When an InstanaSpan is finished, it is converted into either an SDKSpan
+or RegisteredSpan depending on type.
+
+BaseSpan: Base class containing the commonalities for the two descendants
+  - SDKSpan: Class that represents an SDK type span
+  - RegisteredSpan: Class that represents a Registered type span
+"""
 import six
-import sys
-from .log import logger
-from .util import DictionaryOfStan
+
 from basictracer.span import BasicSpan
 import opentracing.ext.tags as ot_tags
 
-
-class SpanContext():
-    def __init__(
-            self,
-            trace_id=None,
-            span_id=None,
-            baggage=None,
-            sampled=True,
-            level=1,
-            synthetic=False):
-
-        self.level = level
-        self.trace_id = trace_id
-        self.span_id = span_id
-        self.sampled = sampled
-        self.synthetic = synthetic
-        self._baggage = baggage or {}
-
-    @property
-    def baggage(self):
-        return self._baggage
-
-    def with_baggage_item(self, key, value):
-        new_baggage = self._baggage.copy()
-        new_baggage[key] = value
-        return SpanContext(
-            trace_id=self.trace_id,
-            span_id=self.span_id,
-            sampled=self.sampled,
-            baggage=new_baggage)
+from .log import logger
+from .util import DictionaryOfStan
 
 
 class InstanaSpan(BasicSpan):
     stack = None
     synthetic = False
 
-    def finish(self, finish_time=None):
-        super(InstanaSpan, self).finish(finish_time)
+    def __init__(self, tracer, operation_name=None, context=None, parent_id=None, tags=None, start_time=None):
+        # Tag validation
+        filtered_tags = {}
+        if tags is not None:
+            for key in tags.keys():
+                validated_key, validated_value = self._validate_tag(key, tags[key])
+                if validated_key is not None:
+                    filtered_tags[validated_key] = validated_value
+
+        super(InstanaSpan, self).__init__(tracer, operation_name, context, parent_id, filtered_tags, start_time)
+
+    def _validate_tag(self, key, value):
+        """
+        This method will assure that <key> and <value> are valid to set as a tag.
+        If <value> fails the check, an attempt will be made to convert it into
+        something useful.
+
+        On check failure, this method will return None values indicating that the tag is
+        not valid and could not be converted into something useful
+
+        :param key: The tag key
+        :param value: The tag value
+        :return: Tuple (key, value)
+        """
+        validated_key = None
+        validated_value = None
+
+        try:
+            # Tag keys must be some type of text or string type
+            if isinstance(key, (six.text_type, six.string_types)):
+                validated_key = key[0:1024] # Max key length of 1024 characters
+
+                if isinstance(value, (bool, float, int, list, dict, six.text_type, six.string_types)):
+                    validated_value = value
+                else:
+                    validated_value = self._convert_tag_value(value)
+            else:
+                logger.debug("(non-fatal) tag names must be strings. tag discarded for %s", type(key))
+        except Exception:
+            logger.debug("instana.span._validate_tag: ", exc_info=True)
+
+        return (validated_key, validated_value)
+
+    def _convert_tag_value(self, value):
+        final_value = None
+
+        try:
+            final_value = repr(value)
+        except Exception:
+            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 None
+        return final_value
 
     def set_tag(self, key, value):
-        # 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
+        validated_key, validated_value = self._validate_tag(key, value)
 
-        final_value = value
-        value_type = type(value)
+        if validated_key is not None and validated_value is not None:
+            return super(InstanaSpan, self).set_tag(validated_key, validated_value)
 
-        # Value validation
-        if value_type in [bool, float, int, list, str]:
-            return super(InstanaSpan, self).set_tag(key, final_value)
+        return self
 
-        elif isinstance(value, six.text_type):
-            final_value = str(value)
+    def log_kv(self, key_values, timestamp=None):
+        validated_key = None
+        validated_value = None
 
-        else:
-            try:
-                final_value = repr(value)
-            except:
-                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
+        for key in key_values.keys():
+            validated_key, validated_value = self._validate_tag(key, key_values[key])
+
+        if validated_key is not None and validated_value is not None:
+            return super(InstanaSpan, self).log_kv({validated_key: validated_value}, timestamp)
 
-        return super(InstanaSpan, self).set_tag(key, final_value)
+        return self
 
     def mark_as_errored(self, tags = None):
         """
@@ -81,7 +108,7 @@ def mark_as_errored(self, tags = None):
             ec = self.tags.get('ec', 0)
             self.set_tag('ec', ec + 1)
 
-            if tags is not None and type(tags) is dict:
+            if tags is not None and isinstance(tags, dict):
                 for key in tags:
                     self.set_tag(key, tags[key])
         except Exception:
@@ -99,7 +126,7 @@ def assure_errored(self):
         except Exception:
             logger.debug('span.assure_errored', exc_info=True)
 
-    def log_exception(self, e):
+    def log_exception(self, exc):
         """
         Log an exception onto this span.  This will log pertinent info from the exception and
         assure that this span is marked as errored.
@@ -110,12 +137,12 @@ def log_exception(self, e):
             message = ""
             self.mark_as_errored()
 
-            if hasattr(e, '__str__') and len(str(e)) > 0:
-                message = str(e)
-            elif hasattr(e, 'message') and e.message is not None:
-                message = e.message
+            if hasattr(exc, '__str__') and len(str(exc)) > 0:
+                message = str(exc)
+            elif hasattr(exc, 'message') and exc.message is not None:
+                message = exc.message
             else:
-                message = repr(e)
+                message = repr(exc)
 
             if self.operation_name in ['rpc-server', 'rpc-client']:
                 self.set_tag('rpc.error', message)
@@ -133,39 +160,18 @@ def log_exception(self, e):
             logger.debug("span.log_exception", exc_info=True)
             raise
 
-    def collect_logs(self):
-        """
-        Collect up log data and feed it to the Instana brain.
-
-        :param span: The span to search for logs in
-        :return: Logs ready for consumption by the Instana brain.
-        """
-        logs = {}
-        for log in self.logs:
-            ts = int(round(log.timestamp * 1000))
-            if ts not in logs:
-                logs[ts] = {}
-
-            if 'message' in log.key_values:
-                logs[ts]['message'] = log.key_values['message']
-            if 'event' in log.key_values:
-                logs[ts]['event'] = log.key_values['event']
-            if 'parameters' in log.key_values:
-                logs[ts]['parameters'] = log.key_values['parameters']
-
-        return logs
-
 
 class BaseSpan(object):
     sy = None
-    
+
     def __str__(self):
         return "BaseSpan(%s)" % self.__dict__.__str__()
 
     def __repr__(self):
         return self.__dict__.__str__()
 
     def __init__(self, span, source, service_name, **kwargs):
+        # pylint: disable=invalid-name
         self.t = span.context.trace_id
         self.p = span.parent_id
         self.s = span.context.span_id
@@ -189,6 +195,7 @@ class SDKSpan(BaseSpan):
     EXIT_KIND = ["exit", "client", "producer"]
 
     def __init__(self, span, source, service_name, **kwargs):
+        # pylint: disable=invalid-name
         super(SDKSpan, self).__init__(span, source, service_name, **kwargs)
 
         span_kind = self.get_span_kind(span)
@@ -202,13 +209,18 @@ def __init__(self, span, source, service_name, **kwargs):
         self.data["sdk"]["name"] = span.operation_name
         self.data["sdk"]["type"] = span_kind[0]
         self.data["sdk"]["custom"]["tags"] = span.tags
-        self.data["sdk"]["custom"]["logs"] = span.logs
+
+        if span.logs is not None and len(span.logs) > 0:
+            logs = DictionaryOfStan()
+            for log in span.logs:
+                logs[repr(log.timestamp)] = log.key_values
+            self.data["sdk"]["custom"]["logs"] = logs
 
         if "arguments" in span.tags:
-            self.data.sdk.arguments = span.tags["arguments"]
+            self.data['sdk']['arguments'] = span.tags["arguments"]
 
         if "return" in span.tags:
-            self.data.sdk.Return = span.tags["return"]
+            self.data['sdk']['return'] = span.tags["return"]
 
         if len(span.context.baggage) > 0:
             self.data["baggage"] = span.context.baggage
@@ -244,6 +256,7 @@ class RegisteredSpan(BaseSpan):
     LOCAL_SPANS = ("render")
 
     def __init__(self, span, source, service_name, **kwargs):
+        # pylint: disable=invalid-name
         super(RegisteredSpan, self).__init__(span, source, service_name, **kwargs)
         self.n = span.operation_name
 
@@ -263,7 +276,7 @@ def __init__(self, span, source, service_name, **kwargs):
             self.k = 1  # entry
 
         # Store any leftover tags in the custom section
-        if len(span.tags):
+        if len(span.tags) > 0:
             self.data["custom"]["tags"] = span.tags
 
     def _populate_entry_span_data(self, span):