Feature Flag & Rollouts - Decision Service Logic #69

oakbani · 2017-10-26T13:21:26Z

👂 Will also show changes of data models and file parsing until first PR is merged
🖊 getVariationForFeature and Unit Tests

commit 75224a4 Author: Owais <oakbani@folio3.com> Date: Thu Oct 26 18:14:31 2017 +0500 :memo: Final changes commit 75fa44d Author: Owais <oakbani@folio3.com> Date: Thu Oct 26 16:29:35 2017 +0500 # Conflicts: # tests/ProjectConfigTest.php # tests/UtilsTests/ValidatorTest.php commit a443056 Author: Owais <oakbani@folio3.com> Date: Thu Oct 26 12:21:16 2017 +0500 Partial fixes commit e03926b Author: Owais <oakbani@folio3.com> Date: Tue Oct 24 17:07:39 2017 +0500 Squashed commit of the following: commit 24569b8 Author: Owais <oakbani@folio3.com> Date: Fri Oct 20 18:18:55 2017 +0500 :pen: Fixed Code Coverage commit a6fa1e6 Author: Owais <oakbani@folio3.com> Date: Tue Oct 24 16:46:44 2017 +0500 :pen2: All Unit tests completed commit 72173d0 Author: Owais <oakbani@folio3.com> Date: Mon Oct 23 20:36:21 2017 +0500 :pen: 90% Unit tests done commit 9051dda Author: Owais <oakbani@folio3.com> Date: Mon Oct 23 18:41:04 2017 +0500 :pen: Unit tests done for feature experiment commit 8a5722d Author: Owais <oakbani@folio3.com> Date: Mon Oct 23 13:13:53 2017 +0500 :pen: Implementation First pass done commit 92dd219 Author: Owais <oakbani@folio3.com> Date: Fri Oct 20 17:32:04 2017 +0500 :pen: Feature Flag bucketing to Experiment done :memo: Todo: Rollout Logic commit ad15a2c Author: Owais <oakbani@folio3.com> Date: Fri Oct 20 12:52:47 2017 +0500 :pen: indentation commit dc41bf9 Author: Owais <oakbani@folio3.com> Date: Thu Oct 19 17:36:46 2017 +0500 :pencil2: Added Logger to Validator :pencil2: Added method defs commit 547b7ad Author: Owais <oakbani@folio3.com> Date: Thu Oct 19 17:11:22 2017 +0500 Feature Flags models and parsing from new v4 file

optibot · 2017-10-26T13:21:27Z

Can one of the admins verify this patch?

coveralls · 2017-10-26T13:25:58Z

Coverage increased (+0.4%) to 93.315% when pulling 1253af6 on msohailhussain:getVariationForFeature into e60d1c5 on optimizely:master.

coveralls · 2017-10-26T13:25:58Z

Coverage increased (+0.4%) to 93.315% when pulling 1253af6 on msohailhussain:getVariationForFeature into e60d1c5 on optimizely:master.

msohailhussain · 2017-10-26T19:42:08Z

@asaschachar Can you please check for reviewer.

aliabbasrizvi · 2017-10-27T04:51:52Z

src/Optimizely/DecisionService/DecisionService.php

 use Optimizely\UserProfile\UserProfileUtils;
 use Optimizely\Utils\Validator;
+use Optimizely\Entity\FeatureFlag;
+use Optimizely\Entity\Rollout;


nit. Alphabetize

aliabbasrizvi · 2017-10-27T04:52:24Z

src/Optimizely/DecisionService/DecisionService.php

