Integrate ExPlat and add A/A experiment

[renanferrari]

This PR integrates the ExPlat to the app and adds an A/A experiment implementation so that we can test the integration.

It builds upon the work previously done on the FluxC library:

• Add initial support for experimentation platform WordPress-FluxC-Android#1756
• ExPlat: Improve handling of null Variations and not found Experiments WordPress-FluxC-Android#1845

For more background context, please check these internal refs:

• pbmo2S-EF-p2
• pbxNRc-Fx-p2

Note: This is ready for review, but not ready for merge just yet in case we need to coordinate with the iOS team.

To test

This doesn't change anything on the UI, so first make sure all unit tests pass and then, with logcat opened, follow the steps below:

1. Open the app.
2. Notice a message like ExPlat: fetching assignments successful with result.
3. Put the app on the background and open it again.
4. Make sure you don't see any new message containing ExPlat.
5. Close/stop the app and open it again.
6. Notice the message from step 2 appears again.
7. Tap on the Reader button in the bottom navigation bar.
8. Notice a message like Tracked: reader_accessed.

Login/Logout

1. Log out from the app.
2. Notice a message like ExPlat: clearing cached assignments.
3. Login on the app again.
4. Notice a message like ExPlat: fetching assignments successful with result.

---

PR submission checklist:

• I have considered adding unit tests where possible.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[peril-wordpress-mobile]

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

[peril-wordpress-mobile]

You can test the changes on this Pull Request by downloading the APK here.

[planarvoid]

Thanks for the PR! I have a few comments about the experiment process, let me know what you think

[planarvoid]

When working with experiments I think it's very useful to have the variants defined explicitly. I'd consider hiding the getVariation method to be only called internally from within the experiments and creating an enum with the variants containing the control group.

[renanferrari]

For most experiments, where the only variations are Control and Treatment, I see no need to define them explicitly, as they can be easily handled like this:

when (myExperiment.getVariation()) {
    Control -> handleControl()    
    else -> handleTreatment()
}

For experiments with multiple variations, it would still be pretty simple:

when (myExperiment.getVariation()) {
    Treatment("treatment_one") -> handleTreatmentOne()
    Treatment("treatment_two") -> handleTreatmentTwo()
    else -> handleControl()
}

But we could also introduce an enum on top of the regular variation and achieve a similar result to what you suggested:

// Experiment definition
class MyExperiment(exPlat: ExPlat) : Experiment("my_experiment", exPlat) {
    fun getVariation(): EnumVariation {
        return when (super.getVariation(false)) {
            Treatment("treatment_one") -> TREATMENT_ONE
            Treatment("treatment_two") -> TREATMENT_TWO
            else -> CONTROL
        }
    }

    enum class EnumVariation {
        CONTROL, TREATMENT_ONE, TREATMENT_TWO
    }
}

// How we would handle it
when (myExperiment.getVariation()) {
    TREATMENT_ONE -> handleTreatmentOne()
    TREATMENT_TWO -> handleTreatmentTwo()
    else -> handleControl()
}

Keeping this flexible and having these options seems like a good deal for me. Let me know what you think.

[planarvoid]

I think all of these calls are ok if they happen within the Experiment. However, I think calls like

when (myExperiment.getVariation()) {
    Treatment("treatment_one") -> handleTreatmentOne()
    Treatment("treatment_two") -> handleTreatmentTwo()
    else -> handleControl()
}

it is simple but quite error prone. Especially since we'll have multiple places calling these. For experiments with just one treatment I think it would be much neater to just expose one method isTreatment instead of exposing getVariations. The same is for more treatments. Either having an enum or exposing methods like isTreatmentA, isTreatmentB and so on. Enums are not mandatory but I'd really consider making the getVariations method private so any future developers have to think about what they are exposing to make sure there is any unintentional mistake. Sometimes less flexibility is good for more safety :-).

[renanferrari]

I'd really consider making the getVariations method private so any future developers have to think about what they are exposing to make sure there is any unintentional mistake.

That's a solid point.

My intention was for us to have a standard, default way of handling variations that would work for most cases, where we wouldn't need to call getVariations from multiple places and wouldn't have more than two variations (this feature is not even available at the moment). But I guess making getVariations private and introducing a basic isControl method could probably achieve the same result I was looking for.

Another option I had in mind would be to introduce a ABExperiment class that extends Experiment and implements those methods by default. Then we could also introduce a MultivariateExperiment later on, that could enforce safety by using enums or something similar.

[planarvoid]

how do we make sure that the experiment is not fetched in the middle of a user process and doesn't change the behaviour of the current flow? For example you open an activity with a control, the app goes to the background, goes back, experiments are refreshed, you get assigned to a variant A, you click on a button and even though your View is "control", the action could be "variant A". We need a pull based cache for the experiments - once the value is read, it should never be changed as long as the app is alive.

[renanferrari]

If I'm not missing something, I believe this would depend on the experiment. Sometimes we will want to update the UI as soon as possible, some other times we won't.

In the example you gave, we would basically have two options. The first one would be to re-render the view when the app came back from background using the "variant A". If we don't want to do that because, let's say, the user was in the middle of a flow where updating the UI would be disrupting, then we should keeping using the control variant for the duration of that flow, but this wouldn't be handled by ExPlat.

