added properties to track method

Matias Melograno · Matias Melograno · commit 00eb469e929b · 2019-05-23T14:12:16.000-03:00
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -1,3 +1,5 @@
+8.1.0 (May, 2019)
+ - Added properties to track method.
 8.0.0 (Apr 24, 2019)
  - Full SDK Refactor/rewrite.
  - New block until ready behaviour.
diff --git a/splitio/client/client.py b/splitio/client/client.py
@@ -14,7 +14,7 @@
 from splitio.client.listener import ImpressionListenerException
 
 
-class Client(object):  #pylint: disable=too-many-instance-attributes
+class Client(object):  # pylint: disable=too-many-instance-attributes
     """Entry point for the split sdk."""
 
     _METRIC_GET_TREATMENT = 'sdk.getTreatment'
@@ -41,11 +41,11 @@ def __init__(self, factory, labels_enabled=True, impression_listener=None):
         self._impression_listener = impression_listener
 
         self._splitter = Splitter()
-        self._split_storage = factory._get_storage('splits')  #pylint: disable=protected-access
-        self._segment_storage = factory._get_storage('segments')  #pylint: disable=protected-access
-        self._impressions_storage = factory._get_storage('impressions')  #pylint: disable=protected-access
-        self._events_storage = factory._get_storage('events')  #pylint: disable=protected-access
-        self._telemetry_storage = factory._get_storage('telemetry')  #pylint: disable=protected-access
+        self._split_storage = factory._get_storage('splits')  # pylint: disable=protected-access
+        self._segment_storage = factory._get_storage('segments')  # pylint: disable=protected-access
+        self._impressions_storage = factory._get_storage('impressions')  # pylint: disable=protected-access
+        self._events_storage = factory._get_storage('events')  # pylint: disable=protected-access
+        self._telemetry_storage = factory._get_storage('telemetry')  # pylint: disable=protected-access
         self._evaluator = Evaluator(self._split_storage, self._segment_storage, self._splitter)
 
     def destroy(self):
@@ -136,7 +136,7 @@ def get_treatment_with_config(self, key, feature, attributes=None):
             self._record_stats(impression, start, self._METRIC_GET_TREATMENT)
             self._send_impression_to_listener(impression, attributes)
             return result['treatment'], result['configurations']
-        except Exception:  #pylint: disable=broad-except
+        except Exception:  # pylint: disable=broad-except
             self._logger.error('Error getting treatment for feature')
             self._logger.debug('Error: ', exc_info=True)
             try:
@@ -231,7 +231,7 @@ def get_treatments_with_config(self, key, features, attributes=None):
                 bulk_impressions.append(impression)
                 treatments[feature] = (treatment['treatment'], treatment['configurations'])
 
-            except Exception:  #pylint: disable=broad-except
+            except Exception:  # pylint: disable=broad-except
                 self._logger.error('get_treatments: An exception occured when evaluating '
                                    'feature ' + feature + ' returning CONTROL.')
                 treatments[feature] = CONTROL, None
@@ -244,14 +244,13 @@ def get_treatments_with_config(self, key, features, attributes=None):
                 self._record_stats(bulk_impressions, start, self._METRIC_GET_TREATMENTS)
                 for impression in bulk_impressions:
                     self._send_impression_to_listener(impression, attributes)
-        except Exception:  #pylint: disable=broad-except
+        except Exception:  # pylint: disable=broad-except
             self._logger.error('get_treatments: An exception when trying to store '
                                'impressions.')
             self._logger.debug('Error: ', exc_info=True)
 
         return treatments
 
-
     def get_treatments(self, key, features, attributes=None):
         """
         Evaluate multiple features and return a dictionary with all the feature/treatments.
@@ -271,7 +270,7 @@ def get_treatments(self, key, features, attributes=None):
         with_config = self.get_treatments_with_config(key, features, attributes)
         return {feature: result[0] for (feature, result) in six.iteritems(with_config)}
 
-    def _build_impression(  #pylint: disable=too-many-arguments
+    def _build_impression(  # pylint: disable=too-many-arguments
             self,
             matching_key,
             feature_name,
@@ -311,11 +310,11 @@ def _record_stats(self, impressions, start, operation):
             else:
                 self._impressions_storage.put(impressions)
             self._telemetry_storage.inc_latency(operation, get_latency_bucket_index(end - start))
-        except Exception:  #pylint: disable=broad-except
+        except Exception:  # pylint: disable=broad-except
             self._logger.error('Error recording impressions and metrics')
             self._logger.debug('Error: ', exc_info=True)
 
-    def track(self, key, traffic_type, event_type, value=None):
+    def track(self, key, traffic_type, event_type, value=None, properties=None):
         """
         Track an event.
 
@@ -327,6 +326,8 @@ def track(self, key, traffic_type, event_type, value=None):
         :type event_type: str
         :param value: (Optional) value associated to the event
         :type value: Number
+        :param properties: (Optional) properties associated to the event
+        :type properties: dict
 
         :return: Whether the event was created or not.
         :rtype: bool
