From 4fca8c985769d891822823eb91120ea91aa57acd Mon Sep 17 00:00:00 2001
From: Jake Malachowski <5766239+jakemalachowski@users.noreply.github.com>
Date: Mon, 20 Jan 2020 11:13:53 -0600
Subject: [PATCH] Add runtime validation in setAttribute (#348)

Validate attribute value data types before adding to span
Add lists as an accepted data type.

By adding validation during the setAttribute phase, this
allows allows exporters to avoid redundant code to validate
attributes.
---
 .../src/opentelemetry/sdk/trace/__init__.py   | 32 +++++++++++
 opentelemetry-sdk/tests/trace/test_trace.py   | 53 ++++++++++++++++++-
 2 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
index b969587eeb0..0bae38c8b3c 100644
--- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
+++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
@@ -18,6 +18,7 @@
 import random
 import threading
 from contextlib import contextmanager
+from numbers import Number
 from types import TracebackType
 from typing import Iterator, Optional, Sequence, Tuple, Type
 
@@ -216,8 +217,39 @@ def set_attribute(self, key: str, value: types.AttributeValue) -> None:
         if has_ended:
             logger.warning("Setting attribute on ended span.")
             return
+
+        if isinstance(value, Sequence):
+            error_message = self._check_attribute_value_sequence(value)
+            if error_message is not None:
+                logger.warning("%s in attribute value sequence", error_message)
+                return
+        elif not isinstance(value, (bool, str, Number, Sequence)):
+            logger.warning("invalid type for attribute value")
+            return
+
         self.attributes[key] = value
 
+    @staticmethod
+    def _check_attribute_value_sequence(sequence: Sequence) -> Optional[str]:
+        """
+        Checks if sequence items are valid and are of the same type
+        """
+        if len(sequence) == 0:
+            return None
+
+        first_element_type = type(sequence[0])
+
+        if issubclass(first_element_type, Number):
+            first_element_type = Number
+
+        if first_element_type not in (bool, str, Number):
+            return "invalid type"
+
+        for element in sequence:
+            if not isinstance(element, first_element_type):
+                return "different type"
+        return None
+
     def add_event(
         self,
         name: str,
diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py
index 98a7bb100e7..5f32f775fe9 100644
--- a/opentelemetry-sdk/tests/trace/test_trace.py
+++ b/opentelemetry-sdk/tests/trace/test_trace.py
@@ -368,7 +368,11 @@ def test_attributes(self):
             root.set_attribute("attr-key", "attr-value1")
             root.set_attribute("attr-key", "attr-value2")
 
-            self.assertEqual(len(root.attributes), 7)
+            root.set_attribute("empty-list", [])
+            root.set_attribute("list-of-bools", [True, True, False])
+            root.set_attribute("list-of-numerics", [123, 3.14, 0])
+
+            self.assertEqual(len(root.attributes), 10)
             self.assertEqual(root.attributes["component"], "http")
             self.assertEqual(root.attributes["http.method"], "GET")
             self.assertEqual(
@@ -379,6 +383,13 @@ def test_attributes(self):
             self.assertEqual(root.attributes["http.status_text"], "OK")
             self.assertEqual(root.attributes["misc.pi"], 3.14)
             self.assertEqual(root.attributes["attr-key"], "attr-value2")
+            self.assertEqual(root.attributes["empty-list"], [])
+            self.assertEqual(
+                root.attributes["list-of-bools"], [True, True, False]
+            )
+            self.assertEqual(
+                root.attributes["list-of-numerics"], [123, 3.14, 0]
+            )
 
         attributes = {
             "attr-key": "val",
@@ -393,6 +404,46 @@ def test_attributes(self):
             self.assertEqual(root.attributes["attr-key2"], "val2")
             self.assertEqual(root.attributes["attr-in-both"], "span-attr")
 
+    def test_invalid_attribute_values(self):
+        with self.tracer.start_as_current_span("root") as root:
+            root.set_attribute("non-primitive-data-type", dict())
+            root.set_attribute(
+                "list-of-mixed-data-types-numeric-first",
+                [123, False, "string"],
+            )
+            root.set_attribute(
+                "list-of-mixed-data-types-non-numeric-first",
+                [False, 123, "string"],
+            )
+            root.set_attribute(
+                "list-with-non-primitive-data-type", [dict(), 123]
+            )
+
+            self.assertEqual(len(root.attributes), 0)
+
+    def test_check_sequence_helper(self):
+        # pylint: disable=protected-access
+        self.assertEqual(
+            trace.Span._check_attribute_value_sequence([1, 2, 3.4, "ss", 4]),
+            "different type",
+        )
+        self.assertEqual(
+            trace.Span._check_attribute_value_sequence([dict(), 1, 2, 3.4, 4]),
+            "invalid type",
+        )
+        self.assertEqual(
+            trace.Span._check_attribute_value_sequence(
+                ["sw", "lf", 3.4, "ss"]
+            ),
+            "different type",
+        )
+        self.assertIsNone(
+            trace.Span._check_attribute_value_sequence([1, 2, 3.4, 5])
+        )
+        self.assertIsNone(
+            trace.Span._check_attribute_value_sequence(["ss", "dw", "fw"])
+        )
+
     def test_sampling_attributes(self):
         decision_attributes = {
             "sampler-attr": "sample-val",