Skip to content
This repository has been archived by the owner on Jan 1, 2024. It is now read-only.

Multi metric support for ab tests (using redis) #318

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ html
.rvmrc
.ruby-version
.ruby-gemset
tags
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, if you're generating ctags via an editor plugin, what do you think about putting this in a global gitignore in your home directory instead of per project?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang sad I only just saw this suggestion -- it's brilliant

2 changes: 1 addition & 1 deletion doc/adapters.textile
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,6 @@ These ab_* methods interact with the underlying data of an experiment.
* ab_add_participant(experiment, alternative, identity)
* ab_seen(experiment, identity, assignment)
* ab_assigned(experiment, identity)
* ab_add_conversion(experiment, alternative, identity, count, implicit)
* ab_add_conversion(experiment, alternative, identity, options)
* ab_get_outcome(experiment)
* ab_set_outcome(experiment, alternative)
43 changes: 26 additions & 17 deletions lib/vanity/adapters/abstract_adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,72 +48,81 @@ def destroy_metric(metric)

# -- Experiments --

def experiment_persisted?(experiment)
def experiment_persisted?(experiment_id)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this would be a breaking, non-backward compatible change, can we pull this change into a separate PR? When these changes are made, we also should be included backward compatible code, for example, see #317. What do you think about keeping these methods accepting experiment for this PR?

fail "Not implemented"
end

# Store when experiment was created (do not write over existing value).
def set_experiment_created_at(experiment, time)
def set_experiment_created_at(experiment_id, time)
fail "Not implemented"
end

# Return when experiment was created.
def get_experiment_created_at(experiment)
def get_experiment_created_at(experiment_id)
fail "Not implemented"
end

# Returns true if experiment completed.
def is_experiment_completed?(experiment)
def is_experiment_completed?(experiment_id)
fail "Not implemented"
end

# Store whether an experiment is enabled or not
def set_experiment_enabled(experiment, enabled)
def set_experiment_enabled(experiment_id, enabled)
fail "Not implemented"
end

# Returns true if experiment is enabled, the default (Vanity.configuration.experiments_start_enabled) is true.
# (*except for mock_adapter, where default is true for testing)
def is_experiment_enabled?(experiment)
def is_experiment_enabled?(experiment_id)
fail "Not implemented"
end

# Returns counts for given A/B experiment and alternative (by index).
# Returns counts for given A/B experiment (actual object) and alternative (by index).
# Returns hash with values for the keys :participants, :converted and
# :conversions.
def ab_counts(experiment, alternative)
fail "Not implemented"
end

# Returns counts by metric for a given A/B experiment (actual object) & alternative (by index).
# Returns a hash with keys metric id and values that are hashes like {added_to_cart: {converted: x, conversions: y}}
# Keep in mind that "converted" is unique conversions while "conversions" is a total tally (or
# you could set it up to track dollar sales or something like that)
# only really supported for redis adapter
def ab_counts_by_metric(experiment, alternative)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add this new method to doc/adapters.textile? We wouldn't need to include which adapters support it there.

fail "Not implemented"
end

# Pick particular alternative (by index) to show to this particular
# participant (by identity).
def ab_show(experiment, identity, alternative)
def ab_show(experiment_id, identity, alternative)
fail "Not implemented"
end

# Indicates which alternative to show to this participant. See #ab_show.
def ab_showing(experiment, identity)
def ab_showing(experiment_id, identity)
fail "Not implemented"
end

# Cancels previously set association between identity and alternative. See
# #ab_show.
def ab_not_showing(experiment, identity)
def ab_not_showing(experiment_id, identity)
fail "Not implemented"
end

# Records a participant in this experiment for the given alternative.
def ab_add_participant(experiment, alternative, identity)
def ab_add_participant(experiment_id, alternative, identity)
fail "Not implemented"
end

# Determines if a participant already has seen this alternative in this experiment.
def ab_seen(experiment, identity, assignment)
def ab_seen(experiment_id, identity, assignment)
fail "Not implemented"
end

# Determines what alternative a participant has already been given, if any
def ab_assigned(experiment, identity)
def ab_assigned(experiment_id, identity)
fail "Not implemented"
end

Expand All @@ -122,23 +131,23 @@ def ab_assigned(experiment, identity)
# true, add particpant if not already recorded for this experiment. If
# implicit is false (default), only add conversion is participant
# previously recorded as participating in this experiment.
def ab_add_conversion(experiment, alternative, identity, count = 1, implicit = false)
def ab_add_conversion(experiment_id, alternative, identity, options={})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to condense these into a options parameter, but we'll probably have to make this change in a backwards compatible manner as well, see #317 for an example of that.

fail "Not implemented"
end

# Returns the outcome of this expriment (if set), the index of a
# particular alternative.
def ab_get_outcome(experiment)
def ab_get_outcome(experiment_id)
fail "Not implemented"
end

# Sets the outcome of this experiment to a particular alternative.
def ab_set_outcome(experiment, alternative = 0)
def ab_set_outcome(experiment_id, alternative = 0)
fail "Not implemented"
end

# Deletes all information about this experiment.
def destroy_experiment(experiment)
def destroy_experiment(experiment_id)
fail "Not implemented"
end

