add check for event attribute values

Author: Andrew Xue

Diff for: opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -372,38 +372,39 @@ def set_attribute(self, key: str, value: types.AttributeValue) -> None:
             logger.warning("invalid key (empty or null)")
             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
-            # Freeze mutable sequences defensively
-            if isinstance(value, MutableSequence):
-                value = tuple(value)
-        elif not isinstance(value, (bool, str, int, float)):
-            logger.warning("invalid type for attribute value")
+        error_message = self._check_attribute_value(value)
+        if error_message is not None:
+            logger.warning(error_message)
             return
+        # Freeze mutable sequences defensively
+        if isinstance(value, MutableSequence):
+            value = tuple(value)
 
         self.attributes[key] = value
 
     @staticmethod
-    def _check_attribute_value_sequence(sequence: Sequence) -> Optional[str]:
+    def _check_attribute_value(value: types.AttributeValue) -> Optional[str]:
         """
         Checks if sequence items are valid and are of the same type
         """
-        if len(sequence) == 0:
-            return None
+        if isinstance(value, Sequence):
+            if len(value) == 0:
+                return None
 
-        first_element_type = type(sequence[0])
+            first_element_type = type(value[0])
 
-        if first_element_type not in (bool, str, int, float):
-            return "invalid type"
+            if first_element_type not in (bool, str, int, float):
+                return "invalid type in attribute value sequence"
 
-        for element in sequence:
-            if not isinstance(element, first_element_type):
-                return "different type"
+            for element in value:
+                if not isinstance(element, first_element_type):
+                    return "different types in attribute value sequence"
+            return None
+        elif not isinstance(value, (bool, str, int, float)):
+            return "invalid type for attribute value"
         return None
 
+
     def _add_event(self, event: EventBase) -> None:
         with self._lock:
             if not self.is_recording_events():
@@ -425,6 +426,14 @@ def add_event(
     ) -> None:
         if attributes is None:
             attributes = Span._empty_attributes
