From 587297b6658f0eaecfb5948fd83cf601e1296511 Mon Sep 17 00:00:00 2001
From: Andrew Xue <aaxue@google.com>
Date: Tue, 12 May 2020 14:03:31 -0400
Subject: [PATCH] improve attribute filtering logic

---
 .../src/opentelemetry/sdk/trace/__init__.py   | 47 ++++++++-----------
 opentelemetry-sdk/tests/trace/test_trace.py   |  8 +++-
 2 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
index 91eb4d6325d..106a80df739 100644
--- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
+++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
@@ -245,33 +245,19 @@ def __init__(
         self.status = None
         self._lock = threading.Lock()
 
-        if attributes is None:
+        self._filter_attribute_values(attributes)
+        if attributes is None or len(attributes) == 0:
             self.attributes = Span._empty_attributes
         else:
-            self.attributes = BoundedDict(MAX_NUM_ATTRIBUTES)
-            for attr_key, attr_value in attributes.items():
-                error_message = self._check_attribute_value(attr_value)
-                if error_message:
-                    logger.warning(error_message)
-                else:
-                    self.attributes[attr_key] = attr_value
+            self.attributes = BoundedDict.from_map(MAX_NUM_ATTRIBUTES, attributes)
 
         if events is None:
             self.events = Span._empty_events
         else:
             self.events = BoundedList(MAX_NUM_EVENTS)
             for event in events:
-                good_event = True
-                for attr_key, attr_value in list(event.attributes.items()):
-                    error_message = self._check_attribute_value(attr_value)
-                    if error_message:
-                        logger.warning(error_message)
-                        good_event = False
-                        break
-                    if isinstance(attr_value, MutableSequence):
-                        attributes[attr_key] = tuple(attr_value)
-                if good_event:
-                    self.events.append(event)
+                self._filter_attribute_values(event.attributes)
+                self.events.append(event)
 
         if links is None:
             self.links = Span._empty_links
@@ -428,6 +414,18 @@ def _check_attribute_value(value: types.AttributeValue) -> Optional[str]:
                 [valid_type.__name__ for valid_type in valid_types],
             )
 
+    def _filter_attribute_values(self, attributes: types.Attributes):
+        if attributes:
+            for attr_key, attr_value in list(attributes.items()):
+                error_message = self._check_attribute_value(attr_value)
+                if error_message:
+                    attributes.pop(attr_key)
+                    logger.warning(error_message)
+                elif isinstance(attr_value, MutableSequence):
+                    attributes[attr_key] = tuple(attr_value)
+                else:
+                    attributes[attr_key] = attr_value
+
     def _add_event(self, event: EventBase) -> None:
         with self._lock:
             if not self.is_recording_events():
@@ -447,16 +445,9 @@ def add_event(
         attributes: types.Attributes = None,
         timestamp: Optional[int] = None,
     ) -> None:
-        if attributes is None:
+        self._filter_attribute_values(attributes)
+        if attributes is None or len(attributes) == 0:
             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 --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py
index 71e3046c0c7..5ff118d5579 100644
--- a/opentelemetry-sdk/tests/trace/test_trace.py
+++ b/opentelemetry-sdk/tests/trace/test_trace.py
@@ -619,9 +619,13 @@ def test_invalid_event_attributes(self):
             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()]})
+            root.add_event("event0", {"attr1": [dict()], "attr2": [1, 2]})
 
-            self.assertEqual(len(root.events), 0)
+            self.assertEqual(len(root.events), 4)
+            self.assertEqual(root.events[0].attributes, {"attr1": True})
+            self.assertEqual(root.events[1].attributes, {})
+            self.assertEqual(root.events[2].attributes, {})
+            self.assertEqual(root.events[3].attributes, {"attr2": (1, 2)})
 
     def test_links(self):
         other_context1 = trace_api.SpanContext(