Refactor order of bucketing operations. (#40)

Author: onufryk

lib/optimizely.rb

@@ -57,7 +57,7 @@ def initialize(datafile, event_dispatcher = nil, logger = nil, error_handler = n
       @event_dispatcher = event_dispatcher || EventDispatcher.new
 
       begin
-        validate_inputs(datafile, skip_json_validation)
+        validate_instantiation_options(datafile, skip_json_validation)
       rescue InvalidInputError => e
         @is_valid = false
         logger = SimpleLogger.new
@@ -82,7 +82,7 @@ def initialize(datafile, event_dispatcher = nil, logger = nil, error_handler = n
       end
 
       @bucketer = Bucketer.new(@config)
-      @event_builder = EVENT_BUILDERS_BY_VERSION[@config.version].new(@config, @bucketer)
+      @event_builder = EVENT_BUILDERS_BY_VERSION[@config.version].new(@config)
     end
 
     def activate(experiment_key, user_id, attributes = nil)
@@ -101,24 +101,15 @@ def activate(experiment_key, user_id, attributes = nil)
         return nil
       end
 
-      if attributes && !attributes_valid?(attributes)
-        @logger.log(Logger::INFO, "Not activating user '#{user_id}'.")
-        return nil
-      end
-
-      unless preconditions_valid?(experiment_key, user_id, attributes)
-        @logger.log(Logger::INFO, "Not activating user '#{user_id}'.")
-        return nil
-      end
-
-      variation_id = @bucketer.bucket(experiment_key, user_id)
+      variation_key = get_variation(experiment_key, user_id, attributes)
 
-      if not variation_id
+      if variation_key.nil?
         @logger.log(Logger::INFO, "Not activating user '#{user_id}'.")
         return nil
       end
 
       # Create and dispatch impression event
+      variation_id = @config.get_variation_id_from_key(experiment_key, variation_key)
       impression_event = @event_builder.create_impression_event(experiment_key, variation_id, user_id, attributes)
       @logger.log(Logger::INFO,
                   'Dispatching impression event to URL %s with params %s.' % [impression_event.url,
@@ -129,7 +120,7 @@ def activate(experiment_key, user_id, attributes = nil)
         @logger.log(Logger::ERROR, "Unable to dispatch impression event. Error: #{e}")
       end
 
-      @config.get_variation_key_from_id(experiment_key, variation_id)
+      variation_key
     end
 
     def get_variation(experiment_key, user_id, attributes = nil)
@@ -148,24 +139,34 @@ def get_variation(experiment_key, user_id, attributes = nil)
         return nil
       end
 
-      if attributes && !attributes_valid?(attributes)
+      unless preconditions_valid?(experiment_key, attributes)
         @logger.log(Logger::INFO, "Not activating user '#{user_id}.")
         return nil
       end
 
-      unless preconditions_valid?(experiment_key, user_id, attributes)
-        @logger.log(Logger::INFO, "Not activating user '#{user_id}.")
+      variation_id = @bucketer.get_forced_variation_id(experiment_key, user_id)
+
+      unless variation_id.nil?
+        return @config.get_variation_key_from_id(experiment_key, variation_id)
+      end
+
+      unless Audience.user_in_experiment?(@config, experiment_key, attributes)
+        @logger.log(Logger::INFO,
+                    "User '#{user_id}' does not meet the conditions to be in experiment '#{experiment_key}'.")
         return nil
       end
 
       variation_id = @bucketer.bucket(experiment_key, user_id)
-      @config.get_variation_key_from_id(experiment_key, variation_id)
+      unless variation_id.nil?
+        return @config.get_variation_key_from_id(experiment_key, variation_id)
+      end
+      nil
     end
 
     def track(event_key, user_id, attributes = nil, event_tags = nil)
       # Send conversion event to Optimizely.
       #
-      # event_key - Goal key representing the event which needs to be recorded.
+      # event_key - Event key representing the event which needs to be recorded.
       # user_id - String ID for user.
       # attributes - Hash representing visitor attributes and values which need to be recorded.
       # event_tags - Hash representing metadata associated with the event.
@@ -183,34 +184,26 @@ def track(event_key, user_id, attributes = nil, event_tags = nil)
         @logger.log(Logger::WARN, 'Event value is deprecated in track call. Use event tags to pass in revenue value instead.')
       end
 
-      return nil if attributes && !attributes_valid?(attributes)
-      return nil if event_tags && !event_tags_valid?(event_tags)
+      return nil unless user_inputs_valid?(attributes, event_tags)
 
-      experiment_ids = @config.get_experiment_ids_for_goal(event_key)
+      experiment_ids = @config.get_experiment_ids_for_event(event_key)
       if experiment_ids.empty?
         @config.logger.log(Logger::INFO, "Not tracking user '#{user_id}'.")
         return nil
       end
 
       # Filter out experiments that are not running or that do not include the user in audience conditions
-      valid_experiment_keys = []
-      experiment_ids.each do |experiment_id|
-        experiment_key = @config.experiment_id_map[experiment_id]['key']
-        unless preconditions_valid?(experiment_key, user_id, attributes)
-          @config.logger.log(Logger::INFO, "Not tracking user '#{user_id}' for experiment '#{experiment_key}'.")
-          next
-        end
-        valid_experiment_keys.push(experiment_key)
-      end
+
+      experiment_variation_map = get_valid_experiments_for_event(event_key, user_id, attributes)
 
       # Don't track events without valid experiments attached
-      if valid_experiment_keys.empty?
+      if experiment_variation_map.empty?
         @logger.log(Logger::INFO, "There are no valid experiments for event '#{event_key}' to track.")
         return nil
       end
 
       conversion_event = @event_builder.create_conversion_event(event_key, user_id, attributes,
-                                                                event_tags, valid_experiment_keys)
+                                                                event_tags, experiment_variation_map)
       @logger.log(Logger::INFO,
                   'Dispatching conversion event to URL %s with params %s.' % [conversion_event.url,
                                                                               conversion_event.params])
@@ -223,7 +216,35 @@ def track(event_key, user_id, attributes = nil, event_tags = nil)
 
     private
 
-    def preconditions_valid?(experiment_key, user_id, attributes)
+    def get_valid_experiments_for_event(event_key, user_id, attributes)
+      # Get the experiments that we should be tracking for the given event.
+      #
+      # event_key - Event key representing the event which needs to be recorded.
+      # user_id - String ID for user.
+      # attributes - Map of attributes of the user.
+      #
+      # Returns Map where each object contains the ID of the experiment to track and the ID of the variation the user
+      # is bucketed into.
+
+      valid_experiments = {}
+      experiment_ids = @config.get_experiment_ids_for_event(event_key)
+      experiment_ids.each do |experiment_id|
+        experiment_key = @config.get_experiment_key(experiment_id)
+        variation_key = get_variation(experiment_key, user_id, attributes)
+
+        if variation_key.nil?
+          @logger.log(Logger::INFO, "Not tracking user '#{user_id}' for experiment '#{experiment_key}'.")
+          next
+        end
+
+        variation_id = @config.get_variation_id_from_key(experiment_key, variation_key)
+        valid_experiments[experiment_id] = variation_id
+      end
+
+      valid_experiments
+    end
+
+    def preconditions_valid?(experiment_key, attributes = nil, event_tags = nil)
       # Validates preconditions for bucketing a user.
       #
       # experiment_key - String key for an experiment.
@@ -232,18 +253,29 @@ def preconditions_valid?(experiment_key, user_id, attributes)
       #
       # Returns boolean representing whether all preconditions are valid.
 
+      return false unless user_inputs_valid?(attributes, event_tags)
+
       unless @config.experiment_running?(experiment_key)
         @logger.log(Logger::INFO, "Experiment '#{experiment_key}' is not running.")
         return false
       end
 
-      if @config.user_in_forced_variation?(experiment_key, user_id)
-        return true
+      true
+    end
+
+    def user_inputs_valid?(attributes = nil, event_tags = nil)
+      # Helper method to validate user inputs.
+      #
+      # attributes - Dict representing user attributes.
+      # event_tags - Dict representing metadata associated with an event.
+      #
+      # Returns boolean True if inputs are valid. False otherwise.
+
+      if !attributes.nil? && !attributes_valid?(attributes)
+        return false
       end
 
-      unless Audience.user_in_experiment?(@config, experiment_key, attributes)
-        @logger.log(Logger::INFO,
-                    "User '#{user_id}' does not meet the conditions to be in experiment '#{experiment_key}'.")
+      if !event_tags.nil? && !event_tags_valid?(event_tags)
         return false
       end
 
@@ -268,7 +300,7 @@ def event_tags_valid?(event_tags)
       true
     end
 
-    def validate_inputs(datafile, skip_json_validation)
+    def validate_instantiation_options(datafile, skip_json_validation)
       unless skip_json_validation
         raise InvalidInputError.new('datafile') unless Helpers::Validator.datafile_valid?(datafile)
       end

lib/optimizely/bucketer.rb

@@ -1,5 +1,5 @@
 #
-#    Copyright 2016, Optimizely and contributors
+#    Copyright 2016-2017, Optimizely and contributors
 #
 #    Licensed under the Apache License, Version 2.0 (the "License");
 #    you may not use this file except in compliance with the License.
@@ -43,23 +43,6 @@ def bucket(experiment_key, user_id)
       #
       # Returns String variation ID in which visitor with ID user_id has been placed. Nil if no variation.
 
-      # handle forced variations if applicable
-      forced_variations = @config.get_forced_variations(experiment_key)
-      forced_variation_key = forced_variations[user_id]
-      if forced_variation_key
-        forced_variation_id = @config.get_variation_id_from_key(experiment_key, forced_variation_key)
-        if forced_variation_id
-          @config.logger.log(Logger::INFO, "User '#{user_id}' is forced in variation '#{forced_variation_key}'.")
-          return forced_variation_id
-        else
-          @config.logger.log(
-            Logger::INFO,
-            "Variation key '#{forced_variation_key}' is not in datafile. Not activating user '#{user_id}'."
-          )
-          return nil
-        end
-      end
-
       # check if experiment is in a group; if so, check if user is bucketed into specified experiment
       experiment_id = @config.get_experiment_id(experiment_key)
       group_id = @config.get_experiment_group_id(experiment_key)
@@ -118,6 +101,36 @@ def bucket(experiment_key, user_id)
       nil
     end
 
+    def get_forced_variation_id(experiment_key, user_id)
+      # Determine if a user is forced into a variation for the given experiment and return the id of that variation.
+      #
+      # experiment_key - Key representing the experiment for which user is to be bucketed.
+      # user_id - ID for the user.
+      #
+      # Returns variation ID in which the user with ID user_id is forced into. Nil if no variation.
+
+      forced_variations = @config.get_forced_variations(experiment_key)
+
+      return nil unless forced_variations
+
+      forced_variation_key = forced_variations[user_id]
+
+      return nil unless forced_variation_key
+
+      forced_variation_id = @config.get_variation_id_from_key(experiment_key, forced_variation_key)
+
+      unless forced_variation_id
+        @config.logger.log(
+          Logger::INFO,
+          "Variation key '#{forced_variation_key}' is not in datafile. Not activating user '#{user_id}'."
+        )
+        return nil
+      end
+
+      @config.logger.log(Logger::INFO, "User '#{user_id}' is forced in variation '#{forced_variation_key}'.")
+      forced_variation_id
+    end
+
     private
 
     def find_bucket(bucket_value, traffic_allocations)

lib/optimizely/event_builder.rb

@@ -42,12 +42,10 @@ def ==(event)
 
   class BaseEventBuilder
     attr_reader :config
-    attr_reader :bucketer
     attr_accessor :params
 
-    def initialize(config, bucketer)
+    def initialize(config)
       @config = config
-      @bucketer = bucketer
       @params = {}
     end
 
@@ -90,22 +88,22 @@ def create_impression_event(experiment_key, variation_id, user_id, attributes)
       Event.new(:post, IMPRESSION_EVENT_ENDPOINT, @params, POST_HEADERS)
     end
 
-    def create_conversion_event(event_key, user_id, attributes, event_tags, experiment_keys)
+    def create_conversion_event(event_key, user_id, attributes, event_tags, experiment_variation_map)
       # Create conversion Event to be sent to the logging endpoint.
       #
       # event_key - Event key representing the event which needs to be recorded.
       # user_id - ID for user.
       # attributes - Hash representing user attributes and values which need to be recorded.
       # event_tags - Hash representing metadata associated with the event.
-      # experiment_keys - Array of valid experiment keys for the event
+      # experiment_variation_map - Map of experiment ID to the ID of the variation that the user is bucketed into.
       #
       # Returns event hash encapsulating the conversion event.
 
       @params = {}
       add_common_params(user_id, attributes)
       add_conversion_event(event_key)
       add_event_tags(event_tags)
-      add_layer_states(user_id, experiment_keys)
+      add_layer_states(experiment_variation_map)
       Event.new(:post, CONVERSION_EVENT_ENDPOINT, @params, POST_HEADERS)
     end
 
@@ -206,14 +204,16 @@ def add_conversion_event(event_key)
       @params['eventName'] = event_name
     end
 
-    def add_layer_states(user_id, experiment_keys)
+    def add_layer_states(experiments_map)
+      # Add layer states information to the event.
+      #
+      # experiments_map - Hash with experiment ID as a key and variation ID as a value.
+
       @params['layerStates'] = []
 
-      experiment_keys.each do |experiment_key|
-        variation_id = @bucketer.bucket(experiment_key, user_id)
-        experiment_id = @config.experiment_key_map[experiment_key]['id']
+      experiments_map.each do |experiment_id, variation_id|
         layer_state = {
-          'layerId' => @config.experiment_key_map[experiment_key]['layerId'],
+          'layerId' => @config.experiment_id_map[experiment_id]['layerId'],
           'decision' => {
             'variationId' => variation_id,
             'experimentId' => experiment_id,

lib/optimizely/project_config.rb

@@ -131,16 +131,30 @@ def get_experiment_id(experiment_key)
       nil
     end
 
-    def get_experiment_ids_for_goal(goal_key)
-      # Get experiment IDs for the provided goal key.
+    def get_experiment_key(experiment_id)
+      # Retrieves experiment key for a given ID.
       #
-      # goal_key - Goal key for which experiment IDs are to be retrieved.
+      # experiment_id - String ID representing the experiment.
       #
-      # Returns array of all experiment IDs for the goal.
+      # Returns String key.
 
-      goal = @event_key_map[goal_key]
-      return goal['experimentIds'] if goal
-      @logger.log Logger::ERROR, "Event '#{goal_key}' is not in datafile."
+      experiment = @experiment_id_map[experiment_id]
+      return experiment['key'] unless experiment.nil?
+      @logger.log Logger::ERROR, "Experiment id '#{experiment_id}' is not in datafile."
+      @error_handler.handle_error InvalidExperimentError
+      nil
+    end
+
+    def get_experiment_ids_for_event(event_key)
+      # Get experiment IDs for the provided event key.
+      #
+      # event_key - Event key for which experiment IDs are to be retrieved.
+      #
+      # Returns array of all experiment IDs for the event.
+
+      event = @event_key_map[event_key]
+      return event['experimentIds'] if event
+      @logger.log Logger::ERROR, "Event '#{event_key}' is not in datafile."
       @error_handler.handle_error InvalidEventError
       []
     end