+        else:
+            for attr_key, attr_value in list(attributes.items()):
+                error_message = self._check_attribute_value(attr_value)
+                if error_message:
+                    logger.warning(error_message)
+                    return
+                if isinstance(attr_value, MutableSequence):
+                    attributes[attr_key] = tuple(attr_value)
         self._add_event(
             Event(
                 name=name,

Diff for: opentelemetry-sdk/tests/trace/test_trace.py

@@ -487,39 +487,61 @@ def test_invalid_attribute_values(self):
 
             self.assertEqual(len(root.attributes), 0)
 
-    def test_check_sequence_helper(self):
+    def test_check_attribute_helper(self):
         # pylint: disable=protected-access
         self.assertEqual(
-            trace.Span._check_attribute_value_sequence([1, 2, 3.4, "ss", 4]),
-            "different type",
+            trace.Span._check_attribute_value([1, 2, 3.4, "ss", 4]),
+            "different types in attribute value sequence",
         )
         self.assertEqual(
-            trace.Span._check_attribute_value_sequence([dict(), 1, 2, 3.4, 4]),
-            "invalid type",
+            trace.Span._check_attribute_value([dict(), 1, 2, 3.4, 4]),
+            "invalid type in attribute value sequence",
         )
         self.assertEqual(
-            trace.Span._check_attribute_value_sequence(
+            trace.Span._check_attribute_value(
                 ["sw", "lf", 3.4, "ss"]
             ),
-            "different type",
+            "different types in attribute value sequence",
         )
         self.assertEqual(
-            trace.Span._check_attribute_value_sequence([1, 2, 3.4, 5]),
-            "different type",
+            trace.Span._check_attribute_value([1, 2, 3.4, 5]),
+            "different types in attribute value sequence",
         )
         self.assertIsNone(
-            trace.Span._check_attribute_value_sequence([1, 2, 3, 5])
+            trace.Span._check_attribute_value([1, 2, 3, 5])
         )
         self.assertIsNone(
-            trace.Span._check_attribute_value_sequence([1.2, 2.3, 3.4, 4.5])
+            trace.Span._check_attribute_value([1.2, 2.3, 3.4, 4.5])
         )
         self.assertIsNone(
-            trace.Span._check_attribute_value_sequence([True, False])
+            trace.Span._check_attribute_value([True, False])
         )
         self.assertIsNone(
-            trace.Span._check_attribute_value_sequence(["ss", "dw", "fw"])
+            trace.Span._check_attribute_value(["ss", "dw", "fw"])
+        )
+        self.assertIsNone(
+            trace.Span._check_attribute_value([])
+        )
+
+
+        self.assertEqual(
+            trace.Span._check_attribute_value(dict()),
+            "invalid type for attribute value",
+        )
+        self.assertIsNone(
+            trace.Span._check_attribute_value(True)
+        )
+        self.assertIsNone(
+            trace.Span._check_attribute_value('hi')
+        )
+        self.assertIsNone(
+            trace.Span._check_attribute_value(3.4)
+        )
+        self.assertIsNone(
+            trace.Span._check_attribute_value(15)
         )
 
+
     def test_sampling_attributes(self):
         decision_attributes = {
             "sampler-attr": "sample-val",
@@ -561,33 +583,52 @@ def test_events(self):
 
             # event name and attributes
             now = time_ns()
-            root.add_event("event1", {"name": "pluto"})
+            root.add_event("event1", {"name": "pluto", "some_bools": [True, False]})
 
             # event name, attributes and timestamp
             now = time_ns()
-            root.add_event("event2", {"name": "birthday"}, now)
+            root.add_event("event2", {"name": ["birthday"]}, now)
+
+            mutable_list = ["original_contents"]
+            root.add_event("event3", {"name": mutable_list})
 
             def event_formatter():
                 return {"name": "hello"}
 
             # lazy event
-            root.add_lazy_event("event3", event_formatter, now)
+            root.add_lazy_event("event4", event_formatter, now)
 
-            self.assertEqual(len(root.events), 4)
+            self.assertEqual(len(root.events), 5)
 
             self.assertEqual(root.events[0].name, "event0")
             self.assertEqual(root.events[0].attributes, {})
 
             self.assertEqual(root.events[1].name, "event1")
-            self.assertEqual(root.events[1].attributes, {"name": "pluto"})
+            self.assertEqual(root.events[1].attributes, {"name": "pluto", "some_bools": (True, False)})
 
             self.assertEqual(root.events[2].name, "event2")
-            self.assertEqual(root.events[2].attributes, {"name": "birthday"})
+            self.assertEqual(root.events[2].attributes, {"name": ("birthday",)})
             self.assertEqual(root.events[2].timestamp, now)
 
             self.assertEqual(root.events[3].name, "event3")
-            self.assertEqual(root.events[3].attributes, {"name": "hello"})
-            self.assertEqual(root.events[3].timestamp, now)
+            self.assertEqual(root.events[3].attributes, {"name": ("original_contents",)})
+            mutable_list = ["new_contents"]
+            self.assertEqual(root.events[3].attributes, {"name": ("original_contents",)})
+
+            self.assertEqual(root.events[4].name, "event4")
+            self.assertEqual(root.events[4].attributes, {"name": "hello"})
+            self.assertEqual(root.events[4].timestamp, now)
+
+    def test_invalid_event_attributes(self):
+        self.assertIsNone(self.tracer.get_current_span())
+
+        with self.tracer.start_as_current_span("root") as root:
+            root.add_event("event0", {"attr1": True, "attr2": ['hi', False]})
+            root.add_event("event0", {"attr1": dict()})
+            root.add_event("event0", {"attr1": [[True]]})
+            root.add_event("event0", {"attr1": [dict()]})
+
+            self.assertEqual(len(root.events), 0)
 
     def test_links(self):
         other_context1 = trace_api.SpanContext(
@@ -882,3 +923,4 @@ def test_add_span_processor_after_span_creation(self):
         expected_list.append(span_event_end_fmt("SP1", "foo"))
 
         self.assertListEqual(spans_calls_list, expected_list)
+