search

Betterment / test_track

Server app for the TestTrack multi-platform split-testing and feature-gating system
MIT License
119 stars 33 forks source link

Update assignment when mixpanel result has changed #126

Closed aburgel closed 5 years ago

aburgel commented 5 years ago

NOTE: This is built upon #125. I had to open a new PR since that PR was off of a fork. I made the following modification: Consolidated the logic in DeterministicAssignmentCreation to reduce coupling to ArbitraryAssignmentCreation and to close the window in which a new variant could be unintentially assigned.

Summary

https://github.com/Betterment/test_track/pull/123 introduced a change that results in the mixpanel_result never being updated when the client sends a request to DeterministicAssignmentCreation for an existing assignment. I believe the sequence of events would look something like this:

  1. The client encounters a split and sends the first request to Api::V1::AssignmentEventsController with params that have a nil mixpanel_result.
  2. The client's request to the analytics library succeeds and it sends another request to Api::V1::AssignmentEventsController with params that have a "success" mixpanel_result.
  3. The DeterministicAssignmentCreation doesn't call ArbitraryAssignmentCreation because it finds an existing_assignment.
  4. On a subsequent page load the client retrieves the split registry and sees a split with "unsynced": true.
  5. The client sends another request to Api::V1::AssignmentEventsController with params that have a "success" mixpanel_result.
  6. The DeterministicAssignmentCreation doesn't call ArbitraryAssignmentCreation because it finds an existing_assignment.

4 to 6 keep repeating for each page load.

This changes proposes a should_create_or_update_assignment? expression that incorporates a comparison of whether the existing and new mixpanel_results are equal.

It also adds an optional created_at attr to ArbitraryAssignmentCreation so we can choose to preserve the existing assignment timestamp if we don't want the default now.

/domain @jmileham /platform @jmileham /cc @joejansen

nanda-prbot commented 5 years ago

Needs somebody from @jmileham to claim domain review Needs somebody from @jmileham to claim platform review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

jmileham commented 5 years ago

<< domain platform

nanda-prbot commented 5 years ago

Needs @jmileham to provide platform review Needs @jmileham to provide domain review

When you finish a round of review, be sure to say you've finished or sign off on the PR, e.g.:

TAFN or DomainLGTM

If you're too busy to review, unclaim the PR, e.g.:

@myname >> domain

jmileham commented 5 years ago

should we add a ratchet so you can't change the mixpanel result after it's already in a terminal state? That's an optional feature add so domain lgtm platform lgtm!

nanda-prbot commented 5 years ago

Approved! :fireworks: :dart: :clap:

aburgel commented 5 years ago

should we add a ratchet so you can't change the mixpanel result after it's already in a terminal state

i like this idea. i'm now only allowing mixpanel_result to change if its not 'success'