Merge pull request #742 from splitrb/refactor-experiment-load

Refactor Experiment Storage In order to reduce the amount of Redis Calls made during a single experiment

Author: andrehjr

lib/split/experiment.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "split/experiment_storage"
+
 module Split
   class Experiment
     attr_accessor :name
@@ -26,6 +28,9 @@ def initialize(name, options = {})
 
       @name = name.to_s
 
+      @config_storage = ExperimentStorage::ConfigStorage.new(name)
+      @redis_storage = ExperimentStorage::RedisStorage.new(name)
+
       extract_alternatives_from_options(options)
     end
 
@@ -50,23 +55,16 @@ def extract_alternatives_from_options(options)
 
       if alts.length == 1
         if alts[0].is_a? Hash
-          alts = alts[0].map { |k, v| { k => v } }
-        end
-      end
-
-      if alts.empty?
-        exp_config = Split.configuration.experiment_for(name)
-        if exp_config
-          alts = load_alternatives_from_configuration
-          options[:goals] = Split::GoalsCollection.new(@name).load_from_configuration
-          options[:metadata] = load_metadata_from_configuration
-          options[:resettable] = exp_config[:resettable]
-          options[:algorithm] = exp_config[:algorithm]
+          alts[0].map! { |k, v| { k => v } }
         end
       end
 
       options[:alternatives] = alts
 
+      if alts.empty? && @config_storage.exists?
+        options.merge!(@config_storage.load)
+      end
+
       set_alternatives_and_options(options)
 
       # calculate probability that each alternative is the winner
@@ -85,8 +83,12 @@ def save
         persist_experiment_configuration
       end
 
-      redis.hmset(experiment_config_key, :resettable, resettable.to_s,
-                                         :algorithm, algorithm.to_s)
+      stored_data = @redis_storage.load
+      if stored_data[:resettable] != resettable.to_s ||
+         stored_data[:algorithm] != algorithm.to_s
+        redis.hmset(experiment_config_key, :resettable, resettable.to_s,
+                                           :algorithm, algorithm.to_s)
+      end
       self
     end
 
@@ -99,7 +101,7 @@ def validate!
     end
 
     def new_record?
-      ExperimentCatalog.find(name).nil?
+      !@redis_storage.exists?
     end
 
     def ==(obj)
@@ -173,7 +175,7 @@ def start
     end
 
     def start_time
-      Split.cache(:experiment_start_times, @name) do
+      @start_time ||= Split.cache(:experiment_start_times, @name) do
         t = redis.hget(:experiment_start_times, @name)
         if t
           # Check if stored time is an integer
@@ -257,17 +259,7 @@ def delete_metadata
     end
 
     def load_from_redis
-      exp_config = redis.hgetall(experiment_config_key)
-
-      options = {
-        resettable: exp_config["resettable"],
-        algorithm: exp_config["algorithm"],
-        alternatives: load_alternatives_from_redis,
-        goals: Split::GoalsCollection.new(@name).load_from_redis,
-        metadata: load_metadata_from_redis
-      }
-
-      set_alternatives_and_options(options)
+      set_alternatives_and_options(@redis_storage.load)
     end
 
     def can_calculate_winning_alternatives?
@@ -430,37 +422,6 @@ def experiment_config_key
         "experiment_configurations/#{@name}"
       end
 
-      def load_metadata_from_configuration
-        Split.configuration.experiment_for(@name)[:metadata]
-      end
-
-      def load_metadata_from_redis
-        meta = redis.get(metadata_key)
-        JSON.parse(meta) unless meta.nil?
-      end
-
-      def load_alternatives_from_configuration
-        alts = Split.configuration.experiment_for(@name)[:alternatives]
-        raise ArgumentError, "Experiment configuration is missing :alternatives array" unless alts
-        if alts.is_a?(Hash)
-          alts.keys
-        else
-          alts.flatten
-        end
-      end
-
-      def load_alternatives_from_redis
-        alternatives = redis.lrange(@name, 0, -1)
-        alternatives.map do |alt|
-          alt = begin
-                  JSON.parse(alt)
-                rescue
-                  alt
-                end
-          Split::Alternative.new(alt, @name)
-        end
-      end
-
     private
       def redis
         Split.redis
