Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parses revenue value and logger integration #134

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions optimizely/event_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@ def __init__(self, url, params, http_verb=None, headers=None):
class BaseEventBuilder(object):
""" Base class which encapsulates methods to build events for tracking impressions and conversions. """

def __init__(self, config):
def __init__(self, config, logger):
self.config = config
self.logger = logger

@abstractproperty
class EventParams(object):
Expand Down Expand Up @@ -263,11 +264,11 @@ def _get_required_params_for_conversion(self, event_key, event_tags, decisions):
}

if event_tags:
revenue_value = event_tag_utils.get_revenue_value(event_tags)
revenue_value = event_tag_utils.get_revenue_value(event_tags, self.logger)
if revenue_value is not None:
event_dict[event_tag_utils.REVENUE_METRIC_TYPE] = revenue_value

numeric_value = event_tag_utils.get_numeric_value(event_tags, self.config.logger)
numeric_value = event_tag_utils.get_numeric_value(event_tags, self.logger)
if numeric_value is not None:
event_dict[event_tag_utils.NUMERIC_METRIC_TYPE] = numeric_value

Expand Down
51 changes: 44 additions & 7 deletions optimizely/helpers/event_tag_utils.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright 2017, Optimizely
# Copyright 2017-2018, Optimizely
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
Expand All @@ -19,34 +19,71 @@
NUMERIC_METRIC_TYPE = 'value'


def get_revenue_value(event_tags):
def get_revenue_value(event_tags, logger):
"""
A smart getter of the revenue value from the event tags.

Args:
event_tags: A dictionary of event tags.
logger - A component which provides a log method to log messages.

Returns:
An integer revenue value is returned when the provided revenue
value is in the following format:
- A string (properly formatted, e.g., no commas)
- An integer
- A float or double
- 4.0 or "4.0" will be parsed to int(4).
None is returned when the provided revenue values is in
the following format:
- None
- A boolean
- inf, -inf, nan
- A string not properly formatted (e.g., '1,234')
- 4.1 can not be parsed
- Any values that cannot be cast to a float (e.g., an array or dictionary)
"""

if event_tags is None:
logger.log(enums.LogLevels.DEBUG, 'Event tags is undefined.')
return None

if not isinstance(event_tags, dict):
logger.log(enums.LogLevels.DEBUG, 'Event tags is not a dict.')
return None

if REVENUE_METRIC_TYPE not in event_tags:
logger.log(enums.LogLevels.DEBUG, 'The revenue key is not defined in the event tags.')
return None

raw_value = event_tags[REVENUE_METRIC_TYPE]

if isinstance(raw_value, bool):
if not type(raw_value) in (float, int, str):
logger.log(enums.LogLevels.WARNING, 'Revenue value is not an integer or float or a string.')
return None

if not isinstance(raw_value, numbers.Integral):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This restriction exists because the revenue field, unlike value, can only take on integral values when tracked to Optimizely's events endpoint. So if we accept other types, we'd have to only drop all values that can't be converted to integers.

Given that, and given the fact that APIs can't be narrowed once broadened, I'd err on the side of closing this PR. 😕

(Regarding the size limits that we're introducing in #151: internal documentation leads me to believe that the revenue field can be rather large. But I'm not sure since it's not well-documented. I'd say we don't need to worry about these size limits for now.)

Copy link
Contributor

@nchilada nchilada Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Oops, @aliabbasrizvi did suggest that we apply a size limit to the revenue field and its companions. Let's follow up in the context of #151, the PR for size limiting.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikeng13 Can you please give your input on this? We already have revenue value parsing implemented in PHP and Ruby. optimizely/php-sdk#91.
Please note that 4.1 is not accepted as a valid revenue value while 4.0, "4", "4.0" are valid.

Javascript is parsing 13 from 13.37. https://github.com/optimizely/javascript-sdk/blob/master/packages/optimizely-sdk/lib/utils/event_tag_utils/index.tests.js#L46

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oakbani oh, interesting, I didn't know that other SDKs already do some kind of advanced parsing. Seems okay if we accept a consistent set of string formats and if we ignore non-integer values.

@mikeng13 @ceimaj can you all follow up here? TL;DR we need to figure out how to parse revenue across our SDKs

if isinstance(raw_value, str):
try:
raw_value = float(raw_value)
except ValueError:
logger.log(enums.LogLevels.WARNING, 'Revenue value is not a numeric string.')
return None

if raw_value != int(raw_value):
logger.log(enums.LogLevels.WARNING, 'Failed to parse revenue value "%s" from event tags.' % raw_value)
return None

return raw_value
logger.log(enums.LogLevels.INFO, 'Parsed revenue value "%s" from event tags.' % int(raw_value))
return int(raw_value)


def get_numeric_value(event_tags, logger=None):
def get_numeric_value(event_tags, logger):
"""
A smart getter of the numeric value from the event tags.

Args:
event_tags: A dictionary of event tags.
logger: Optional logger.
logger - A component which provides a log method to log messages.

Returns:
A float numeric metric value is returned when the provided numeric
Expand Down
2 changes: 1 addition & 1 deletion optimizely/optimizely.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def __init__(self,
self.logger.error(enums.Errors.UNSUPPORTED_DATAFILE_VERSION)
return

self.event_builder = event_builder.EventBuilder(self.config)
self.event_builder = event_builder.EventBuilder(self.config, self.logger)
self.decision_service = decision_service.DecisionService(self.config, user_profile_service)
self.notification_center = notification_center(self.logger)

Expand Down
Loading