fix(logs): Fixing log messages #201

oakbani · 2020-06-24T07:14:44Z

Summary

This PR modifies log messages to be explicit when it's evaluating an experiment and when it's doing so for a Rollout.

Test plan

Added additional unit test to assert audience logs for rollouts.
Existing tests should continue to pass.

coveralls · 2020-06-24T07:29:41Z

Coverage increased (+0.04%) to 98.256% when pulling 107e914 on oakbani/fix-targeted-rollouts-log into 3f53ed9 on master.

aliabbasrizvi · 2020-06-26T19:08:18Z

src/Optimizely/DecisionService/DecisionService.php

                $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)


nit. $i + 1

This comment is still not addressed.

aliabbasrizvi · 2020-06-26T19:10:07Z

src/Optimizely/Utils/Validator.php

     *
     * @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 isUserInExperiment($config, $experiment, $userAttributes, $logger, $isRollout = null, $rolloutRule = null)


This method name needs to be updated. It should be called doesUserMeetAudienceConditions.

aliabbasrizvi · 2020-06-26T19:11:45Z

src/Optimizely/Enums/AudienceEvaluationLogs.php

+    const AUDIENCE_EVALUATION_RESULT_COMBINED = "Audiences for %s collectively evaluated to %s.";
+    const EVALUATING_AUDIENCES_COMBINED = "Evaluating audiences for %s: %s.";


I'd prefer if we break out the log messages just so that we have greater control for later changes.

aliabbasrizvi

Almost there.

aliabbasrizvi · 2020-06-30T16:49:03Z

src/Optimizely/Utils/Validator.php

     *
     * @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)


Instead of passing isRollout, you can just pass the loggingClass.

aliabbasrizvi · 2020-06-30T16:49:17Z

src/Optimizely/Utils/Validator.php

@@ -136,27 +137,37 @@ public static function areEventTagsValid($eventTags)
     * @param $experiment Experiment Entity representing the experiment.


representing the experiment or rollout rule

aliabbasrizvi · 2020-06-30T16:50:13Z

src/Optimizely/Utils/Validator.php

@@ -136,27 +137,37 @@ public static function areEventTagsValid($eventTags)
     * @param $experiment Experiment Entity representing the experiment.
     * @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.


You can just pass in the experiment key or rule index and just log that here.

oakbani · 2020-07-01T09:19:54Z

@aliabbasrizvi Assuming experiment class and experiment key by default to avoid unnecessarily modifying many unit tests. Since, this is an internal method, shouldn't make a difference. Let me know if you would like me to explicitly pass arguments when evaluating experiment.

…ts-log

aliabbasrizvi

Mostly looks good. Some small changes needed.

aliabbasrizvi · 2020-07-09T22:35:24Z

src/Optimizely/DecisionService/DecisionService.php

                $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)


This comment is still not addressed.

aliabbasrizvi · 2020-07-09T22:36:09Z

src/Optimizely/DecisionService/DecisionService.php

            }
            break;
        }
        // Evaluate Everyone Else Rule / Last Rule now
-        $experiment = $rolloutRules[sizeof($rolloutRules)-1];
+        $rolloutRule = $rolloutRules[sizeof($rolloutRules)-1];


Is it conventional to not have spacing around operators?

Not something enforced. Subjective

aliabbasrizvi · 2020-07-09T22:37:02Z

src/Optimizely/Utils/Validator.php

+        if ($loggingClass === null) {
+            $loggingClass = 'Optimizely\Enums\ExperimentAudienceEvaluationLogs';
+        }
+
+        if ($loggingKey === null) {
+            $loggingKey = $experiment->getKey();
+        }


aliabbasrizvi · 2020-07-09T22:38:28Z

tests/UtilsTests/ValidatorTest.php

@@ -188,7 +188,7 @@ public function testAreEventTagsValidInvalidEventTags()
    }

    // test that Audience evaluation proceeds if provided attributes are empty or null.
-    public function testIsUserInExperimentAudienceUsedInExperimentNoAttributesProvided()
+    public function testdoesUserMeetAudienceConditionsAudienceUsedInExperimentNoAttributesProvided()


nit. Does and not does.

aliabbasrizvi · 2020-07-09T22:38:39Z

tests/UtilsTests/ValidatorTest.php

-    // test that isUserInExperiment returns true when no audience is attached to experiment.
-    public function testIsUserInExperimentNoAudienceUsedInExperiment()
+    // test that doesUserMeetAudienceConditions returns true when no audience is attached to experiment.
+    public function testdoesUserMeetAudienceConditionsNoAudienceUsedInExperiment()


Same comment as above

You probably need to update this in all the tests.

aliabbasrizvi

Some more comments around variable logging.

aliabbasrizvi · 2020-07-09T22:48:01Z

src/Optimizely/Optimizely.php