+  /**
+   * Gets the Bucketing Id for Bucketing
+   * @param  string $userId         
+   * @param  array $userAttributes 


nit. Space above this line between params and return

aliabbasrizvi · 2017-10-27T04:52:28Z

src/Optimizely/DecisionService/DecisionService.php

    return $variation;
  }

+  /**


aliabbasrizvi · 2017-10-27T04:54:07Z

src/Optimizely/DecisionService/DecisionService.php

+   * @return array/null  {"experiment" : Experiment, "variation": Variation } / null
+   */
+  public function getVariationForFeature(FeatureFlag $featureFlag, $userId, $userAttributes){
+    //Evaluate in this order: 


nit. At any point in time user will only be part of 1 experiment. A more accurate statement will be Attempt to bucket user into experiment using feature flag

aliabbasrizvi · 2017-10-27T04:54:24Z

src/Optimizely/DecisionService/DecisionService.php

+   */
+  public function getVariationForFeature(FeatureFlag $featureFlag, $userId, $userAttributes){
+    //Evaluate in this order: 
+    //1. Attempt to bucket user into all experiments in the feature flag.


nit. ...rollout using the feature...

aliabbasrizvi · 2017-10-27T04:54:48Z

src/Optimizely/DecisionService/DecisionService.php

+   * @param  array $userAttributes 
+   * @return string  
+   */
+  private function getBucketingId($userId, $userAttributes){


aliabbasrizvi · 2017-10-27T17:15:15Z

src/Optimizely/DecisionService/DecisionService.php

+      $this->_logger->log(Logger::INFO,
+        "User '{$userId}' is bucketed into a rollout for feature flag '{$featureFlag->getKey()}'."
+      );
+


I would request you to model this more as how it is in the Python SDK to be explicit that an event needs to be sent or not. See here: https://github.com/optimizely/python-sdk/blob/master/optimizely/decision_service.py#L195

🖊️ Logic slightly changed based on Decision 🖊️ nits addressed

oakbani · 2017-10-30T11:16:38Z

@aliabbasrizvi Decision Object added and logic modified. Let me know if there is something else

coveralls · 2017-10-30T11:25:25Z

Coverage increased (+0.2%) to 93.052% when pulling 79788ee on msohailhussain:getVariationForFeature into e60d1c5 on optimizely:master.

# Conflicts: # src/Optimizely/Entity/Variation.php # src/Optimizely/ProjectConfig.php # src/Optimizely/Utils/Validator.php # tests/DecisionServiceTests/DecisionServiceTest.php # tests/ProjectConfigTest.php # tests/TestData.php

…ationForFeature # Conflicts: # src/Optimizely/Entity/Variation.php # src/Optimizely/ProjectConfig.php # src/Optimizely/Utils/Validator.php # tests/ProjectConfigTest.php # tests/TestData.php

coveralls · 2017-11-01T12:26:33Z

Coverage increased (+0.6%) to 93.052% when pulling b6fc3d2 on msohailhussain:getVariationForFeature into 258ce17 on optimizely:master.

oakbani · 2017-11-06T13:19:07Z

@aliabbasrizvi Let me know if there is anything else

aliabbasrizvi

Multiple fixes needed for styling. Please use other files in the repo as an example for how styling should be done.

aliabbasrizvi · 2017-11-07T01:33:02Z

src/Optimizely/DecisionService/Decision.php

+class Decision
+{
+	const DECISION_SOURCE_EXPERIMENT = 'experiment';
+	const DECISION_SOURCE_ROLLOUT = 'rollout';


Commenting from https://github.com/optimizely/php-sdk/pull/71/files but please adjust indentation in this file. It looks like there is 5 spaces here and 2 spaces else where whereas we should be at 4 spaces.

aliabbasrizvi · 2017-11-07T01:34:02Z

src/Optimizely/DecisionService/DecisionService.php


+  /**
+   * Gets the Bucketing ID for Bucketing
+   * @param  string $userId         


nit. Please add more description for the 2 fields by looking else where

aliabbasrizvi · 2017-11-07T01:34:16Z

src/Optimizely/DecisionService/DecisionService.php

+   * Gets the Bucketing ID for Bucketing
+   * @param  string $userId         
+   * @param  array $userAttributes
+   * 


nit. Describe what is actually returned.

aliabbasrizvi · 2017-11-07T01:34:35Z

src/Optimizely/DecisionService/DecisionService.php

+   * Get the variation the user is bucketed into for the given FeatureFlag
+   * @param  FeatureFlag $featureFlag The feature flag the user wants to access
+   * @param  string      $userId      user id
+   * @param  array       $userAttributes  user attributes


nit. Like above, these comments can be more descriptive.

Mention when null is returned and when decision is

aliabbasrizvi · 2017-11-07T01:38:29Z

src/Optimizely/DecisionService/DecisionService.php

+    // Check if the feature flag has rollout and the user is bucketed into one of it's rules
+    $decision = $this->getVariationForFeatureRollout($featureFlag, $userId, $userAttributes);
+    if($decision){
+      $this->_logger->log(Logger::INFO,


I think the comments in Python SDK are more appropriate. Something like: https://github.com/optimizely/python-sdk/blob/master/optimizely/decision_service.py#L190

Perhaps, be User '{$userId} meets conditions for a targeting rule of rollout for feature flag {$featureFlag->getKey()}

The placement of log messages in python has been done differently. I have mainly followed Ruby and Java messages in this change.
'{$userId} meets conditions for a targeting rule of rollout for feature flag {$featureFlag->getKey()}
This message fits when the user meets the audience conditions for the rollout rule. That's not we intend to say here.

aliabbasrizvi · 2017-11-07T01:54:13Z

src/Optimizely/DecisionService/DecisionService.php

+    return null;
+  }
+
+   /**


There is only 1 rollout, not one of the rollouts.

aliabbasrizvi · 2017-11-07T02:39:11Z

src/Optimizely/DecisionService/DecisionService.php

+    if(sizeof($rolloutRules) == 0)
+      return null;
+
+    // Evaluate all rollout rules except for last one


Not particularly fond of the spacing here. Please add spaces around the operators

aliabbasrizvi · 2017-11-07T02:39:57Z

src/Optimizely/DecisionService/DecisionService.php

+        sprintf("Attempting to bucket user '{$userId}' into rollout rule '%s'.", $experiment->getKey()));
+
+      // Evaluate if user satisfies the traffic allocation for this rollout rule
+      $variation = $this->_bucketer->bucket($this->_projectConfig, $experiment, $bucketing_id, $userId);


Similar comment as above

aliabbasrizvi · 2017-11-07T02:40:17Z

src/Optimizely/DecisionService/DecisionService.php

+        return new Decision($experiment->getId(), $variation->getId(), DECISION::DECISION_SOURCE_ROLLOUT);
+      } else {
+        $this->_logger->log(Logger::DEBUG,
+         "User '{$userId}' was excluded due to traffic allocation. Checking 'Eveyrone Else' rule now.");


Align the b of break with "

aliabbasrizvi · 2017-11-07T02:40:36Z

src/Optimizely/DecisionService/DecisionService.php

+    if($variation && $variation != new Variation()){
+        return new Decision($experiment->getId(), $variation->getId(), DECISION::DECISION_SOURCE_ROLLOUT);
+      } else {
+        $this->_logger->log(Logger::DEBUG,


Too many spaces before was

coveralls · 2017-11-07T13:00:44Z

Coverage increased (+0.7%) to 93.148% when pulling 2abf1ad on msohailhussain:getVariationForFeature into 258ce17 on optimizely:master.

oakbani · 2017-11-08T08:14:21Z

@aliabbasrizvi Addressed styling issues 2abf1ad.

aliabbasrizvi

Please add unit tests. Are they added in PR 71?

aliabbasrizvi · 2017-11-09T02:26:38Z

src/Optimizely/DecisionService/DecisionService.php

+     *
+     * @return string  the bucketing ID assigned to user
+     */
+    private function getBucketingId($userId, $userAttributes)


nit. Please place this method on line 86 i.e. above getVariation. We should try and put private functions together as well as functions being referenced above functions they are being referenced in.

aliabbasrizvi · 2017-11-09T02:26:55Z

src/Optimizely/DecisionService/DecisionService.php

  }

+    /**
+     * Gets the Bucketing ID for Bucketing


Gets the ID for bucketing

aliabbasrizvi · 2017-11-09T02:30:03Z

src/Optimizely/DecisionService/DecisionService.php

+
+            return $decision;
+        } else {
+            $this->_logger->log(


nit. Just a note on styling. I hope this suggestion will help improve readability, but we can remove the else clause and all statements i.e. line 197 - 202 will be executed anyway. That way, I do not have to figure out when the else is triggered and just by looking at the function I know if the else is not met, it will end up returning null

That will be similar to what you do in the next function i.e. getVariationForFeatureExperiment

Right. Thanks for pointing it out.

aliabbasrizvi · 2017-11-09T02:31:12Z

src/Optimizely/DecisionService/DecisionService.php

+    {
+        $feature_flag_key = $featureFlag->getKey();
+        $experimentIds = $featureFlag->getExperimentIds();
+        //Check if there are any experiment ids inside feature flag


nit. Inset empty line above this statement.

aliabbasrizvi · 2017-11-09T02:34:33Z

src/Optimizely/DecisionService/Decision.php

+ */
+namespace Optimizely\DecisionService;
+
+class Decision


I see the problem with Decision being a thing in UserProfile also. Please rename this to FeatureDecisionto make the distinction.

aliabbasrizvi · 2017-11-09T02:34:50Z

src/Optimizely/DecisionService/DecisionService.php

    $variationId = $variation->getId();
    if (is_null($decision)) {
-        $decision = new Decision($variationId);
+        $decision = new \Optimizely\UserProfile\Decision($variationId);


Lets avoid the name conflict and rename the new thing to FeatureDecision

aliabbasrizvi · 2017-11-09T02:35:27Z

src/Optimizely/DecisionService/DecisionService.php

+     * @return Decision  if a variation is returned for the user
+     *         null  if feature flag is not used in any experiments or no variation is returned for the user
+     */
+    public function getVariationForFeatureExperiment(FeatureFlag $featureFlag, $userId, $userAttributes)


Unit tests missing for this

Please see them here : https://github.com/msohailhussain/php-sdk/blob/2abf1adbf9e25e9cfdcbb90af02431b41a19d71a/tests/DecisionServiceTests/DecisionServiceTest.php#L611-#L734

aliabbasrizvi · 2017-11-09T02:35:31Z

src/Optimizely/DecisionService/DecisionService.php

+     * @return Decision  if getVariationForFeatureExperiment or getVariationForFeatureRollout returns a Decision
+     *         null      otherwise
+     */
+    public function getVariationForFeature(FeatureFlag $featureFlag, $userId, $userAttributes)


Unit tests missing for this

They are in place. Please see them here: https://github.com/msohailhussain/php-sdk/blob/2abf1adbf9e25e9cfdcbb90af02431b41a19d71a/tests/DecisionServiceTests/DecisionServiceTest.php#L736-#L824

…ision, repositioned getBucketingId

coveralls · 2017-11-09T06:04:05Z

Coverage increased (+0.6%) to 93.142% when pulling eb54a9d on msohailhussain:getVariationForFeature into 258ce17 on optimizely:master.

oakbani · 2017-11-09T06:04:26Z

@aliabbasrizvi Please see the changes here. eb54a9d
Thanks

aliabbasrizvi

Please address remaining styling comments before merging. Please be consistent in spacing in comments.

aliabbasrizvi · 2017-11-10T06:30:21Z

src/Optimizely/DecisionService/DecisionService.php

+  {
+      // By default, the bucketing ID should be the user ID
+      $bucketingId = $userId;
+


nit. Line seems too long. Please break up into 2 lines.

aliabbasrizvi · 2017-11-10T06:31:28Z

src/Optimizely/DecisionService/DecisionService.php

+        $feature_flag_key = $featureFlag->getKey();
+        $experimentIds = $featureFlag->getExperimentIds();
+
+        //Check if there are any experiment ids inside feature flag


Also, please be consistent in commenting style. Some places like here there is no space after //and then on line 229 there is space. The spacing is correct on line 229.

aliabbasrizvi · 2017-11-10T06:31:48Z

src/Optimizely/DecisionService/DecisionService.php

+            return null;
+        }
+
+        // Evaluate each experiment id and return the first bucketed experiment variation


aliabbasrizvi · 2017-11-10T06:33:20Z

src/Optimizely/DecisionService/DecisionService.php

+    }
+
+    /**
+    * Get the variation if the user is bucketed into rollout on this feature flag


nit. rollout for this feature flag.

aliabbasrizvi · 2017-11-10T06:34:36Z

src/Optimizely/Utils/Validator.php

                foreach ($validator->getErrors() as $error) {
                   $logger->log(Logger::DEBUG, "[%s] %s\n", $error['property'], $error['message']);
-               }
+


Seems unnecessary

coveralls · 2017-11-10T07:47:54Z

Coverage increased (+0.6%) to 93.142% when pulling e331e42 on msohailhussain:getVariationForFeature into 258ce17 on optimizely:master.

msohailhussain · 2017-11-13T21:20:06Z

@wangjoshuah @aliabbasrizvi if anything is not pending, can u plz merge this PR.

wangjoshuah · 2017-11-13T21:51:28Z

build

wangjoshuah · 2017-11-13T21:51:36Z

build

coveralls · 2017-11-13T21:55:03Z

Coverage increased (+0.6%) to 93.142% when pulling 7985112 on msohailhussain:getVariationForFeature into 38f33df on optimizely:master.

wangjoshuah · 2017-11-13T21:55:46Z

build

wangjoshuah · 2017-11-14T19:32:26Z

build

aliabbasrizvi suggested changes Oct 27, 2017

View reviewed changes

🖊️ Decision Object introduced

79788ee

🖊️ Logic slightly changed based on Decision 🖊️ nits addressed

oakbani added 2 commits November 1, 2017 17:00

Merge Master

52f6960

# Conflicts: # src/Optimizely/Entity/Variation.php # src/Optimizely/ProjectConfig.php # src/Optimizely/Utils/Validator.php # tests/DecisionServiceTests/DecisionServiceTest.php # tests/ProjectConfigTest.php # tests/TestData.php

Merge remote-tracking branch 'remotes/optimizely/master' into getVari…

b6fc3d2

…ationForFeature # Conflicts: # src/Optimizely/Entity/Variation.php # src/Optimizely/ProjectConfig.php # src/Optimizely/Utils/Validator.php # tests/ProjectConfigTest.php # tests/TestData.php

aliabbasrizvi suggested changes Nov 7, 2017

View reviewed changes

🖊️ Styling issues addressed and minor refactoring

2abf1ad

oakbani mentioned this pull request Nov 7, 2017

Is Feature Enabled and Feature Variable Accessor APIs #71

Merged

aliabbasrizvi suggested changes Nov 9, 2017

View reviewed changes

📝 Addressed all nits, renamed DecisionService Decision to Feature Dec…

eb54a9d

…ision, repositioned getBucketingId

aliabbasrizvi approved these changes Nov 10, 2017

View reviewed changes

📝 Final styling issues addressed

e331e42

Merge branch 'master' into getVariationForFeature

7985112

wangjoshuah merged commit afa7a55 into optimizely:master Nov 14, 2017

oakbani deleted the getVariationForFeature branch November 30, 2017 02:53

Feature Flag & Rollouts - Decision Service Logic #69

Feature Flag & Rollouts - Decision Service Logic #69

Uh oh!

Conversation

oakbani commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

optibot commented Oct 26, 2017

Uh oh!

coveralls commented Oct 26, 2017

Uh oh!

coveralls commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msohailhussain commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oakbani commented Oct 30, 2017

Uh oh!

coveralls commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oakbani commented Nov 6, 2017

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oakbani commented Nov 8, 2017

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

oakbani commented Oct 26, 2017 •

edited

Loading

coveralls commented Oct 26, 2017 •

edited

Loading

coveralls commented Oct 30, 2017 •

edited

Loading

coveralls commented Nov 1, 2017 •

edited

Loading

coveralls commented Nov 7, 2017 •

edited

Loading

coveralls commented Nov 9, 2017 •

edited

Loading