We would also have to make sure that the exposure event for that experiment was triggered as soon as the user entered the flow and that it wasn't triggered again when the app came back from the background. As the variant changed, we can assume that the correct variant was only assigned to the user after he had already entered the flow. This means that all actions the user takes during that flow will be discarded by ExPlat and won't count towards the experiment analysis.

[planarvoid]

I think this is error prone. This requires us to keep the current experiment state somewhere out of the experiment. For example an experiment in the login flow would require us to store the value when the flow starts somewhere outside of the Experiment class.

We would also have to make sure that the exposure event for that experiment was triggered as soon as the user entered the flow and that it wasn't triggered again when the app came back from the background. As the variant changed, we can assume that the correct variant was only assigned to the user after he had already entered the flow. This means that all actions the user takes during that flow will be discarded by ExPlat and won't count towards the experiment analysis.

Is the exposure event triggered manually? Or does it happen when the value is fetched? If we implement a pull-based-cache, solving this would be easy. Whenever we check the experiment value and it's not set yet, we can send the exposure event.

[renanferrari]

I think this is error prone.

At this point I believe most experiments that escape the most common cases are probably error prone in one way or another. This is by design. That said, I think I understand the point you're making and I agree with it.

This requires us to keep the current experiment state somewhere out of the experiment.

By default, yes, that's true. But we can always implement an experiment that manages its own state later on, as needed.

This would probably require some changes to the way we're currently refreshing assignments, but that's not a problem. In fact, one the reasons this implementation is so lax at this point, is because we're supposed to change it as we learn what we actually need for each type of experiment. If we can anticipate some of those needs, great, if not, that's ok too.

Once we're in a place where we feel the implementation is solid enough for most of the cases we're trying to work with, then we would move this to its own library, so it can be used by other apps as well. Until then, all of this should be considered experimental.

Is the exposure event triggered manually? Or does it happen when the value is fetched?

It is triggered manually. Keep in mind that this event could be any of the existing Tracks events we already send. It is not necessarily an event created specifically for the experiment. In this experiment, we're using the reader_accessed event, for example.

If we implement a pull-based-cache, solving this would be easy. Whenever we check the experiment value and it's not set yet, we can send the exposure event.

Actually, the exposure event should be sent whenever the user is exposed to the experiment, regardless of the variation they are seeing. But again, I understand this is besides the point you're making.

[renanferrari]

Hey @planarvoid 👋 Thanks a lot for the review. I've answered most of your questions above, but some things have changed since then:

• We don't send custom event properties with the exposure event anymore.
• We force refresh the assignments when the app starts.
• We are clearing the cached assignments on logout and force refreshing them on login.

I've updated the tests steps above to reflect that and this is now ready for another look.

[renanferrari]

@planarvoid has raised some interesting questions about this PR and we will discuss them in more detail by tomorrow. I'm not expecting any major changes to immediately take place following that discussion, so I'm keeping the current Milestone for now. I'll keep this updated.

[renanferrari]

@planarvoid After our conversation today and thinking a bit more about what we want to achieve, I explored several different approaches to the issues you mentioned and here's what I've decided to do for now:

• Implemented a simple cache to prevent a different variation from being loaded during the same session, just as you had suggested.
• Kept the Experiment::getVariation method as it is, until I get a better understanding on how we are going to need to manipulate aggregate Experiment data.

The current implementation is definitely more than enough for our first A/A experiment and also flexible enough to evolve in any direction we may want in the near future.

Let me know what you think.

[planarvoid]

The code is looking good 👍 thanks for the changes! I was testing the flows and checking the messages in the logcat and two things are a bit strange.

When the app is restarted, the experiments are fetched twice (I see this message twice: ExPlat: fetching assignments successful with result: Assignments)

When I try to log in, I get the ExPlat: clearing cached assignments and active variations after putting in my username and password but before I use my authentication code. The ExPlat: fetching assignments successful with result: Assignments message comes after I use the authentication code but before I'm actually logged in in the app (it comes when there is still the progress bar visible).

Are these expected?

[renanferrari]

@planarvoid Thanks for the review and the excellent comments 🙂

When the app is restarted, the experiments are fetched twice (I see this message twice: ExPlat: fetching assignments successful with result: Assignments)

Good catch! This seems to be caused by a race condition: if the assignments are stale and the app startup finishes before the response of the first call arrives, then a second call is made.

The change in d69506f should prevent this.

When I try to log in, I get the ExPlat: clearing cached assignments and active variations after putting in my username and password but before I use my authentication code.

Another good catch! This happens because the onAuthenticationChanged() callback is also called when a 2FA code is retrieved.

Even though clearing the cached assignments multiple times while logged out is not really a big deal right now (since we're not running any logged out experiments), it's also not ideal.

The change in 7f44273 should address this.

The ExPlat: fetching assignments successful with result: Assignments message comes after I use the authentication code but before I'm actually logged in in the app (it comes when there is still the progress bar visible).

That's the expected behavior.

We're fetching the assignments as soon as we have an auth token available. The progress bar you're referring to takes a while to disappear because it waits for the result of two additional calls made at the end of the login flow (fetch account and fetch settings).

[peril-wordpress-mobile]

	Warnings
⚠️	This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

[planarvoid]

thanks for the fixes! It's looking good 👍. Feel free to merge this PR as soon as everyone is happy