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

Bugfix: Noop assignment_events for existing assignments #123

Closed jmileham closed 5 years ago

jmileham commented 5 years ago

Summary

119 introduced a change that unintentionally caused the assignment_event endpoint to touch updated_at on existing assignments for decided splits when clients, seeing no assignment (due to assignment masking in the relatively-new decision logic) attempted to create fresh assignments. As a result, the old assignment would win over the decision, effectively negating the decision.

This only impacts experiments, as feature gates are already immune to being explicitly assigned via the assignment_event endpoint

Other Information

I got to this implementation by testing from the outside in and redefining a test whose title matched what I wanted to be more strict. Ultimately it turns out that the DeterministicAssignmentCreation model was way fancier than it needed to be, unless I'm missing something.

DeterministicAssignmentCreation is only used by the AssignmentEvents controller, so the blast radius of this change, even if it is buggy, is contained. There's already a relatively nasty bug in play here, so my gut is to move aggressively here.

/domain @Betterment/test_track_core /platform @coreyja @samandmoore

nanda-prbot commented 5 years ago

Needs somebody from @Betterment/test_track_core to claim domain review Needs somebody from @coreyja and @samandmoore to claim platform review

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

@myname << domain && platform

jmileham commented 5 years ago

/cc @maidoesthings thanks for surfacing the weird behavior

jmileham commented 5 years ago

Another option here would've been to, additionally or instead, specifically noop assignment_events post decision, but that doesn't feel quite right. While it's true that the end-user-visible bug manifests only for decided splits with existing assignments to the non-decision variant, it actually feels OK for a first-time assignment to a decided split to be recorded. This would lead to UX continuity for a visitor who first experienced the split post-decision, should the decision be unmade (e.g. because it was called early or by accident).

At the same time, it doesn't feel appropriate to ever touch updated_at when receiving a duplicate assignment_event for a visitor, so that's what this PR proposes.

coreyja commented 5 years ago

<< platform LGTM

Also happy to grab domain if you want, but leaving it off since I am slightly less familiar with the TT modeling and stuff. BUT I am also relatively familiar with this bug now, so happy to grab domain too if you want!

nanda-prbot commented 5 years ago

Needs somebody from @Betterment/test_track_core to claim domain review

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

@myname << domain && platform

coreyja commented 5 years ago

<< domain LGTM

I just got indoctured as a new member of test_track_core! So seems like I might as well use my new powers to give this an LG

nanda-prbot commented 5 years ago

Approved! :ghost: :nail_care: :guitar: