Skip to content

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

Merged
merged 13 commits into from
Jul 21, 2020
15 changes: 3 additions & 12 deletions src/Optimizely/Bucketer.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* Copyright 2016-2019, Optimizely
* Copyright 2016-2020, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -133,7 +133,7 @@ private function findBucket($bucketingId, $userId, $parentId, $trafficAllocation
* Determine variation the user should be put in.
*
* @param $config ProjectConfigInterface Configuration for the project.
* @param $experiment Experiment Experiment in which user is to be bucketed.
* @param $experiment Experiment Experiment or Rollout rule in which user is to be bucketed.
* @param $bucketingId string A customer-assigned value used to create the key for the murmur hash.
* @param $userId string User identifier.
*
Expand All @@ -146,6 +146,7 @@ public function bucket(ProjectConfigInterface $config, Experiment $experiment, $
}

// Determine if experiment is in a mutually exclusive group.
// This will not affect evaluation of rollout rules.
if ($experiment->isInMutexGroup()) {
$group = $config->getGroup($experiment->getGroupId());

Expand Down Expand Up @@ -188,19 +189,9 @@ public function bucket(ProjectConfigInterface $config, Experiment $experiment, $
if (!empty($variationId)) {
$variation = $config->getVariationFromId($experiment->getKey(), $variationId);

$this->_logger->log(
Logger::INFO,
sprintf(
'User "%s" is in variation %s of experiment %s.',
$userId,
$variation->getKey(),
$experiment->getKey()
)
);
return $variation;
}

$this->_logger->log(Logger::INFO, sprintf('User "%s" is in no variation.', $userId));
return null;
}
}
38 changes: 25 additions & 13 deletions src/Optimizely/DecisionService/DecisionService.php
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.
Expand Down Expand Up @@ -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())
Expand All @@ -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;
}

Expand Down Expand Up @@ -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, 'Optimizely\Enums\RolloutAudienceEvaluationLogs', $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)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. $i + 1

Copy link
Contributor

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.

);
// 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];

// 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, 'Optimizely\Enums\RolloutAudienceEvaluationLogs', '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;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* Copyright 2019, Optimizely
* Copyright 2019-2020, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -17,11 +17,9 @@

namespace Optimizely\Enums;

class AudienceEvaluationLogs
class CommonAudienceEvaluationLogs
{
const AUDIENCE_EVALUATION_RESULT = "Audience \"%s\" evaluated to %s.";
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 because the number value 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\".";
Expand Down
24 changes: 24 additions & 0 deletions src/Optimizely/Enums/ExperimentAudienceEvaluationLogs.php
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.";
}
24 changes: 24 additions & 0 deletions src/Optimizely/Enums/RolloutAudienceEvaluationLogs.php
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.";
}
13 changes: 6 additions & 7 deletions src/Optimizely/Optimizely.php
Original file line number Diff line number Diff line change
Expand Up @@ -931,7 +931,7 @@ private function getFeatureVariableValueFromVariation($featureFlagKey, $variable
if ($variation === null) {
$this->_logger->log(
Logger::INFO,
"User '{$userId}'is not in any variation, ".
"User '{$userId}' is not in experiment or rollout, ".
"returning default value '{$variableValue}'."
);
} else {
Expand All @@ -941,21 +941,20 @@ private function getFeatureVariableValueFromVariation($featureFlagKey, $variable
$variableValue = $variableUsage->getValue();
$this->_logger->log(
Logger::INFO,
"Returning variable value '{$variableValue}' for variation '{$variation->getKey()}' ".
"of feature flag '{$featureFlagKey}'"
"Returning variable value '{$variableValue}' for variable key '{$variableKey}' ".
"of feature flag '{$featureFlagKey}'."
);
} else {
$this->_logger->log(
Logger::INFO,
"Variable '{$variableKey}' is not used in variation '{$variation->getKey()}', ".
"returning default value '{$variableValue}'."
"Variable value is not defined. Returning the default variable value '{$variableValue}'."
);
}
} else {
$this->_logger->log(
Logger::INFO,
"Feature '{$featureFlagKey}' for variation '{$variation->getKey()}' is not enabled, ".
"returning default value '{$variableValue}'."
"Feature '{$featureFlagKey}' is not enabled for user '{$userId}'. ".
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to fix the messaging on line 934, 944, 949 as well. They all talk about variations.

On line 934, change to User '{$userId}'is not in experiment or rollout, returning default value '{$variableValue}'
On line 944, change to Returning variable value '{$variableValue}' for variable key <get variable key here> of feature flag {$featureFlagKey}
On line 949, change to `Variable value is not defined. Returning the default variable value '{$variableValue}'"

"Returning the default variable value '{$variableValue}'."
);
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/Optimizely/Utils/CustomAttributeConditionEvaluator.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* Copyright 2018-2019, Optimizely
* Copyright 2018-2020, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -18,7 +18,7 @@
namespace Optimizely\Utils;

use Monolog\Logger;
use Optimizely\Enums\AudienceEvaluationLogs as logs;
use Optimizely\Enums\CommonAudienceEvaluationLogs as logs;
use Optimizely\Utils\Validator;

class CustomAttributeConditionEvaluator
Expand Down
33 changes: 21 additions & 12 deletions src/Optimizely/Utils/Validator.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
use Monolog\Logger;
use Optimizely\Config\ProjectConfigInterface;
use Optimizely\Entity\Experiment;
use Optimizely\Enums\AudienceEvaluationLogs;
use Optimizely\Logger\LoggerInterface;
use Optimizely\Utils\ConditionTreeEvaluator;
use Optimizely\Utils\CustomAttributeConditionEvaluator;
Expand Down Expand Up @@ -133,30 +132,40 @@ public static function areEventTagsValid($eventTags)

/**
* @param $config ProjectConfigInterface Configuration for the project.
* @param $experiment Experiment Entity representing the experiment.
* @param $experiment Experiment Entity representing the experiment or rollout rule.
* @param $userAttributes array Attributes of the user.
* @param $logger LoggerInterface.
* @param $loggingClass String Class holding log strings with placeholders.
* @param $loggingKey String Identifier of an experiment/rollout rule.
*
* @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, $loggingClass = null, $loggingKey = null)
{
if ($loggingClass === null) {
$loggingClass = 'Optimizely\Enums\ExperimentAudienceEvaluationLogs';
}

if ($loggingKey === null) {
$loggingKey = $experiment->getKey();
}
Comment on lines +145 to +151
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice !!


$audienceConditions = $experiment->getAudienceConditions();
if ($audienceConditions === null) {
$audienceConditions = $experiment->getAudienceIds();
}

$logger->log(Logger::DEBUG, sprintf(
AudienceEvaluationLogs::EVALUATING_AUDIENCES_COMBINED,
$experiment->getKey(),
$loggingClass::EVALUATING_AUDIENCES_COMBINED,
$loggingKey,
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,
$loggingKey,
'TRUE'
));
return true;
Expand All @@ -171,14 +180,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())
));
Expand All @@ -188,7 +197,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
));
Expand All @@ -201,8 +210,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,
$loggingKey,
strtoupper(var_export($evalResult, true))
));

Expand Down
Loading