-
Notifications
You must be signed in to change notification settings - Fork 166
LG-8190: A/B Testing of Acuant SDK Upgrade #7392
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
Changes from 12 commits
f149c93
6b5c1e9
8e3bd42
c3102b1
7ff9ee7
c313395
ae046aa
3afcaab
fea12c2
3a34d28
f032cf6
f9a4f79
b2192a1
61ea1f1
71f6713
82b3bef
df1b92c
6c1a5e1
24361ad
4774b99
9f7b4d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import { createContext } from 'react'; | ||
|
|
||
| interface AcuantSdkUpgradeABTestContextValue { | ||
| /** | ||
| * Whether or not the A/B testing of using the newer Acuant | ||
| * SDK version vs the older version is enabled | ||
| */ | ||
| acuantSdkUpgradeABTestingEnabled: boolean; | ||
| /** | ||
| * Whether or not we should use the newer sdk version as opposed | ||
| * to the older / current version | ||
| */ | ||
| useNewerSdk: boolean; | ||
| } | ||
| export const AcuantSdkUpgradeABTestContext = createContext<AcuantSdkUpgradeABTestContextValue>({ | ||
| acuantSdkUpgradeABTestingEnabled: false, | ||
| useNewerSdk: false, | ||
| }); | ||
|
|
||
| AcuantSdkUpgradeABTestContext.displayName = 'AcuantSdkUpgradeABTestContext'; | ||
|
|
||
| export default AcuantSdkUpgradeABTestContext; | ||
| export const AcuantSdkUpgradeABTestContextProvider = AcuantSdkUpgradeABTestContext.Provider; |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -521,6 +521,23 @@ def idv_native_camera_forced( | |||||||||
| ) | ||||||||||
| end | ||||||||||
|
|
||||||||||
| # @param [Boolean] use_newer_sdk Whether not to load the newer Acuant SDK version | ||||||||||
| # @param [String] version The version of the Acuant SDK that was loaded | ||||||||||
| # An A/B test for loading the old vs new version of the Acuant SDK. | ||||||||||
| # If this analytics call is not made at all, then the test isn't enabled. | ||||||||||
| def idv_acuant_sdk_upgrade_a_b_test( | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. based on the wording of the JIRA, I think that it might be good to add AB test attributes to the existing having a separate event like this to proactively log which bucket is good too in case users don't make it as far as submissions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah we need to know at SDK load time (ie before any submission or even attempts). Should we do both, keep the new event and amend the upload vendor event?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah seems like it would be useful to have the info at both events
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way that identity-idp/app/javascript/packs/document-capture.tsx Lines 69 to 72 in 9657a83
|
||||||||||
| use_newer_sdk:, | ||||||||||
| version:, | ||||||||||
| **extra | ||||||||||
| ) | ||||||||||
| track_event( | ||||||||||
| 'IdV: Acuant SDK Upgrade A/B Test', | ||||||||||
| use_newer_sdk: use_newer_sdk, | ||||||||||
| version: version, | ||||||||||
| **extra, | ||||||||||
| ) | ||||||||||
| end | ||||||||||
|
|
||||||||||
| # @param [String] step the step that the user was on when they clicked cancel | ||||||||||
| # @param [Idv::ProofingComponentsLogging] proofing_components User's current proofing components | ||||||||||
| # The user confirmed their choice to cancel going through IDV | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,6 +116,8 @@ idv_max_attempts: 5 | |
| idv_min_age_years: 13 | ||
| idv_native_camera_a_b_testing_enabled: false | ||
| idv_native_camera_a_b_testing_percent: 10 | ||
| idv_acuant_sdk_upgrade_a_b_testing_enabled: true | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want this enabled by default? I would expect a test to be disabled by default and explicitly enabled per environment.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll switch back and you can walk me through the typical process |
||
| idv_acuant_sdk_upgrade_a_b_testing_percent: 50 | ||
| idv_send_link_attempt_window_in_minutes: 10 | ||
| idv_send_link_max_attempts: 5 | ||
| ie11_support_end_date: '2022-12-31' | ||
|
|
||
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.
Are we planning to keep this code pattern around for a while?
If so, and if we plan to hard-code the version sources, I'd suggest getting rid of the prop altogether, since it's misleading to imply that it can be customized if the logic of the component will ignore the value depending on the A/B testing value.
Alternatively, we could adapt the component to receive the two sets of sources.
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.
My understanding is that we wanted to let this run for a week, maybe two. However I think we should have clarification. If we are going to AB test upgrades in the future (and why not?) then you are right, we should change some if these assumptions. Additionally, we'd probably want to set the old/new version numbers in the identitystore config
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.
@aduth After toying around with this a bit more, it seems that the
sdkSrcandcameraSrcprops were introduced specifically for the tests so that an error isn't thrown when these resources don't load during a frontend test. Here is an example.I've gone ahead and pushed some changes, whereby a source prop is passed in explicitly, that's what gets used no matter what (for now only tests use it this way). Otherwise, we kick it to the ab test context to determine which version/path to specify.
I've also added some constants to make this a little cleaner.
lmk what you think