LG-8190: A/B Testing of Acuant SDK Upgrade

[eric-gade]

🎫 Ticket

LG-8190

🛠 Summary of changes

We want to A/B test the upgrade of the Acuant SDK from 11.7.0 to 11.7.1 (50/50 each) for a bit and see if we notice any key differences.

This PR implements an AbTests constant and bucket instance for the specific case. We also have implemented the necessary changes on the React side (mostly context object code) and associated tests.

📜 Testing Plan

Run through the following two scenarios:

Scenario 1

• Disable the a/b test in your local application config
• Load the HTTPS Idp locally and create a new account
• Go through the idv steps until you get to the document capture screen
• Click one of the inputs to load the Acuant SDK
• Check the logs for the message 'IdV: Acuant SDK loaded'.
• It should have acuant_version and user_newer_sdk properties be 11.7.0 and false, respectively

Scenario 2

• Enable the a/b test in your local application config and set the percentage to be 100
• Load the HTTPS Idp locally and create a new account
• Go through the idv steps until you get to the document capture screen
• Click one of the inputs to load the Acuant SDK
• Check the logs for the message 'IdV: Acuant SDK loaded'
• - [ ] It should have acuant_version and user_newer_sdk properties be 11.7.1 and true, respectively

[zachmargolis]

based on the wording of the JIRA, I think that it might be good to add AB test attributes to the existing idv_doc_auth_submitted_image_upload_vendor or something, so that we could easily correlate A/B test bucket with results

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

[eric-gade]

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?

[zachmargolis]

yeah seems like it would be useful to have the info at both events

[aduth]

The way that flow_path is tracked across events during document capture could serve as good prior art to follow:

identity-idp/app/javascript/packs/document-capture.tsx

Lines 69 to 72 in 9657a83

	const trackEvent: typeof baseTrackEvent = (event, payload) => {
	const { flowPath } = appRoot.dataset;
	return baseTrackEvent(event, { ...payload, flow_path: flowPath });
	};

[aduth]

Since this is a new file and all new code should be TypeScript, can we make this file .tsx?

[aduth]

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.

[eric-gade]

Are we planning to keep this code pattern around for a while?

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

[eric-gade]

@aduth After toying around with this a bit more, it seems that the sdkSrc and cameraSrc props 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

[aduth]

Should we consider populating the context with the Acuant version, and using that as part of the analytics event, as well as in place of the constants in acuant-sdk-upgrade-a-b-test.tsx? Or, maybe we don't even need an extra context provider, and can just use AcuantContext in the way it was designed to receive the sdkSrc?

(Short on time to elaborate at the moment, but will follow-up with more detail on what I had in mind)

[aduth]

What I was thinking is maybe the frontend would provide an SDK version if and only if A/B testing is enabled, and then that version (if given) is used to derive a source string to be given directly to the existing AcuantContext, so that we don't need a new context, and so that AcuantContext wouldn't require many (or any) additional changes.

In packs/document-capture.tsx

interface AppRootData {
  // ...
  acuantVersion?: string;
}

const {
  // ...
  acuantVersion,
} = appRoot.dataset;

const isAcuantSDKUpgradeABTestingEnabled = typeof acuantVersion === 'string';

const trackEvent: typeof baseTrackEvent = (event, payload) => {
  // ...
  return baseTrackEvent(event, {
    ...payload,
    // ...
    acuant_sdk_upgrade_a_b_testing_enabled: isAcuantSDKUpgradeABTestingEnabled,
    acuant_version: acuantVersion,
  });
};

const App = composeComponents(
  // ...
  [
    AcuantContextProvider,
    {
      // ...
      sdkSrc: acuantVersion && `/acuant/${acuantVersion}/AcuantJavascriptWebSdk.min.js`,

[eric-gade]

Makes sense. The one amendment I would suggest is to also have the actual acuant_sdk_upgrade_a_b_testing_enabled value also be on the AppRoot, because this way we can specify the acuant version to use regardless of the AB test (ie, let's always specify a version to use). Otherwise we would have hard coded values for the version still present in the acuant context

[eric-gade]

I just pushed 6c1a5e1 which should implement most of your suggestions, along with some minor differences.

To me the outstanding issues are -- as I mentioned in the commit message -- whether/where to store the version numbers as globally accessible values, and whether or not we track an event for when the acuant sdk successfully loads or not.

[aduth]

whether/where to store the version numbers as globally accessible values

Do we need to access them anywhere else? If not, I don't think we'd need to do anything.

If we do, we can probably expose / access AcuantConfig.acuantVersion which would give us the version detail after the script is loaded.

whether or not we track an event for when the acuant sdk successfully loads or not.

What questions are we trying to answer with such an event? Are we concerned that the scripts might not load correctly?

Could we indirectly infer that from this event?

identity-idp/app/javascript/packages/document-capture/context/acuant.tsx

Lines 253 to 256 in 8c40e37

	trackEvent('IdV: Acuant SDK loaded', {
	success: true,
	isCameraSupported: nextIsCameraSupported,
	});

We could add the AcuantConfig.acuantVersion in there if we wanted?

[eric-gade]

Could we indirectly infer that from this event?

Yeah for sure, since we wrapped the default trackEvent in document capture and add all that extra information (in the pack). I am not so savvy on Cloudwatch queries so I don't know how much of a pain that is, however. If you think it's GTG let's just roll with it.

Do we need to access them anywhere else?

I am only thinking of the version numbers present in the default paths of the AcuantContext provider here. All other version information is defined on the Rails side now.

But again, if you think this is GTG for the moment let's push on

[aduth]

Oh, right, I forgot we added it in the base trackEvent, so it should be easy to distinguish in CloudWatch by e.g. filter properties.event_properties.acuant_version = '11.7.0', etc.

[solipet]

LGTM. Only comment I had was in regards to the default config.

[solipet]

Do we really want this enabled by default? I would expect a test to be disabled by default and explicitly enabled per environment.

[eric-gade]

I'll switch back and you can walk me through the typical process