@@ -490,11 +451,11 @@ def remove_experiment_configuration
       end
 
       def experiment_configuration_has_changed?
-        existing_experiment = Experiment.find(@name)
+        stored_data = @redis_storage.load
 
-        existing_experiment.alternatives.map(&:to_s) != @alternatives.map(&:to_s) ||
-          existing_experiment.goals != @goals ||
-          existing_experiment.metadata != @metadata
+        stored_data[:alternatives].map(&:to_s) != @alternatives.map(&:to_s) ||
+          stored_data[:goals] != @goals ||
+          stored_data[:metadata] != @metadata
       end
 
       def goals_collection

lib/split/experiment_storage.rb

@@ -0,0 +1,122 @@
+# frozen_string_literal: true
+
+module Split
+  class ExperimentStorage
+    class BaseStorage
+      attr_accessor :name
+
+      def initialize(name)
+        @name = name
+      end
+
+      def load
+        @data ||= load!
+      end
+
+      def load!
+        experiment_config = load_experiment
+        alternatives = load_alternatives
+        metadata = load_metadata
+        goals = load_goals
+
+        {
+          resettable: experiment_config[:resettable],
+          algorithm: experiment_config[:algorithm],
+          alternatives: alternatives,
+          goals: goals,
+          metadata: metadata
+        }
+      end
+
+      def exists?
+        raise "implement"
+      end
+
+      def load_alternatives
+        raise "implement"
+      end
+
+      def load_metadata
+        raise "implement"
+      end
+
+      def load_goals
+        raise "implement"
+      end
+
+      def load_experiment
+        raise "implement"
+      end
+    end
+
+    class ConfigStorage < BaseStorage
+      def exists?
+        !!Split.configuration.experiment_for(@name)
+      end
+
+      def load_alternatives
+        alts = Split.configuration.experiment_for(@name)[:alternatives]
+        raise ArgumentError, "Experiment configuration is missing :alternatives array" unless alts
+
+        alts = alts.keys if alts.is_a?(Hash)
+        alts.flatten
+      end
+
+      def load_metadata
+        Split.configuration.experiment_for(@name)[:metadata]
+      end
+
+      def load_goals
+        Split::GoalsCollection.new(@name).load_from_configuration
+      end
+
+      def load_experiment
+        Split.configuration.experiment_for(@name)
+      end
+    end
+
+    class RedisStorage < BaseStorage
+      def exists?
+        redis.exists?(@name)
+      end
+
+      def load_alternatives
+        alternatives = redis.lrange(@name, 0, -1)
+        alternatives.map do |alt|
+          alt = begin
+                  JSON.parse(alt)
+                rescue
+                  alt
+                end
+          Split::Alternative.new(alt, @name)
+        end
+      end
+
+      def load_metadata
+        meta = redis.get(metadata_key)
+        JSON.parse(meta) unless meta.nil?
+      end
+
+      def load_goals
+        Split::GoalsCollection.new(@name).load_from_redis
+      end
+
+      def load_experiment
+        redis.hgetall(experiment_config_key).transform_keys(&:to_sym)
+      end
+
+      def experiment_config_key
+        "experiment_configurations/#{@name}"
+      end
+
+      def metadata_key
+        "#{name}:metadata"
+      end
+
+      private
+        def redis
+          Split.redis
+        end
+    end
+  end
+end

lib/split/persistence/redis_adapter.rb

@@ -27,7 +27,7 @@ def [](field)
       end
 
       def []=(field, value)
-        Split.redis.hset(redis_key, field, value.to_s)
+        Split.redis.hset(redis_key, field, value.to_s) if self[field] != value.to_s
         expire_seconds = self.class.config[:expire_seconds]
         Split.redis.expire(redis_key, expire_seconds) if expire_seconds
       end

lib/split/user.rb

@@ -16,7 +16,7 @@ def initialize(context, adapter = nil)
     def cleanup_old_experiments!
       return if @cleaned_up
       keys_without_finished(user.keys).each do |key|
-        experiment = ExperimentCatalog.find key_without_version(key)
+        experiment = Experiment.new key_without_version(key)
         if experiment.nil? || experiment.has_winner? || experiment.start_time.nil?
           user.delete key
           user.delete Experiment.finished_key(key)

spec/experiment_storage_spec.rb

@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe Split::ExperimentStorage do
+  let(:experiment) do
+    {
+      resettable: "false",
+      algorithm: "Split::Algorithms::WeightedSample",
+      alternatives: [ "foo", "bar" ],
+      goals: ["purchase", "refund"],
+      metadata: {
+        "foo" => { "text" => "Something bad" },
+        "bar" => { "text" => "Something good" }
+      }
+    }
+  end
+  before do
+    Split.configuration.experiments = {
+      my_exp: experiment
+    }
+  end
+
+  context "ConfigStorage" do
+    let(:config_store) { Split::ExperimentStorage::ConfigStorage.new("my_exp") }
+
+    it "loads an experiment from the configuration" do
+      stored_data = config_store.load
+      expect(stored_data).to match(experiment)
+    end
+
+    it "checks if an experiment exists on the configuration" do
+      expect(config_store.exists?).to be_truthy
+      expect(Split::ExperimentStorage::ConfigStorage.new("whatever").exists?).to be_falsy
+    end
+
+    it "memoizes data from the configuration by default" do
+      expect(config_store).to receive(:load!).once.and_call_original
+      config_store.load
+      config_store.load
+    end
+  end
+
+  context "from Redis" do
+    before { Split::ExperimentCatalog.find_or_create(:my_exp) }
+    let(:config_store) { Split::ExperimentStorage::RedisStorage.new("my_exp") }
+
+    it "loads an experiment from the configuration" do
+      stored_data = config_store.load
+      stored_data[:alternatives].map! { |alternative| alternative.name }
+      expect(stored_data).to match(experiment)
+    end
+
+    it "checks if an experiment exists on the configuration" do
+      expect(config_store.exists?).to be_truthy
+      expect(Split::ExperimentStorage::ConfigStorage.new("whatever").exists?).to be_falsy
+    end
+  end
+end

spec/user_spec.rb

@@ -48,16 +48,17 @@
     end
 
     it "removes key if experiment has a winner" do
-      allow(Split::ExperimentCatalog).to receive(:find).with("link_color").and_return(experiment)
-      allow(experiment).to receive(:start_time).and_return(Date.today)
-      allow(experiment).to receive(:has_winner?).and_return(true)
+      experiment = Split::ExperimentCatalog.find_or_create("link_color", "red", "blue")
+      experiment.start
+      experiment.winner = "red"
+
+      expect(experiment.has_winner?).to be_truthy
       @subject.cleanup_old_experiments!
       expect(@subject.keys).to be_empty
     end
 
     it "removes key if experiment has not started yet" do
-      allow(Split::ExperimentCatalog).to receive(:find).with("link_color").and_return(experiment)
-      allow(experiment).to receive(:has_winner?).and_return(false)
+      expect(Split::ExperimentCatalog.find("link_color")).to be_nil
       @subject.cleanup_old_experiments!
       expect(@subject.keys).to be_empty
     end
@@ -66,11 +67,12 @@
       let(:user_keys) { { "link_color" => "blue", "link_color:finished" => true } }
 
       it "does not remove finished key for experiment without a winner" do
-        allow(Split::ExperimentCatalog).to receive(:find).with("link_color").and_return(experiment)
-        allow(Split::ExperimentCatalog).to receive(:find).with("link_color:finished").and_return(nil)
-        allow(experiment).to receive(:start_time).and_return(Date.today)
-        allow(experiment).to receive(:has_winner?).and_return(false)
+        experiment = Split::ExperimentCatalog.find_or_create("link_color", "red", "blue")
+        experiment.start
+
+        expect(experiment.has_winner?).to be_falsey
         @subject.cleanup_old_experiments!
+
         expect(@subject.keys).to include("link_color")
         expect(@subject.keys).to include("link_color:finished")
       end