@@ -954,8 +954,8 @@ private function getFeatureVariableValueFromVariation($featureFlagKey, $variable
            } 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}'. ".


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}'"

aliabbasrizvi

LGTM

oakbani added 3 commits June 24, 2020 09:59

relocate bucketer log messages

e32e2b8

refact: Fix log messages for rollout rule

2b32343

update headers

ef92697

oakbani requested a review from a team as a code owner June 24, 2020 07:14

optimizely-codereview-bot assigned oakbani Jun 24, 2020

oakbani removed their assignment Jun 24, 2020

oakbani requested a review from aliabbasrizvi June 24, 2020 07:15

add docs

1332498

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs 🤖 16 hrs and removed 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs labels Jun 24, 2020

aliabbasrizvi suggested changes Jun 26, 2020

View reviewed changes

optimizely-codereview-bot removed the 🤖 16 hrs label Jun 26, 2020

optimizely-codereview-bot assigned oakbani Jun 26, 2020

oakbani added 3 commits June 30, 2020 18:05

Merge branch 'master' into oakbani/fix-targeted-rollouts-log

2617b03

rename isUserInExperiment

5481ce4

break up log messages

45e7ed8

oakbani requested a review from aliabbasrizvi June 30, 2020 14:11

oakbani removed their assignment Jun 30, 2020

aliabbasrizvi suggested changes Jun 30, 2020

View reviewed changes

optimizely-codereview-bot assigned oakbani Jun 30, 2020

address comments

16b4f6d

oakbani removed their assignment Jul 1, 2020

optimizely-codereview-bot requested a review from aliabbasrizvi July 1, 2020 09:20

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs 🤖 16 hrs 🤖 20 hrs 🤖 24 hrs and removed 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs 🤖 16 hrs 🤖 20 hrs labels Jul 1, 2020

oakbani added 2 commits July 8, 2020 18:51

first comment

6b9a7a6

Merge branch 'oakbani/feat-var-logs' into oakbani/fix-targeted-rollou…

8a0264f

…ts-log

oakbani changed the title ~~fix(logs): Fixing log messages for Targeted Rollouts~~ fix(logs): Fixing log messages Jul 8, 2020

aliabbasrizvi suggested changes Jul 9, 2020

View reviewed changes

optimizely-codereview-bot removed the 🤖 24 hrs label Jul 9, 2020

optimizely-codereview-bot assigned oakbani Jul 9, 2020

aliabbasrizvi reviewed Jul 9, 2020

View reviewed changes

oakbani added 2 commits July 10, 2020 11:47

nits

ea3fa48

update logs

f4b577e

oakbani requested a review from aliabbasrizvi July 10, 2020 07:19

oakbani removed their assignment Jul 10, 2020

Merge branch 'master' into oakbani/fix-targeted-rollouts-log

107e914

aliabbasrizvi approved these changes Jul 21, 2020

View reviewed changes

aliabbasrizvi merged commit efa480f into master Jul 21, 2020

aliabbasrizvi deleted the oakbani/fix-targeted-rollouts-log branch July 21, 2020 01:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(logs): Fixing log messages #201

fix(logs): Fixing log messages #201

oakbani commented Jun 24, 2020

coveralls commented Jun 24, 2020 •

edited

Loading

aliabbasrizvi Jun 26, 2020

aliabbasrizvi Jul 9, 2020

aliabbasrizvi Jun 26, 2020

aliabbasrizvi Jun 26, 2020

aliabbasrizvi left a comment

aliabbasrizvi Jun 30, 2020

aliabbasrizvi Jun 30, 2020

aliabbasrizvi Jun 30, 2020

oakbani commented Jul 1, 2020

aliabbasrizvi left a comment

aliabbasrizvi Jul 9, 2020

aliabbasrizvi Jul 9, 2020

oakbani Jul 10, 2020

aliabbasrizvi Jul 9, 2020

aliabbasrizvi Jul 9, 2020

aliabbasrizvi Jul 9, 2020

aliabbasrizvi Jul 9, 2020

aliabbasrizvi left a comment

aliabbasrizvi Jul 9, 2020

aliabbasrizvi left a comment

		const AUDIENCE_EVALUATION_RESULT_COMBINED = "Audiences for %s collectively evaluated to %s.";
		const EVALUATING_AUDIENCES_COMBINED = "Evaluating audiences for %s: %s.";

		@@ -136,27 +137,37 @@ public static function areEventTagsValid($eventTags)
		* @param $experiment Experiment Entity representing the experiment.

fix(logs): Fixing log messages #201

fix(logs): Fixing log messages #201

Conversation

oakbani commented Jun 24, 2020

Summary

Test plan

coveralls commented Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oakbani commented Jul 1, 2020

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliabbasrizvi left a comment

Choose a reason for hiding this comment

coveralls commented Jun 24, 2020 •

edited

Loading