Improve DualAdapter

[santib]

Motivation:

1. Given a user that hasn't been bucketed before in a running experiment.
2. When accessing a logged out page he/she is bucketed with version A.
3. When logging in

Expected Result:
Version A is still being shown.

Actual Result:
The current DualAdapter will just leave it up to the logged_in_adapter which has no information of that user, and any version could be chosen.

Related issue #483

Notes:

Currently, the DualAdapter make use of the logged_in_adapter and logged_out_adapter in a complete independent way. Because of that, the user won't have a consistent experience across logged in and logged out pages. This could also affect the conversions depending on how the experiment is set up. For example if the goal is in a logged in page, if someone was bucketed with two different versions when logged_in and logged_out, then the logged_out version will take no credit for the conversion but the logged_in version will take all of the credit.

With these changes the DualAdapter will use both adapters at the same time in a way such that we can make the UX consistent across logged in and logged out pages.

[andrehjr]

Nice @santib ! It does make a lot of sense to get the user a consistent experience between sessions using the DualAdapter when possible.

Although, seems like the CI failed due to something related to RSpec, I'll address that first, and then I'll review your PR.

Thanks for taking a look at such an old issue!

[santib]

Hi @andrehjr any news on this? I tested this further and I'm really happy with its behavior. In my project, I even added an extra line to decrement the participation for a version when the value gets overridden to account for the following scenario:

1. User X navigates on his smartphone and sees version A while logged out (cookies adapter)
2. User X logs in in his smartphone and continues to see version A thanks to this PR (redis adapter)
3. User X navigates on his notebook and sees version B while logged out (cookies adapter)
4. User X logs in in his notebook and sees version A thanks to this PR which prefers the logged in adapter over the logged out adapter. In this case we are "overriding" the cookie adapter version B with A because the user had already seen version A in another device. In this case we can also decrement participation count for B since it won't be able to reach the goal anymore.

What are your thoughts on this?

[andrehjr]

@santib I'll take a more in-depth look into this today, sorry for taking so long 🙌

[andrehjr]

I left a few comments, after that, it should be good to go :)

Good point on (4) Doing that would make the participation count more accurate when using the DualAdapter since we started using data from both adapters.

[andrehjr]

Return keys from both adapters can return duplicated keys. Other places internally iterate over keys so, this can lead to extra work.

I believe that it's better to keep the old behavior here, which is to return the keys from the current adapter (logged_in/logged_out).

[santib]

good catch, are you sure it's better logged_in/logged_out instead of (logged_in + logged_out).uniq ? I'm asking because I'm not sure if it can have some negative impacts

[andrehjr]

I don't think it's going to have negative impacts(at least internally) if we can gate this behavior under a configuration flag I mentioned on the other comment, we can use the uniq version.

[santib]

Thanks for your comments @andrehjr . I just added an extra commit for the decrement participation feature, I can remove the commit if you don't like it.

Also, have you thought how this change impacts the next gem release? Since we are changing a bit the behavior of an existing adapter, should we somehow notify the users about a breaking change? Is the CHANGELOGenough?

[andrehjr]

Thanks for your comments @andrehjr . I just added an extra commit for the decrement participation feature, I can remove the commit if you don't like it.

Also, have you thought how this change impacts the next gem release? Since we are changing a bit the behavior of an existing adapter, should we somehow notify the users about a breaking change? Is the CHANGELOGenough?

Yeap, I'm planning to cut release v3.3.3 release with these changes, will also need to add a config flag for DualAdapter (let's say 'fallback_to_logged_out_adapter = true') to make this an opt-in behavior, maybe adding a log on the initialization too, if the user has not configured such flag.

On the next major (v4), this is going to be the default behavior for DualAdapter. wdyt?

[santib]

Thanks for your comments @andrehjr . I just added an extra commit for the decrement participation feature, I can remove the commit if you don't like it.
Also, have you thought how this change impacts the next gem release? Since we are changing a bit the behavior of an existing adapter, should we somehow notify the users about a breaking change? Is the CHANGELOGenough?

Yeap, I'm planning to cut release v3.3.3 release with these changes, will also need to add a config flag for DualAdapter (let's say 'fallback_to_logged_out_adapter = true') to make this an opt-in behavior, maybe adding a log on the initialization too, if the user has not configured such flag.

On the next major (v4), this is going to be the default behavior for DualAdapter. wdyt?

@andrehjr Sounds like a plan 👌 The opt-in flag should be done within this same PR or in a separate PR only for the release v3.3.3 branch?
I just pushed some fixes to the specs and the comment about the keys method.

[andrehjr]

Let's add the optin flag on this PR if you're up to it. @santib ✨

[santib]

Let's add the optin flag on this PR if you're up to it. @santib ✨

@andrehjr Done 👌

[andrehjr]

Thanks @santib !