-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add initial support for experimentation platform #1756
Conversation
c76a1dc
to
c4bed3f
Compare
c4bed3f
to
29822a0
Compare
29822a0
to
de86a4e
Compare
hey @renanferrari , I was wondering why you used event bus instead of the newer approach with coroutines (FluxC wiki). I think we shouldn't be using EventBus for new stores. |
@planarvoid This is the first time I've created a store and I actually wasn't aware we shouldn't be using EventBus for new ones. It makes total sense! I've now made the coroutine function public so the client can call that directly. |
private val gson: Gson by lazy { GsonBuilder().serializeNulls().create() } | ||
|
||
@Subscribe(threadMode = ThreadMode.ASYNC) | ||
override fun onAction(action: Action<*>) { |
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.
I think I'd just remove this function and the Store parent unless you really need the reason for the event bus usage (it should be pretty rare - mostly for things consumed already by multiple places in the app).
AppLog.d(API, "${this.javaClass.simpleName}: onRegister") | ||
} | ||
|
||
suspend fun fetchAssignments(fetchPayload: FetchAssignmentsPayload): OnAssignmentsFetched { |
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.
do you think you could wrap this in coroutineEngine.withDefaultContext
in order to add some logging to the method call?
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.
The store looks great! I have two minor comments 👍 Thanks for the changes from event bus to coroutines 👍
Hey @planarvoid, I just realized I never pinged you back to let you know this is ready for another round of review. |
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.
Thanks for the ping and the changes @renanferrari , it's looking good 👍
fun fromName(name: String?) = when (name) { | ||
"control" -> Control | ||
"treatment" -> Treatment | ||
null -> Unknown |
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.
I haven't finished reviewing this code, but the assignment will be null
if it's "control" and the control's name will never be sent back in an API response. This is so an old client is guaranteed to handle a disabled experiment properly or in the event an experiment needs to be disabled in an emergency, it will revert back to the control.
|
||
fun isStale(now: Date = Date()) = !now.before(expiresAt) | ||
|
||
fun getVariationForExperiment(experiment: String) = variations[experiment] ?: Unknown |
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.
Instead of returning Unknown
, consider returning Control
if the experiment isn't found. This way old clients will properly handle a disabled experiment which are not returned via the API response, or in the case a client is released before the experiment goes live.
Thanks for your comments, @withinboredom! I'll make sure to address them in a future PR soon, since this one has already been merged. |
This PR adds an initial support for the Experimentation Platform API (internal ref: pbmo2S-yK-p2).
Usage
The main class added by this PR is the
ExperimentStore
, to which a client implementation would interact in the following way:First, it would fetch the latest experiment assignments for an user by calling
experimentStore::fetchAssignments
:And then handle the resulting
OnAssignmentsFetched
:Once the assignments are fetched, they are automatically cached and can be retrieved like so:
With an
Assignments
object in hand, it's possible to retrieve aVariation
for a specific experiment and then handle it accordingly:It's also possible to check if we should fetch newer assignments:
Next steps
Theoretically, this implementation already gives us the ability to use the Experimentation Platform API in our apps, although we still need to wait for the apps to be officially supported by the platform (more information about this in the internal reference linked above).
From a practical perspective however, we should probably still add a middle abstraction between the clients and this implementation to make it a little bit easier to do stuff like generating and storing anonymous IDs and automatically fetching new values when the cached results are stale, etc.
The way I see it, the most obvious candidates to receive such implementation would be either WordPress-Analytics-Android, a brand new library, or even the apps themselves if we don't care much about sharing that logic.
This is totally open for discussion.
To test
You can either run the included unit tests or use the following steps on the example app:
calypso
andwpcom
).Feel free to try out different combinations, both signed in and signed out.