-
Notifications
You must be signed in to change notification settings - Fork 83
feat(Audience Evaluation): Use log messages to explain the outcome of audience evaluation #210
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great start! I've added a number of suggestions but hopefully they're fairly straightforward!
packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.js
Outdated
Show resolved
Hide resolved
packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.js
Outdated
Show resolved
Hide resolved
packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.js
Outdated
Show resolved
Hide resolved
packages/optimizely-sdk/lib/core/decision_service/index.tests.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates look good. Just a couple last requests!
var result = conditionTreeEvaluator.evaluate(audience.conditions, evaluateConditionWithUserAttributes); | ||
var resultText = result === null ? 'UNKNOWN' : sprintf('%s', result); | ||
logger.log(LOG_LEVEL.DEBUG, sprintf(LOG_MESSAGES.AUDIENCE_EVALUATION_RESULT, MODULE_NAME, audienceId, resultText)); | ||
var resultText = result === null ? 'UNKNOWN' : result.toString().toUpperCase(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result.toString()
only works if result
isn't null or undefined, but I guess we should be okay in this case. 👍
packages/optimizely-sdk/lib/core/audience_evaluator/index.tests.js
Outdated
Show resolved
Hide resolved
packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.js
Show resolved
Hide resolved
} | ||
|
||
if (!fns.isFinite(userValue)) { | ||
logger.log(LOG_LEVEL.WARNING, sprintf(LOG_MESSAGES.UNEXPECTED_TYPE, MODULE_NAME, JSON.stringify(condition), conditionName, userValue)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for changing this! Looks like you also fixed the order of the last two arguments. I'm guessing the test case¹ was failing and we just hadn't noticed yet?
¹ should log and return null if the user-provided value is of a different type than the condition value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more suggestions, but almost there. Thanks again! 😊
packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.js
Outdated
Show resolved
Hide resolved
packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.js
Outdated
Show resolved
Hide resolved
packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.js
Outdated
Show resolved
Hide resolved
packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.js
Outdated
Show resolved
Hide resolved
packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.tests.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Thanks so much for bearing with me!
Summary
This adds logging for audience evaluation.