Expand Down
33 changes: 25 additions & 8 deletions lib/vanity/adapters/active_record_adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -219,10 +219,9 @@ def is_experiment_enabled?(experiment)
# Returns hash with values for the keys :participants, :converted and
# :conversions.
def ab_counts(experiment, alternative)
record = VanityExperiment.retrieve(experiment)
participants = VanityParticipant.where(:experiment_id => experiment.to_s, :seen => alternative).count
converted = VanityParticipant.where(:experiment_id => experiment.to_s, :converted => alternative).count
conversions = record.vanity_conversions.where(:alternative => alternative).sum(:conversions)
participants = VanityParticipant.where(:experiment_id => experiment.id, :seen => alternative).count
converted = VanityParticipant.where(:experiment_id => experiment.id, :converted => alternative).count
conversions = experiment.vanity_conversions.where(:alternative => alternative).sum(:conversions)

{
:participants => participants,
Expand All @@ -231,6 +230,20 @@ def ab_counts(experiment, alternative)
}
end

def ab_counts_by_metric(experiment, alternative)
# not really supported for activerecord so fake it
metric_id = experiment.conversion_metric
converted = VanityParticipant.where(:experiment_id => experiment.id, :converted => alternative).count
conversions = experiment.vanity_conversions.where(:alternative => alternative).sum(:conversions)

{
metric_id.to_sym => {
:converted => converted,
:conversions => conversions
}
}
end

# Pick particular alternative (by index) to show to this particular
# participant (by identity).
def ab_show(experiment, identity, alternative)
Expand Down Expand Up @@ -273,10 +286,14 @@ def ab_assigned(experiment, identity)
# true, add participant if not already recorded for this experiment. If
# implicit is false (default), only add conversion if participant
# previously recorded as participating in this experiment.
def ab_add_conversion(experiment, alternative, identity, count = 1, implicit = false)
participant = VanityParticipant.retrieve(experiment, identity, false)
VanityParticipant.retrieve(experiment, identity, implicit, :converted => alternative, :seen => alternative)
VanityExperiment.retrieve(experiment).increment_conversion(alternative, count)
def ab_add_conversion(experiment, alternative, identity, options={})
count = options[:count] || 1
implicit = !!options[:implicit]
metric_id = options[:metric_id] # unsupported for ActiveRecord

if VanityParticipant.retrieve(experiment, identity, implicit, :converted => alternative, :seen => alternative)
VanityExperiment.retrieve(experiment).increment_conversion(alternative, count)
end
end

# Returns the outcome of this experiment (if set), the index of a
Expand Down
13 changes: 10 additions & 3 deletions lib/vanity/adapters/mock_adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -153,15 +153,22 @@ def ab_assigned(experiment, identity)
nil
end

def ab_add_conversion(experiment, alternative, identity, count = 1, implicit = false)
alt = alternative(experiment, alternative)
def ab_add_conversion(experiment, alternative, identity, options={})
count = options[:count] || 1
implicit = !!options[:implicit]
metric_id = options[:metric_id] # unsupported for mock adapter

@experiments[experiment] ||= {}
@experiments[experiment][:alternatives] ||= {}
alt = @experiments[experiment][:alternatives][alternative] ||= {}

alt[:participants] ||= Set.new
alt[:converted] ||= Set.new
alt[:conversions] ||= 0
if implicit
alt[:participants] << identity
else
participating = alt[:participants].include?(identity)
participating = alt[:participants].include?(identity)
end
alt[:converted] << identity if implicit || participating
alt[:conversions] += count
Expand Down
37 changes: 26 additions & 11 deletions lib/vanity/adapters/mongodb_adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -160,13 +160,24 @@ def is_experiment_enabled?(experiment)
end

def ab_counts(experiment, alternative)
record = @experiments.find(:_id=>experiment ).limit(1).projection(:conversions=>1).first
record = @experiments.find(:_id=>experiment.id ).limit(1).projection(:conversions=>1).first
conversions = record && record["conversions"]
{ :participants => @participants.find({ :experiment=>experiment, :seen=>alternative }).count,
:converted => @participants.find({ :experiment=>experiment, :converted=>alternative }).count,
{ :participants => @participants.find({ :experiment=>experiment.id, :seen=>alternative }).count,
:converted => @participants.find({ :experiment=>experiment.id, :converted=>alternative }).count,
:conversions => conversions && conversions[alternative.to_s] || 0 }
end

def ab_counts_by_metric(experiment, alternative)
# not really supported for mongo so fake it
metric_id = experiment.conversion_metric
{
metric_id.to_sym => {
:converted => @participants.find({ :experiment=>experiment.id, :converted=>alternative }).count,
:conversions => conversions && conversions[alternative.to_s] || 0
}
}
end

def ab_show(experiment, identity, alternative)
@participants.find(:experiment=>experiment, :identity=>identity).find_one_and_replace(
{
Expand Down Expand Up @@ -213,7 +224,11 @@ def ab_assigned(experiment, identity)
participant && participant["seen"].first
end

def ab_add_conversion(experiment, alternative, identity, count = 1, implicit = false)
def ab_add_conversion(experiment, alternative, identity, options={})
count = options[:count] || 1
implicit = !!options[:implicit]
metric_id = options[:metric_id] # unsupported for mongodb

if implicit
@participants.find(:experiment=>experiment, :identity=>identity).find_one_and_replace(
{
Expand All @@ -232,14 +247,14 @@ def ab_add_conversion(experiment, alternative, identity, count = 1, implicit = f
},
:upsert=>true
)
end

@experiments.find(:_id=>experiment).find_one_and_replace(
{
"$inc"=>{ "conversions.#{alternative}"=>count }
},
:upsert=>true
)
@experiments.find(:_id=>experiment).find_one_and_replace(
{
"$inc"=>{ "conversions.#{alternative}"=>count }
},
:upsert=>true
)
end
end

def ab_get_outcome(experiment)
Expand Down
Loading