implements new decision structure (#74)

Author: rashidsp

lib/optimizely.rb

@@ -260,9 +260,9 @@ def is_feature_enabled(feature_flag_key, user_id, attributes = nil)
       decision = @decision_service.get_variation_for_feature(feature_flag, user_id, attributes)
       unless decision.nil?
         variation = decision['variation']
-        experiment = decision['experiment']
-        unless experiment.nil?
-          send_impression(experiment, variation['key'], user_id, attributes)
+        # Send event if Decision came from an experiment.
+        if decision.source == Optimizely::DecisionService::DECISION_SOURCE_EXPERIMENT
+          send_impression(decision.experiment, variation['key'], user_id, attributes)
         else
           @logger.log(Logger::DEBUG,
                       "The user '#{user_id}' is not being experimented on in feature '#{feature_flag_key}'.")
@@ -273,7 +273,7 @@ def is_feature_enabled(feature_flag_key, user_id, attributes = nil)
       end
 
       @logger.log(Logger::INFO,
-                  "Feature '#{feature_flag_key}' is not enabled for user '#{user_id}'.")
+        "Feature '#{feature_flag_key}' is not enabled for user '#{user_id}'.")
       false
     end
 

lib/optimizely/decision_service.rb

@@ -34,6 +34,10 @@ class DecisionService
     attr_reader :bucketer
     attr_reader :config
 
+    Decision = Struct.new(:experiment, :variation, :source)
+    DECISION_SOURCE_EXPERIMENT = 'experiment'.freeze
+    DECISION_SOURCE_ROLLOUT = 'rollout'.freeze
+
     def initialize(config, user_profile_service = nil)
       @config = config
       @user_profile_service = user_profile_service
@@ -105,31 +109,28 @@ def get_variation_for_feature(feature_flag, user_id, attributes = nil)
       # user_id - String ID for the user
       # attributes - Hash representing user attributes
       #
-      # Returns hash with the experiment and variation where visitor will be bucketed (nil if the user is not bucketed into any of the experiments on the feature)
+      # Returns Decision struct (nil if the user is not bucketed into any of the experiments on the feature)
 
       # check if the feature is being experiment on and whether the user is bucketed into the experiment
       decision = get_variation_for_feature_experiment(feature_flag, user_id, attributes)
       return decision unless decision.nil?
 
       feature_flag_key = feature_flag['key']
-      variation = get_variation_for_feature_rollout(feature_flag, user_id, attributes)
-      if variation
+      decision = get_variation_for_feature_rollout(feature_flag, user_id, attributes)
+      if decision
         @config.logger.log(
           Logger::INFO,
           "User '#{user_id}' is bucketed into a rollout for feature flag '#{feature_flag_key}'."
         )
-        # return decision with nil experiment so we don't track impressions for it
-        return {
-          'experiment' => nil,
-          'variation' => variation
-        }
-      else
-        @config.logger.log(
-          Logger::INFO,
-          "User '#{user_id}' is not bucketed into a rollout for feature flag '#{feature_flag_key}'."
-        )
+
+        return decision
       end
 
+      @config.logger.log(
+        Logger::INFO,
+        "User '#{user_id}' is not bucketed into a rollout for feature flag '#{feature_flag_key}'."
+      )
+
       nil
     end
 
