diff --git a/lib/split/alternative.rb b/lib/split/alternative.rb index 8e21f80e..5d83ae77 100644 --- a/lib/split/alternative.rb +++ b/lib/split/alternative.rb @@ -180,13 +180,13 @@ def delete Split.redis.del(key) end + def key + "#{experiment_name}:#{name}" + end + private def hash_with_correct_values?(name) Hash === name && String === name.keys.first && Float(name.values.first) rescue false end - - def key - "#{experiment_name}:#{name}" - end end end diff --git a/lib/split/dashboard/experiment.rb b/lib/split/dashboard/experiment.rb index b17e69ae..fd748905 100644 --- a/lib/split/dashboard/experiment.rb +++ b/lib/split/dashboard/experiment.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "forwardable" +require "json" module Split class DashboardExperiment @@ -10,9 +11,7 @@ def initialize(experiment, goal = nil) end def alternatives - @alternatives ||= @experiment.alternatives.map do |alternative| - DashboardAlternative.new(alternative, @experiment, @goal) - end + @alternatives ||= build_alternatives end def extra_columns @@ -37,48 +36,82 @@ def total_unfinished def total_completed alternatives.sum(&:completed) end + + private + def build_alternatives + alternatives = @experiment.alternatives + stored = Split.redis.pipelined do |pipe| + alternatives.each { |alternative| pipe.hgetall(alternative.key) } + end + + views = alternatives.zip(stored).map do |alternative, data| + DashboardAlternative.new(alternative, @experiment, @goal, data) + end + views.each { |view| view.control = views.first } + end end class DashboardAlternative extend Forwardable def_delegators :@alternative, :name, :control? - attr_reader :participants, :unfinished, :completed, :extra_info + attr_reader :participants, :unfinished, :completed, :extra_info, :p_winner + attr_writer :control - def initialize(alternative, experiment, goal = nil) + def initialize(alternative, experiment, goal, data) @alternative = alternative - @experiment = experiment - @goal = goal - @participants = alternative.participant_count - @unfinished = alternative.unfinished_count - @completed = alternative.completed_count(goal) - @extra_info = alternative.extra_info + @participants = data["participant_count"].to_i + @completed = data[alternative.set_field(goal)].to_i + @unfinished = @participants - all_completed(data, experiment.goals) + @p_winner = data[alternative.set_prob_field(goal)].to_f + @extra_info = parse_extra_info(data["recorded_info"]) end def conversion_rate - @conversion_rate ||= @alternative.conversion_rate(@goal) + return 0 if participants.zero? + + completed.to_f / participants end def conversion_delta return if control? - control_rate = @experiment.control.conversion_rate(@goal) + control_rate = @control.conversion_rate return if control_rate <= 0 || conversion_rate == control_rate conversion_rate / control_rate - 1 end def z_score - @z_score ||= @alternative.z_score(@goal) - end + return "N/A" if control? - def p_winner - @p_winner ||= @alternative.p_winner(@goal) + rate = conversion_rate + control_rate = @control.conversion_rate + return "N/A" if rate > 1 || control_rate > 1 + + Split::Zscore.calculate(rate, participants, control_rate, @control.participants) end def winner? - @experiment.winner.name == name + @alternative.experiment.winner.name == name end + + private + def all_completed(data, goals) + goals.reduce(data[@alternative.set_field(nil)].to_i) do |sum, goal| + sum + data[@alternative.set_field(goal)].to_i + end + end + + def parse_extra_info(recorded_info) + return {} unless recorded_info && recorded_info.length > 1 + + begin + JSON.parse(recorded_info) + rescue + {} + end + end end end diff --git a/lib/split/experiment.rb b/lib/split/experiment.rb index dc67463f..d80a1124 100644 --- a/lib/split/experiment.rb +++ b/lib/split/experiment.rb @@ -288,31 +288,30 @@ def load_from_redis def can_calculate_winning_alternatives? self.alternatives.all? do |alternative| - alternative.participant_count >= 0 && - (alternative.participant_count >= alternative.completed_count) + participants = alternative.participant_count + participants >= 0 && participants >= alternative.completed_count end end def calc_winning_alternatives - return unless can_calculate_winning_alternatives? - # Cache the winning alternatives so we recalculate them once per the specified interval. intervals_since_epoch = Time.now.utc.to_i / Split.configuration.winning_alternative_recalculation_interval - if self.calc_time != intervals_since_epoch - if goals.empty? - self.estimate_winning_alternative - else - goals.each do |goal| - self.estimate_winning_alternative(goal) - end + return if self.calc_time == intervals_since_epoch + return unless can_calculate_winning_alternatives? + + if goals.empty? + self.estimate_winning_alternative + else + goals.each do |goal| + self.estimate_winning_alternative(goal) end + end - self.calc_time = intervals_since_epoch + self.calc_time = intervals_since_epoch - self.save - end + self.save end def estimate_winning_alternative(goal = nil) diff --git a/spec/dashboard/alternative_spec.rb b/spec/dashboard/alternative_spec.rb index a22fe933..5989893c 100644 --- a/spec/dashboard/alternative_spec.rb +++ b/spec/dashboard/alternative_spec.rb @@ -9,7 +9,7 @@ let(:alternative) { experiment.alternatives[1] } def view_for(alternative) - Split::DashboardAlternative.new(alternative, experiment) + Split::DashboardExperiment.new(experiment).alternatives.find { |view| view.name == alternative.name } end describe "#conversion_delta" do diff --git a/spec/dashboard/experiment_spec.rb b/spec/dashboard/experiment_spec.rb index e063a160..2eed1f62 100644 --- a/spec/dashboard/experiment_spec.rb +++ b/spec/dashboard/experiment_spec.rb @@ -74,4 +74,46 @@ expect(view.total_unfinished).to eq(27) end end + + describe "Redis roundtrips" do + before do + blue.participant_count = 10 + blue.set_completed_count(1) + blue.record_extra_info("revenue", 5) + red.participant_count = 20 + red.set_completed_count(2) + end + + it "loads every alternative in one roundtrip" do + expect { view.alternatives }.to make_redis_calls(1) + end + + it "leaves the alternatives themselves reading straight from Redis" do + view.alternatives + + expect { blue.participant_count }.to make_redis_calls(1) + end + + it "reads a whole table without going back to Redis" do + view.alternatives + + expect { + view.alternatives.each do |alternative| + alternative.participants + alternative.unfinished + alternative.completed + alternative.conversion_rate + alternative.conversion_delta + alternative.z_score + alternative.p_winner + alternative.extra_info + end + view.extra_columns + view.extra_totals + view.total_participants + view.total_unfinished + view.total_completed + }.to make_redis_calls(0) + end + end end diff --git a/spec/experiment_spec.rb b/spec/experiment_spec.rb index 4faf0994..ba28c498 100644 --- a/spec/experiment_spec.rb +++ b/spec/experiment_spec.rb @@ -592,5 +592,12 @@ def same_but_different_alternative expect(experiment.calc_winning_alternatives).not_to be nil expect(experiment.calc_winning_alternatives).to be nil end + + it "reads only the recalculation timestamp when the interval has not elapsed" do + experiment = Split::ExperimentCatalog.find_or_create({ "link_color4" => ["purchase"] }, "blue", "red", "green") + experiment.calc_winning_alternatives + + expect { experiment.calc_winning_alternatives }.to make_redis_calls(1) + end end end