feat(Audience Evaluation): Audience Logging #154
Merged
+1,216
−384
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 similar comments
# Conflicts: # src/Optimizely/DecisionService/DecisionService.php # src/Optimizely/Utils/CustomAttributeConditionEvaluator.php # src/Optimizely/Utils/Validator.php # tests/DecisionServiceTests/DecisionServiceTest.php # tests/OptimizelyTest.php # tests/UtilsTests/CustomAttributeConditionEvaluatorTest.php # tests/UtilsTests/ValidatorTest.php
todo: OptimizelyTest
nchilada
suggested changes
Mar 2, 2019
There was a problem hiding this comment.
Looking good! I've added some comments - a few edits for the log messages and log levels, and then a couple other questions that are probably safe to ignore.
@aliabbasrizvi can you review those questions on Monday?
| const EVALUATING_AUDIENCE = "Starting to evaluate audience \"%s\" with conditions: %s."; | ||
| const INFINITE_ATTRIBUTE_VALUE = "Audience condition %s evaluated to UNKNOWN for user attribute \"%s\" is not in the range [-2^53, +2^53]."; | ||
| const MISSING_ATTRIBUTE_VALUE = "Audience condition %s evaluated to UNKNOWN because no value was passed for user attribute \"%s\"."; | ||
| const NULL_ATTRIBUTE_VALUE = "Audience condition \"%s\" evaluated to UNKNOWN because a null value was passed for user attribute \"%s\"."; |
There was a problem hiding this comment.
Suggested change
| const NULL_ATTRIBUTE_VALUE = "Audience condition \"%s\" evaluated to UNKNOWN because a null value was passed for user attribute \"%s\"."; | |
| const NULL_ATTRIBUTE_VALUE = "Audience condition %s evaluated to UNKNOWN because a null value was passed for user attribute \"%s\"."; |
| const NULL_ATTRIBUTE_VALUE = "Audience condition \"%s\" evaluated to UNKNOWN because a null value was passed for user attribute \"%s\"."; | ||
| const UNEXPECTED_TYPE = "Audience condition %s evaluated to UNKNOWN because a value of type \"%s\" was passed for user attribute \"%s\"."; | ||
|
|
||
| const UNKNOWN_CONDITION_TYPE = "Audience condition \"%s\" uses an unknown condition type. You may need to upgrade to anewer release of the Optimizely SDK."; |
There was a problem hiding this comment.
Suggested change
| const UNKNOWN_CONDITION_TYPE = "Audience condition \"%s\" uses an unknown condition type. You may need to upgrade to anewer release of the Optimizely SDK."; | |
| const UNKNOWN_CONDITION_TYPE = "Audience condition %s uses an unknown condition type. You may need to upgrade to anewer release of the Optimizely SDK."; |
| const UNEXPECTED_TYPE = "Audience condition %s evaluated to UNKNOWN because a value of type \"%s\" was passed for user attribute \"%s\"."; | ||
|
|
||
| const UNKNOWN_CONDITION_TYPE = "Audience condition \"%s\" uses an unknown condition type. You may need to upgrade to anewer release of the Optimizely SDK."; | ||
| const UNKNOWN_CONDITION_VALUE = "Audience condition \"%s\" has an unsupported condition value. You may need to upgrade to a newer release of the Optimizely SDK."; |
There was a problem hiding this comment.
Suggested change
| const UNKNOWN_CONDITION_VALUE = "Audience condition \"%s\" has an unsupported condition value. You may need to upgrade to a newer release of the Optimizely SDK."; | |
| const UNKNOWN_CONDITION_VALUE = "Audience condition %s has an unsupported condition value. You may need to upgrade to a newer release of the Optimizely SDK."; |
|
|
||
| const UNKNOWN_CONDITION_TYPE = "Audience condition \"%s\" uses an unknown condition type. You may need to upgrade to anewer release of the Optimizely SDK."; | ||
| const UNKNOWN_CONDITION_VALUE = "Audience condition \"%s\" has an unsupported condition value. You may need to upgrade to a newer release of the Optimizely SDK."; | ||
| const UNKNOWN_MATCH_TYPE = "Audience condition \"%s\" uses an unknown match type. You may need to upgrade to a newer release of the Optimizely SDK."; |
There was a problem hiding this comment.
Suggested change
| const UNKNOWN_MATCH_TYPE = "Audience condition \"%s\" uses an unknown match type. You may need to upgrade to a newer release of the Optimizely SDK."; | |
| const UNKNOWN_MATCH_TYPE = "Audience condition %s uses an unknown match type. You may need to upgrade to a newer release of the Optimizely SDK."; |
| const AUDIENCE_EVALUATION_RESULT_COMBINED = "Audiences for experiment \"%s\" collectively evaluated to %s."; | ||
| const EVALUATING_AUDIENCES_COMBINED = "Evaluating audiences for experiment \"%s\": %s."; | ||
| const EVALUATING_AUDIENCE = "Starting to evaluate audience \"%s\" with conditions: %s."; | ||
| const INFINITE_ATTRIBUTE_VALUE = "Audience condition %s evaluated to UNKNOWN for user attribute \"%s\" is not in the range [-2^53, +2^53]."; |
There was a problem hiding this comment.
Suggested change
| const INFINITE_ATTRIBUTE_VALUE = "Audience condition %s evaluated to UNKNOWN for user attribute \"%s\" is not in the range [-2^53, +2^53]."; | |
| const INFINITE_ATTRIBUTE_VALUE = "Audience condition %s evaluated to UNKNOWN because the number value for user attribute \"%s\" is not in the range [-2^53, +2^53]."; |
| * | ||
| * @return boolean Representing whether user meets audience conditions to be in experiment or not. | ||
| */ | ||
| public static function isUserInExperiment($config, $experiment, $userAttributes) | ||
| public static function isUserInExperiment($config, $experiment, $userAttributes, $logger) |
There was a problem hiding this comment.
Huh, I would have expected this function to be in DecisionService.php or AudienceSomethingSomething.php, but it's fine since this is pre-existing and doesn't really matter.
There was a problem hiding this comment.
I think it is ok to leave it here for now.
| } | ||
|
|
||
| if ($userValue === null) { | ||
| $this->logger->log(Logger::WARNING, sprintf( |
There was a problem hiding this comment.
Suggested change
| $this->logger->log(Logger::WARNING, sprintf( | |
| $this->logger->log(Logger::DEBUG, sprintf( |
| json_encode($audience->getConditionsList()) | ||
| )); | ||
|
|
||
| $conditionTreeEvaluator = new ConditionTreeEvaluator(); |
There was a problem hiding this comment.
Might be slightly more efficient to hoist this object instantiation out of the $evaluateAudience closure...I'm not sure how well PHP is able to do such optimizations.
But it probably doesn't matter since ConditionTreeEvaluator doesn't have a constructor or member variables, and because there probably won't be many audiences per experiment/feature.
| )); | ||
| return null; | ||
| } else { | ||
| $userValue = $this->userAttributes[$conditionName]; |
There was a problem hiding this comment.
Nit: can we pull this out of the else block? That'd make it clearer that $userValue is always defined by the time we get to the $userValue === null check.
nchilada
approved these changes
Mar 4, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This adds logging to audience evaluation.
For now, logging is limited to issues with user passed value and not the condition value (datafile).
Test plan
Issues