Multi metric support for ab tests (using redis) #318
Conversation
This makes it so the "total conversion count" (as opposed to unique converted participant count) will only be tallied for participants (or implicit conversions), thus making it behave similarly to how unique participant conversions are tallied. Coincidentally, also fixes active record conversion counting (previously it treated conversions as implicit even though the argument defaults to false).
sdhull
force-pushed
the
multi-metrics
branch
from
70b0138 to
8a0e4e4
Compare
This doesn't actually provide real multi-metric support for mongo & ar adapters, but does allow them to continue to be used as they always were previously. Also update abstract_adapter to better reflect how things are passed around.
There was a problem hiding this comment.
Thanks for the hard work on this, sorry for the slow response. In order to get this merged, a few things would make it easier for other consumers of vanity to adopt:
- backwards compatibility
- for new functionality, just raising an error in the adapters that don't have support yet (ie don't override the error returned from AbstractAdapter)
- update the failing tests to account for any changes
- covering the new functionality with reasonable tests
I'll try to put some time towards this over the holidays, but leaving the PR open is great so that if other folks come along and can help they can pick up wherever it's left.
| @@ -16,3 +16,4 @@ html | |||
| .rvmrc | |||
| .ruby-version | |||
| .ruby-gemset | |||
| tags | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Dang sad I only just saw this suggestion -- it's brilliant
| @@ -48,72 +48,81 @@ def destroy_metric(metric) | |||
|
|
|||
| # -- Experiments -- | |||
|
|
|||
| def experiment_persisted?(experiment) | |||
| def experiment_persisted?(experiment_id) | |||
There was a problem hiding this comment.
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?
| @@ -1,3 +1,5 @@ | |||
| .vanity { width: 90%; margin: 2em auto; font-family: "Helvetica Neue", "Helvetica", "Verdana", sans-serif } | |||
There was a problem hiding this comment.
This change to almost-full width and other style changes would also be great to pull into a separate PR.
| # 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) |
There was a problem hiding this comment.
Can you add this new method to doc/adapters.textile? We wouldn't need to include which adapters support it there.
| @@ -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={}) | |||
There was a problem hiding this comment.
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.
|
Closing this for inactivity. Please comment/reopen if you'd like to pick it back up. |
This is really a WIP PR because I only implemented the support for redis adapter. I have neither the time nor the inclination to get this working for mongo & AR, though I was careful not to break those adapters.
You can feel free to drag this across the finish line, as I am allowing edits from vanity maintainers. This also may be useful for others who are struggling with similar problems (eg, tracking funnel conversion with experiments).
Improvements:
implicitparam (eg, if you are tracking "dollars added to cart", this is being tracked in current vanity release regardless of whether or not that conversion was done by a participant in your experiment)