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 #125

Closed joejansen closed 5 years ago

joejansen commented 5 years ago

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 @Betterment/test_track_core /platform @jmileham @aburgel

nanda-prbot commented 5 years ago

Needs somebody from @Betterment/test_track_core to claim domain review Needs somebody from @jmileham to claim platform review

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

@myname << domain && platform

aburgel commented 5 years ago

<< domain

nanda-prbot commented 5 years ago

Needs somebody from @jmileham and @aburgel to claim platform review

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

@myname << domain && platform

jmileham commented 5 years ago

<< platform - Burgel's gonna take this over while @joejansen's out

nanda-prbot commented 5 years ago

Needs @aburgel to provide domain review Needs @jmileham to provide platform 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

@aburgel >> domain << domain

cuz why not

jmileham commented 5 years ago

tafn

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

nanda

nanda-prbot commented 5 years ago

@joejansen needs to incorporate feedback from @jmileham. Bump when done.

aburgel commented 5 years ago

Reopened with some modification in #126 since this PR off a fork.