@@ -140,7 +141,7 @@ def get_variation_for_feature_experiment(feature_flag, user_id, attributes = nil
       # user_id - String ID for the user
       # attributes - Hash representing user attributes
       #
-      # Returns a hash with the experiment and variation where visitor will be bucketed
+      # Returns Decision struct (nil if the user is not bucketed into any of the experiments on the feature)
       # or nil if the user is not bucketed into any of the experiments on the feature
       feature_flag_key = feature_flag['key']
       if feature_flag['experimentIds'].empty?
@@ -150,6 +151,7 @@ def get_variation_for_feature_experiment(feature_flag, user_id, attributes = nil
         )
         return nil
       end
+
       # Evaluate each experiment and return the first bucketed experiment variation
       feature_flag['experimentIds'].each do |experiment_id|
         experiment = @config.experiment_id_map[experiment_id]
@@ -160,18 +162,19 @@ def get_variation_for_feature_experiment(feature_flag, user_id, attributes = nil
           )
           return nil
         end
+        
         experiment_key = experiment['key']
         variation_id = get_variation(experiment_key, user_id, attributes)
+
         next unless variation_id
         variation = @config.variation_id_map[experiment_key][variation_id]
-        @config.logger.log(Logger::INFO,
-                           "The user '#{user_id}' is bucketed into experiment '#{experiment_key}' of feature "\
-                           "'#{feature_flag_key}'.")
-        return {
-          'experiment' => experiment,
-          'variation' => variation
-        }
+        @config.logger.log(
+          Logger::INFO,
+          "The user '#{user_id}' is bucketed into experiment '#{experiment_key}' of feature '#{feature_flag_key}'."
+        )
+        return Decision.new(experiment, variation, DECISION_SOURCE_EXPERIMENT)
       end
+      
       @config.logger.log(
         Logger::INFO,
         "The user '#{user_id}' is not bucketed into any of the experiments on the feature '#{feature_flag_key}'."
@@ -188,10 +191,9 @@ def get_variation_for_feature_rollout(feature_flag, user_id, attributes = nil)
       # user_id - String ID for the user
       # attributes - Hash representing user attributes
       #
-      # Returns the variation the user is bucketed into or nil if not bucketed into any of the targeting rules
+      # Returns the Decision struct or nil if not bucketed into any of the targeting rules
 
       bucketing_id = get_bucketing_id(user_id, attributes)
-
       rollout_id = feature_flag['rolloutId']
       if rollout_id.nil? || rollout_id.empty?
         feature_flag_key = feature_flag['key']
@@ -233,14 +235,15 @@ def get_variation_for_feature_rollout(feature_flag, user_id, attributes = nil)
           next
         end
 
+        # User failed traffic allocation, jump to Everyone Else rule
         @config.logger.log(
           Logger::DEBUG,
           "Attempting to bucket user '#{user_id}' into rollout rule for audience '#{audience_name}'."
         )
+
         # Evaluate if user satisfies the traffic allocation for this rollout rule
         variation = @bucketer.bucket(rollout_rule, bucketing_id, user_id)
-        return variation unless variation.nil?
-
+        return Decision.new(rollout_rule, variation, DECISION_SOURCE_ROLLOUT) unless variation.nil?
         # User failed traffic allocation, jump to Everyone Else rule
         @config.logger.log(
           Logger::DEBUG,
@@ -253,7 +256,7 @@ def get_variation_for_feature_rollout(feature_flag, user_id, attributes = nil)
       # get last rule which is the everyone else rule
       everyone_else_experiment = rollout_rules[number_of_rules]
       variation = @bucketer.bucket(everyone_else_experiment, bucketing_id, user_id)
-      return variation unless variation.nil?
+      return Decision.new(everyone_else_experiment, variation, DECISION_SOURCE_ROLLOUT) unless variation.nil?
 
       @config.logger.log(
         Logger::DEBUG,

spec/decision_service_spec.rb

@@ -349,7 +349,6 @@
         # any string that is not an experiment id in the data file
         feature_flag['experimentIds'] = ['1333333337']
         expect(decision_service.get_variation_for_feature_experiment(feature_flag, user_id, user_attributes)).to eq(nil)
-
         expect(spy_logger).to have_received(:log).once
           .with(Logger::DEBUG, "Feature flag experiment with ID '1333333337' is not in the datafile.")
       end
@@ -384,10 +383,11 @@
         it 'should return the variation' do
           user_attributes = {}
           feature_flag = config.feature_flag_key_map['multi_variate_feature']
-          expected_decision = {
-            'experiment' => config.experiment_key_map['test_experiment_multivariate'],
-            'variation' => config.variation_id_map['test_experiment_multivariate']['122231']
-          }
+          expected_decision = Optimizely::DecisionService::Decision.new(
+              config.experiment_key_map['test_experiment_multivariate'],
+              config.variation_id_map['test_experiment_multivariate']['122231'],
+              Optimizely::DecisionService::DECISION_SOURCE_EXPERIMENT
+          )
           expect(decision_service.get_variation_for_feature_experiment(feature_flag, 'user_1', user_attributes)).to eq(expected_decision)
 
           expect(spy_logger).to have_received(:log).once
@@ -402,13 +402,14 @@
       describe 'and the user is bucketed into one of the experiments' do
         before(:each) do
           mutex_exp = config.experiment_key_map['group1_exp1']
-          expected_variation = mutex_exp['variations'][0]
-          expected_decision = {
-            'experiment' => mutex_exp,
-            'variation' => expected_variation
-          }
+          variation = mutex_exp['variations'][0]
+          expected_decision = Optimizely::DecisionService::Decision.new(
+              mutex_exp,
+              variation,
+              Optimizely::DecisionService::DECISION_SOURCE_EXPERIMENT
+          )
           allow(decision_service).to receive(:get_variation)
-            .and_return(expected_variation['id'])
+            .and_return(variation['id'])
         end
 
         it 'should return the variation the user is bucketed into' do
@@ -483,15 +484,16 @@
         it 'should return the variation the user is bucketed into' do
           feature_flag = config.feature_flag_key_map['boolean_single_variable_feature']
           rollout_experiment = config.rollout_id_map[feature_flag['rolloutId']]['experiments'][0]
-          expected_variation = rollout_experiment['variations'][0]
+          variation = rollout_experiment['variations'][0]
           audience_id = rollout_experiment['audienceIds'][0]
           audience_name = config.get_audience_from_id(audience_id)['name']
+          expected_decision =  Optimizely::DecisionService::Decision.new(rollout_experiment, variation, Optimizely::DecisionService::DECISION_SOURCE_ROLLOUT)
 
           allow(Optimizely::Audience).to receive(:user_in_experiment?).and_return(true)
           allow(decision_service.bucketer).to receive(:bucket)
             .with(rollout_experiment, user_id, user_id)
-            .and_return(expected_variation)
-          expect(decision_service.get_variation_for_feature_rollout(feature_flag, user_id, user_attributes)).to eq(expected_variation)
+            .and_return(variation)
+          expect(decision_service.get_variation_for_feature_rollout(feature_flag, user_id, user_attributes)).to eq(expected_decision)
 
           expect(spy_logger).to have_received(:log).once
             .with(Logger::DEBUG, "Attempting to bucket user '#{user_id}' into rollout rule "\
@@ -542,17 +544,17 @@
             feature_flag = config.feature_flag_key_map['boolean_single_variable_feature']
             rollout = config.rollout_id_map[feature_flag['rolloutId']]
             everyone_else_experiment = rollout['experiments'][2]
-            expected_variation = everyone_else_experiment['variations'][0]
-
+            variation = everyone_else_experiment['variations'][0]
+            expected_decision =  Optimizely::DecisionService::Decision.new(everyone_else_experiment, variation, Optimizely::DecisionService::DECISION_SOURCE_ROLLOUT)
             allow(Optimizely::Audience).to receive(:user_in_experiment?).and_return(true)
             allow(decision_service.bucketer).to receive(:bucket)
               .with(rollout['experiments'][0], user_id, user_id)
               .and_return(nil)
             allow(decision_service.bucketer).to receive(:bucket)
               .with(everyone_else_experiment, user_id, user_id)
-              .and_return(expected_variation)
+              .and_return(variation)
 
-            expect(decision_service.get_variation_for_feature_rollout(feature_flag, user_id, user_attributes)).to eq(expected_variation)
+            expect(decision_service.get_variation_for_feature_rollout(feature_flag, user_id, user_attributes)).to eq(expected_decision)
 
             # make sure we only checked the audience for the first rule
             expect(Optimizely::Audience).to have_received(:user_in_experiment?).once
@@ -579,14 +581,15 @@
         feature_flag = config.feature_flag_key_map['boolean_single_variable_feature']
         rollout = config.rollout_id_map[feature_flag['rolloutId']]
         everyone_else_experiment = rollout['experiments'][2]
-        expected_variation = everyone_else_experiment['variations'][0]
+        variation = everyone_else_experiment['variations'][0]
+        expected_decision =  Optimizely::DecisionService::Decision.new(everyone_else_experiment, variation, Optimizely::DecisionService::DECISION_SOURCE_ROLLOUT)
 
         allow(Optimizely::Audience).to receive(:user_in_experiment?).and_return(false)
         allow(decision_service.bucketer).to receive(:bucket)
           .with(everyone_else_experiment, user_id, user_id)
-          .and_return(expected_variation)
+          .and_return(variation)
 
-        expect(decision_service.get_variation_for_feature_rollout(feature_flag, user_id, user_attributes)).to eq(expected_variation)
+        expect(decision_service.get_variation_for_feature_rollout(feature_flag, user_id, user_attributes)).to eq(expected_decision)
 
         # verify we tried to bucket in all targeting rules except for the everyone else rule
         expect(Optimizely::Audience).to have_received(:user_in_experiment?).once
@@ -636,13 +639,15 @@
         it 'should return the bucketed variation and nil experiment' do
           feature_flag = config.feature_flag_key_map['string_single_variable_feature']
           rollout = config.rollout_id_map[feature_flag['rolloutId']]
-          expected_variation = rollout['experiments'][0]['variations'][0]
-          expected_decision = {
-            'experiment' => nil,
-            'variation' => expected_variation
-          }
+          variation = rollout['experiments'][0]['variations'][0]
+          expected_decision = Optimizely::DecisionService::Decision.new(
+              nil,
+              variation,
+              Optimizely::DecisionService::DECISION_SOURCE_ROLLOUT
+          )
+
           allow(decision_service).to receive(:get_variation_for_feature_experiment).and_return(nil)
-          allow(decision_service).to receive(:get_variation_for_feature_rollout).and_return(expected_variation)
+          allow(decision_service).to receive(:get_variation_for_feature_rollout).and_return(expected_decision)
 
           expect(decision_service.get_variation_for_feature(feature_flag, user_id, user_attributes)).to eq(expected_decision)
           expect(spy_logger).to have_received(:log).once

spec/project_spec.rb

@@ -659,10 +659,12 @@ class InvalidErrorHandler; end
     it 'should return true but not send an impression if the user is not bucketed into a feature experiment' do
       experiment_to_return = config_body['rollouts'][0]['experiments'][0]
       variation_to_return = experiment_to_return['variations'][0]
-      decision_to_return = {
-        'experiment' => nil,
-        'variation' => variation_to_return
-      }
+
+      decision_to_return = Optimizely::DecisionService::Decision.new(
+          experiment_to_return,
+          variation_to_return,
+          Optimizely::DecisionService::DECISION_SOURCE_ROLLOUT
+      )
       allow(project_instance.decision_service).to receive(:get_variation_for_feature).and_return(decision_to_return)
 
       expect(project_instance.is_feature_enabled('boolean_single_variable_feature', 'test_user')).to be true
@@ -674,10 +676,12 @@ class InvalidErrorHandler; end
       allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
       experiment_to_return = config_body['experiments'][3]
       variation_to_return = experiment_to_return['variations'][0]
-      decision_to_return = {
-        'experiment' => experiment_to_return,
-        'variation' => variation_to_return
-      }
+      decision_to_return = Optimizely::DecisionService::Decision.new(
+          experiment_to_return,
+          variation_to_return,
+          Optimizely::DecisionService::DECISION_SOURCE_EXPERIMENT
+      )
+
       allow(project_instance.decision_service).to receive(:get_variation_for_feature).and_return(decision_to_return)
 
       expected_params = @expected_bucketed_params
@@ -746,7 +750,7 @@ class InvalidErrorHandler; end
 
             expect(project_instance.get_feature_variable_string('integer_single_variable_feature', 'integer_variable', user_id, user_attributes))
               .to eq(nil)
-            
+
             expect(spy_logger).to have_received(:log).once
               .with(
                 Logger::WARN,