-
Notifications
You must be signed in to change notification settings - Fork 112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A/B testing infrastructure updates #11026
Conversation
3806180
to
ae41e3e
Compare
a2e80d2
to
20a40c9
Compare
01f8d1a
to
0b38197
Compare
end | ||
|
||
def percent(discriminator) | ||
Digest::SHA256.hexdigest("#{discriminator}:#{experiment_name}").to_i(16).to_f / MAX_SHA * 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so as long as we're here, an idea for the future. Doesn't have to be this PR but something on my mind
I think it would be nice for future, new AB tests to not use SHA256 because it's slow, we could find some other faster stable digest and use that. (maybe something like this)
To maintain legacy compatibility and not redo existing AB tests, we would need to add a use_legacy_digest:
that defaults to false
and opt in our existing tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I like this idea. Captured as LG-14187
339956c
to
315ba75
Compare
AbTests have multiple `buckets`, so this commit renames the class to be a little clearer.
- Move tests into #bucket method block - Set up a `let` for bucket configs
Provide a proc that can be used to determine a discriminator from user/user_session/service_provider/request. changelog: Internal, A/B testing, Rework A/B testing system
Augment analytics events with a top-level `ab_tests` property that lists each active test and which bucket the event is in. (This will likely break a lot of tests)
- Add new method, ab_test_bucket, for controllers to figure out what bucket the user is in
should_log can be a Proc, RegExp, etc. and is matched against the event name.
- Handle case where UUID is present in session (hybrid flow) - Handle case where UUID is in Idv::Session
Right now all we're doing with this is checking to see if it's an idv-related event, which we can do with a Regexp.
- Tell the form what bucket it's in so that it can log properly - Add test coverage for form submission when Acuant A/B test is enabled
Earlier I was playing with having Idv::Session own discriminator calculation, but I didn't like it. I previously removed a method I accidentally committed--this removes a test for that removed method.
Run intialize tests under different conditions and actually verify they can return buckets
3a4090a
to
f5f1d18
Compare
I updated the PR description to include some manual testing steps. Out of that I realized that there were no tests for the AbTest initializers, so I added those in f5f1d18. |
def acuant_sdk_ab_test_analytics_args | ||
return {} if document_capture_session_uuid.blank? | ||
|
||
{ | ||
acuant_sdk_upgrade_ab_test_bucket: | ||
AbTests::ACUANT_SDK.bucket(document_capture_session_uuid), | ||
} | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋🏿 @matthinz ... assuming you're getting rid of this b/c it's noisy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, bucket will now be present on ab_tests.acuant_sdk.bucket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ... I ran through the tests and worked as expected
also tested with idv_acuant_sdk_upgrade_a_b_testing_enabled: true
🎫 Ticket
Relates to work for LG-12451
🛠 Summary of changes
This PR proposes a few changes to our A/B testing system. I think we are in a lull between A/B tests right now, so it seemed like a good opportunity.
The goals here are:
Here's how I'm proposing to do those things:
Rename
AbTestBucket
toAbTest
Not to get too bikesheddy, but I think what we're describing in the
config/initializers/ab_test.rb
file are tests, not buckets, so I think this name is more appropriate.Move discriminator calculation into the A/B test definition
When determining which bucket to put a user in for an A/B test, we first need to calculate a discriminator. Examples of common discriminators:
uuid
Ensures that the same user is always placed in the same bucketdocument_capture_session_uuid
ensures that, for a given identity verification attempt, the user is always placed in the same bucket. If they restart IdV, they may be placed in a new bucket.It is sometimes hard to figure out what is being used as a discriminator for a particular A/B test. This PR updates
AbTest
to accept a block that can be used to determine the discriminator:I believe this makes it clearer what discriminator is actually in use, and makes it so you don't have to go digging through controller files to find out.
If no block is provided, the user's UUID is used as a discriminator. If the user is not logged in, no bucket will be assigned by default.
Automatically add A/B test buckets to analytics events
This PR updates the
Analytics
class to augment analytics events with bucket assignments for the user for any running A/B tests. Here is what that looks like:Importantly:
ab_tests
key will be logged for the event.Warning
This is a breaking change. If you were previously counting on this data being in a certain place for a dashboard, now it will be in another place.
Limiting what events get annotated with A/B test bucket data
The
should_log
option can be set to aRegexp
to limit what events get augmented with A/B test data. For example, this event is limited to identity-verification related events (events that, by convention, start withIdV
,Idv
, oridv
):📜 Testing Plan
Test with no A/B tests enabled
logs/events.log
do not include anab_tests
propertytail -f log/events.log | jq '{name, ab_tests: .properties.ab_tests}'
Test with
DOC_AUTH_VENDOR
A/B test enabledapplication.yml
:log/events.log
include anab_tests
property (you may get an error thatsomething_else
is not a valid doc auth vendor since... it isn't)Test with
ACUANT_SDK
A/B test enabledapplication.yml
:log/events.log
include anab_tests
property.