@@ -339,15 +340,18 @@ def track(self, key, traffic_type, event_type, value=None):
         event_type = input_validator.validate_event_type(event_type)
         traffic_type = input_validator.validate_traffic_type(traffic_type)
         value = input_validator.validate_value(value)
+        valid, properties = input_validator.valid_properties(properties)
 
-        if key is None or event_type is None or traffic_type is None or value is False:
+        if key is None or event_type is None or traffic_type is None or value is False \
+           or valid is False:
             return False
 
         event = Event(
             key=key,
             traffic_type_name=traffic_type,
             event_type_id=event_type,
             value=value,
-            timestamp=int(time.time()*1000)
+            timestamp=int(time.time()*1000),
+            properties=properties,
         )
         return self._events_storage.put([event])
diff --git a/splitio/client/input_validator.py b/splitio/client/input_validator.py
@@ -8,6 +8,7 @@
 import math
 
 import six
+import sys
 
 from splitio.api import APIException
 from splitio.client.key import Key
@@ -18,6 +19,7 @@
 _LOGGER = logging.getLogger(__name__)
 MAX_LENGTH = 250
 EVENT_TYPE_PATTERN = r'^[a-zA-Z0-9][-_.:a-zA-Z0-9]{0,79}$'
+MAX_PROPERTIES_LENGTH_BYTES = 32768
 
 
 def _get_first_split_sdk_call():
@@ -33,9 +35,10 @@ def _get_first_split_sdk_call():
         if calls:
             return calls[-1]
         return unknown_method
-    except Exception:  #pylint: disable=broad-except
+    except Exception:  # pylint: disable=broad-except
         return unknown_method
 
+
 def _check_not_null(value, name, operation):
     """
     Check if value is null.
@@ -358,7 +361,7 @@ def validate_manager_feature_name(feature_name):
     return feature_name
 
 
-def validate_features_get_treatments(features):  #pylint: disable=invalid-name
+def validate_features_get_treatments(features):  # pylint: disable=invalid-name
     """
     Check if features is valid for get_treatments.
 
@@ -432,7 +435,7 @@ def validate_apikey_type(segment_api):
     """
     api_messages_filter = _ApiLogFilter()
     try:
-        segment_api._logger.addFilter(api_messages_filter)  #pylint: disable=protected-access
+        segment_api._logger.addFilter(api_messages_filter)  # pylint: disable=protected-access
         segment_api.fetch_segment('__SOME_INVALID_SEGMENT__', -1)
     except APIException as exc:
         if exc.status_code == 403:
@@ -441,7 +444,7 @@ def validate_apikey_type(segment_api):
                           + 'console that is of type sdk')
             return False
     finally:
-        segment_api._logger.removeFilter(api_messages_filter)  #pylint: disable=protected-access
+        segment_api._logger.removeFilter(api_messages_filter)  # pylint: disable=protected-access
 
     # True doesn't mean that the APIKEY is right, only that it's not of type "browser"
     return True
@@ -453,10 +456,6 @@ def validate_factory_instantiation(apikey):
 
     :param apikey: str
     :type apikey: str
-    :param config: dict
-    :type config: dict
-    :param segment_api: Segment API client
-    :type segment_api: splitio.api.segments.SegmentsAPI
     :return: bool
     :rtype: True|False
     """
@@ -467,3 +466,57 @@ def validate_factory_instantiation(apikey):
        (not _check_string_not_empty(apikey, 'apikey', 'factory_instantiation')):
         return False
     return True
+
+
+def valid_properties(properties):
+    """
+    Check if properties is a valid dict and returns the properties
+    that will be sent to the track method, avoiding unexpected types.
+
+    :param properties: dict
+    :type properties: dict
+    :return: tuple
+    :rtype: (bool,dict)
+    """
+    if properties is None:
+        return True, None
+    if not isinstance(properties, dict):
+        _LOGGER.error('track: properties must be of type dictionary.')
+        return False, None
+
+    size = 1024  # We assume 1kb events without properties (750 bytes avg measured)
+    valid_properties = None
+
+    for property, element in properties.items():
+        if not isinstance(property, six.string_types):  # Exclude property if is not string
+            continue
+
+        if valid_properties is None:
+            valid_properties = dict()
+
+        valid_properties[property] = None
+        size += sys.getsizeof(property)
+
+        if element is None:
+            continue
+
+        if not isinstance(element, six.string_types) and not isinstance(element, Number) \
+           and not isinstance(element, bool):
+            _LOGGER.warning('Property %s is of invalid type. Setting value to None', element)
+            element = None
+
+        valid_properties[property] = element
+        size += sys.getsizeof(str(element))
+
+        if size > MAX_PROPERTIES_LENGTH_BYTES:
+            _LOGGER.error(
+                'The maximum size allowed for the properties is 32768 bytes. ' +
+                'Current one is ' + str(size) + ' bytes. Event not queued'
+            )
+            return False, None
+
+    if isinstance(valid_properties, dict) and len(valid_properties.keys()) > 300:
+        _LOGGER.warning('Event has more than 300 properties. Some of them will be trimmed' +
+                        ' when processed')
+
+    return True, valid_properties
diff --git a/splitio/models/events.py b/splitio/models/events.py
@@ -14,4 +14,5 @@
     'event_type_id',
     'value',
     'timestamp',
+    'properties',
 ])
diff --git a/splitio/version.py b/splitio/version.py
@@ -1 +1 @@
-__version__ = '8.0.0'
+__version__ = '8.1.0-rc1'
diff --git a/tests/client/test_input_validator.py b/tests/client/test_input_validator.py
@@ -13,6 +13,7 @@
 from splitio.storage import SplitStorage, EventStorage, ImpressionStorage, TelemetryStorage, \
     SegmentStorage
 from splitio.models.splits import Split
+from splitio.client import input_validator
 
 
 class ClientInputValidationTests(object):
@@ -445,6 +446,41 @@ def _get_storage_mock(storage):
             mocker.call('%s: feature_name \'%s\' has extra whitespace, trimming.', 'get_treatment_with_config', '  some_feature   ')
         ]
 
+    def test_valid_properties(self, mocker):
+        """Test valid_properties() method"""
+        assert input_validator.valid_properties(None) == (True, None)
+        assert input_validator.valid_properties([]) == (False, None)
+        assert input_validator.valid_properties(True) == (False, None)
+
+        props1 = {
+            "test1": "test",
+            "test2": 1,
+            "test3": True,
+            "test4": None,
+            "test5": [],
+            2: "t",
+        }
+        r1, r2 = input_validator.valid_properties(props1)
+        assert r1 == True
+        assert len(r2.keys()) == 5
+        assert r2["test1"] == "test"
+        assert r2["test2"] == 1
+        assert r2["test3"] == True
+        assert r2["test4"] == None
+        assert r2["test5"] == None
+
+        props2 = dict();
+        for i in range(301):
+            props2[str(i)] = i
+        assert input_validator.valid_properties(props2) == (True, props2)
+
+        props3 = dict();
+        for i in range(110):
+            props3[str(i)] = str(i) * 300
+        r1, r2 = input_validator.valid_properties(props3)
+        assert r1 == False
+
+
     def test_track(self, mocker):
         """Test track method()."""
         events_storage_mock = mocker.Mock(spec=EventStorage)
@@ -602,6 +638,57 @@ def test_track(self, mocker):
             mocker.call("track: value must be a number.")
         ]
 
+        client._logger.reset_mock()
+        assert client.track("some_key", "traffic_type", "event_type", 1, []) is False
+        assert client._logger.error.mock_calls == [
+            mocker.call("track: properties must be of type dictionary.")
+        ]
+
+        client._logger.reset_mock()
+        assert client.track("some_key", "traffic_type", "event_type", 1, []) is False
+        assert client._logger.error.mock_calls == [
+            mocker.call("track: properties must be of type dictionary.")
+        ]
+
+        client._logger.reset_mock()
+        assert client.track("some_key", "traffic_type", "event_type", 1, True) is False
+        assert client._logger.error.mock_calls == [
+            mocker.call("track: properties must be of type dictionary.")
+        ]
+
+        props1 = {
+            "test1": "test",
+            "test2": 1,
+            "test3": True,
+            "test4": None,
+            "test5": [],
+            2: "t",
+        }
+        client._logger.reset_mock()
+        assert client.track("some_key", "traffic_type", "event_type", 1, props1) is True
+        assert client._logger.warning.mock_calls == [
+            mocker.call("Property %s is of invalid type. Setting value to None", [])
+        ]
+
+        props2 = dict();
+        for i in range(301):
+            props2[str(i)] = i
+        client._logger.reset_mock()
+        assert client.track("some_key", "traffic_type", "event_type", 1, props2) is True
+        assert client._logger.warning.mock_calls == [
+            mocker.call("Event has more than 300 properties. Some of them will be trimmed when processed")
+        ]
+
+        client._logger.reset_mock()
+        props3 = dict();
+        for i in range(110):
+            props3[str(i)] = str(i) * 300
+        assert client.track("some_key", "traffic_type", "event_type", 1, props3) is False
+        assert client._logger.error.mock_calls == [
+            mocker.call("The maximum size allowed for the properties is 32768 bytes. Current one is 32949 bytes. Event not queued")
+        ]
+
+
     def test_get_treatments(self, mocker):
         """Test getTreatments() method."""
         split_mock = mocker.Mock(spec=Split)

Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,5 @@`
	`1`	`+8.1.0 (May, 2019)`
	`2`	`+ - Added properties to track method.`
`1`	`3`	`8.0.0 (Apr 24, 2019)`
`2`	`4`	`- Full SDK Refactor/rewrite.`
`3`	`5`	`- New block until ready behaviour.`
Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-__version__ = '8.0.0'`
	`1`	`+__version__ = '8.1.0-rc1'`