-
Notifications
You must be signed in to change notification settings - Fork 30
fix(logs): Fixing log messages #201
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
Changes from 7 commits
e32e2b8
2b32343
ef92697
1332498
2617b03
5481ce4
45e7ed8
16b4f6d
6b9a7a6
8a0264f
ea3fa48
f4b577e
107e914
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
<?php | ||
/** | ||
* Copyright 2017-2019, Optimizely Inc and Contributors | ||
* Copyright 2017-2020, Optimizely Inc and Contributors | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
|
@@ -152,7 +152,7 @@ public function getVariation(ProjectConfigInterface $projectConfig, Experiment $ | |
} | ||
} | ||
|
||
if (!Validator::isUserInExperiment($projectConfig, $experiment, $attributes, $this->_logger)) { | ||
if (!Validator::doesUserMeetAudienceConditions($projectConfig, $experiment, $attributes, $this->_logger)) { | ||
$this->_logger->log( | ||
Logger::INFO, | ||
sprintf('User "%s" does not meet conditions to be in experiment "%s".', $userId, $experiment->getKey()) | ||
|
@@ -161,9 +161,21 @@ public function getVariation(ProjectConfigInterface $projectConfig, Experiment $ | |
} | ||
|
||
$variation = $this->_bucketer->bucket($projectConfig, $experiment, $bucketingId, $userId); | ||
if (!is_null($variation)) { | ||
if ($variation === null) { | ||
$this->_logger->log(Logger::INFO, sprintf('User "%s" is in no variation.', $userId)); | ||
} else { | ||
$this->saveVariation($experiment, $variation, $userProfile); | ||
$this->_logger->log( | ||
Logger::INFO, | ||
sprintf( | ||
'User "%s" is in variation %s of experiment %s.', | ||
$userId, | ||
$variation->getKey(), | ||
$experiment->getKey() | ||
) | ||
); | ||
} | ||
|
||
return $variation; | ||
} | ||
|
||
|
@@ -298,40 +310,40 @@ public function getVariationForFeatureRollout(ProjectConfigInterface $projectCon | |
|
||
// Evaluate all rollout rules except for last one | ||
for ($i = 0; $i < sizeof($rolloutRules) - 1; $i++) { | ||
$experiment = $rolloutRules[$i]; | ||
$rolloutRule = $rolloutRules[$i]; | ||
|
||
// Evaluate if user meets the audience condition of this rollout rule | ||
if (!Validator::isUserInExperiment($projectConfig, $experiment, $userAttributes, $this->_logger)) { | ||
if (!Validator::doesUserMeetAudienceConditions($projectConfig, $rolloutRule, $userAttributes, $this->_logger, true, $i + 1)) { | ||
$this->_logger->log( | ||
Logger::DEBUG, | ||
sprintf("User '%s' did not meet the audience conditions to be in rollout rule '%s'.", $userId, $experiment->getKey()) | ||
sprintf("User '%s' does not meet conditions for targeting rule %s.", $userId, $i+1) | ||
); | ||
// Evaluate this user for the next rule | ||
continue; | ||
} | ||
|
||
// Evaluate if user satisfies the traffic allocation for this rollout rule | ||
$variation = $this->_bucketer->bucket($projectConfig, $experiment, $bucketing_id, $userId); | ||
$variation = $this->_bucketer->bucket($projectConfig, $rolloutRule, $bucketing_id, $userId); | ||
if ($variation && $variation->getKey()) { | ||
return new FeatureDecision($experiment, $variation, FeatureDecision::DECISION_SOURCE_ROLLOUT); | ||
return new FeatureDecision($rolloutRule, $variation, FeatureDecision::DECISION_SOURCE_ROLLOUT); | ||
} | ||
break; | ||
} | ||
// Evaluate Everyone Else Rule / Last Rule now | ||
$experiment = $rolloutRules[sizeof($rolloutRules)-1]; | ||
$rolloutRule = $rolloutRules[sizeof($rolloutRules)-1]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it conventional to not have spacing around operators? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not something enforced. Subjective |
||
|
||
// Evaluate if user meets the audience condition of Everyone Else Rule / Last Rule now | ||
if (!Validator::isUserInExperiment($projectConfig, $experiment, $userAttributes, $this->_logger)) { | ||
if (!Validator::doesUserMeetAudienceConditions($projectConfig, $rolloutRule, $userAttributes, $this->_logger, true, 'Everyone Else')) { | ||
$this->_logger->log( | ||
Logger::DEBUG, | ||
sprintf("User '%s' did not meet the audience conditions to be in rollout rule '%s'.", $userId, $experiment->getKey()) | ||
sprintf("User '%s' does not meet conditions for targeting rule 'Everyone Else'.", $userId) | ||
); | ||
return null; | ||
} | ||
|
||
$variation = $this->_bucketer->bucket($projectConfig, $experiment, $bucketing_id, $userId); | ||
$variation = $this->_bucketer->bucket($projectConfig, $rolloutRule, $bucketing_id, $userId); | ||
if ($variation && $variation->getKey()) { | ||
return new FeatureDecision($experiment, $variation, FeatureDecision::DECISION_SOURCE_ROLLOUT); | ||
return new FeatureDecision($rolloutRule, $variation, FeatureDecision::DECISION_SOURCE_ROLLOUT); | ||
} | ||
return null; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
<?php | ||
/** | ||
* Copyright 2020, 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 | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
namespace Optimizely\Enums; | ||
|
||
class ExperimentAudienceEvaluationLogs extends CommonAudienceEvaluationLogs | ||
{ | ||
const AUDIENCE_EVALUATION_RESULT_COMBINED = "Audiences for experiment \"%s\" collectively evaluated to %s."; | ||
const EVALUATING_AUDIENCES_COMBINED = "Evaluating audiences for experiment \"%s\": %s."; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
<?php | ||
/** | ||
* Copyright 2020, 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 | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
namespace Optimizely\Enums; | ||
|
||
class RolloutAudienceEvaluationLogs extends CommonAudienceEvaluationLogs | ||
{ | ||
const AUDIENCE_EVALUATION_RESULT_COMBINED = "Audiences for rule %s collectively evaluated to %s."; | ||
const EVALUATING_AUDIENCES_COMBINED = "Evaluating audiences for rule %s: %s."; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,8 @@ | |
use Monolog\Logger; | ||
use Optimizely\Config\ProjectConfigInterface; | ||
use Optimizely\Entity\Experiment; | ||
use Optimizely\Enums\AudienceEvaluationLogs; | ||
use Optimizely\Enums\ExperimentAudienceEvaluationLogs; | ||
use Optimizely\Enums\RolloutAudienceEvaluationLogs; | ||
use Optimizely\Logger\LoggerInterface; | ||
use Optimizely\Utils\ConditionTreeEvaluator; | ||
use Optimizely\Utils\CustomAttributeConditionEvaluator; | ||
|
@@ -136,27 +137,37 @@ public static function areEventTagsValid($eventTags) | |
* @param $experiment Experiment Entity representing the experiment. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
* @param $userAttributes array Attributes of the user. | ||
* @param $logger LoggerInterface. | ||
* @param $isRollout Boolean true if experiment to evaluate is a rollout rule. | ||
* @param $rolloutRule String Rollout rule identifier to log. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can just pass in the experiment key or rule index and just log that here. |
||
* | ||
* @return boolean Representing whether user meets audience conditions to be in experiment or not. | ||
*/ | ||
public static function isUserInExperiment($config, $experiment, $userAttributes, $logger) | ||
public static function doesUserMeetAudienceConditions($config, $experiment, $userAttributes, $logger, $isRollout = null, $rolloutRule = null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of passing |
||
{ | ||
$loggingClass = 'Optimizely\Enums\ExperimentAudienceEvaluationLogs'; | ||
$loggingStr = $experiment->getKey(); | ||
|
||
if ($isRollout) { | ||
$loggingClass = 'Optimizely\Enums\RolloutAudienceEvaluationLogs'; | ||
$loggingStr = $rolloutRule; | ||
} | ||
|
||
$audienceConditions = $experiment->getAudienceConditions(); | ||
if ($audienceConditions === null) { | ||
$audienceConditions = $experiment->getAudienceIds(); | ||
} | ||
|
||
$logger->log(Logger::DEBUG, sprintf( | ||
AudienceEvaluationLogs::EVALUATING_AUDIENCES_COMBINED, | ||
$experiment->getKey(), | ||
$loggingClass::EVALUATING_AUDIENCES_COMBINED, | ||
$loggingStr, | ||
json_encode($audienceConditions) | ||
)); | ||
|
||
// Return true if experiment is not targeted to any audience. | ||
if (empty($audienceConditions)) { | ||
$logger->log(Logger::INFO, sprintf( | ||
AudienceEvaluationLogs::AUDIENCE_EVALUATION_RESULT_COMBINED, | ||
$experiment->getKey(), | ||
$loggingClass::AUDIENCE_EVALUATION_RESULT_COMBINED, | ||
$loggingStr, | ||
'TRUE' | ||
)); | ||
return true; | ||
|
@@ -171,14 +182,14 @@ public static function isUserInExperiment($config, $experiment, $userAttributes, | |
return $customAttrCondEval->evaluate($leafCondition); | ||
}; | ||
|
||
$evaluateAudience = function ($audienceId) use ($config, $evaluateCustomAttr, $logger) { | ||
$evaluateAudience = function ($audienceId) use ($config, $evaluateCustomAttr, $logger, $loggingClass) { | ||
$audience = $config->getAudience($audienceId); | ||
if ($audience === null) { | ||
return null; | ||
} | ||
|
||
$logger->log(Logger::DEBUG, sprintf( | ||
AudienceEvaluationLogs::EVALUATING_AUDIENCE, | ||
$loggingClass::EVALUATING_AUDIENCE, | ||
$audienceId, | ||
json_encode($audience->getConditionsList()) | ||
)); | ||
|
@@ -188,7 +199,7 @@ public static function isUserInExperiment($config, $experiment, $userAttributes, | |
$resultStr = $result === null ? 'UNKNOWN' : strtoupper(var_export($result, true)); | ||
|
||
$logger->log(Logger::DEBUG, sprintf( | ||
AudienceEvaluationLogs::AUDIENCE_EVALUATION_RESULT, | ||
$loggingClass::AUDIENCE_EVALUATION_RESULT, | ||
$audienceId, | ||
$resultStr | ||
)); | ||
|
@@ -201,8 +212,8 @@ public static function isUserInExperiment($config, $experiment, $userAttributes, | |
$evalResult = $evalResult || false; | ||
|
||
$logger->log(Logger::INFO, sprintf( | ||
AudienceEvaluationLogs::AUDIENCE_EVALUATION_RESULT_COMBINED, | ||
$experiment->getKey(), | ||
$loggingClass::AUDIENCE_EVALUATION_RESULT_COMBINED, | ||
$loggingStr, | ||
strtoupper(var_export($evalResult, true)) | ||
)); | ||
|
||
|
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.
nit.
$i + 1
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 comment is still not addressed.