Add experiment state

[withinboredom]

Changes proposed in this Pull Request

• Adds state management from Add A/B Test Framework #38755

It's not clear if CURRENT_USER_RECIEVE is required. I'm going to leave it there for now.

This code doesn't do anything currently except adding some reducers. The getAnonId part of the reducer is the riskiest piece of this and close monitoring will be required after deployment.

Testing instructions

• npm test to run the unit tests

[matticbot]

Test live: https://calypso.live/?branch=add/experiment-state

[yanirs]

Left some comments based on my limited understanding of the code. I'm not familiar enough with Calypso to provide very useful feedback on how everything connects, so it'd be good to get @blowery's review.

[yanirs]

Do we need to expose the anonId from the state? It seems like an internal detail to me, but I don't have a good mental image of how everything connects.

Also, Get's -> Gets 🙂

[withinboredom]

Do we need to expose the anonId from the state?

It appears the cookie gets deleted at some point... so we can't rely on it sticking around. We need it in case someone hits a landing page with an anon-experiment and gets assigned to that experiment. When they login, we need to know what they were assigned to as an anon-user so we need the value of the cookie to send to the assignment API to let it know that this user was that anon-user.

[yanirs]

Make sense. I was thinking more about whether we should export this function as opposed to using the anon ID internally and still storing it in the state.

Another thing I thought about is the order of execution: Is it possible for our code to run before the code that sets the cookie? Looks like it sometimes happens on the client side:

wp-calypso/client/lib/analytics/index.js

Lines 118 to 127 in bcddb6c

	if ( ! _ui ) {
	const cookies = cookie.parse( document.cookie );
	if ( cookies.tk_ai ) {
	_ui = cookies.tk_ai;
	} else {
	const randomIdLength = 18; // 18 * 4/3 = 24 (base64 encoded chars).
	_ui = createRandomId( randomIdLength );
	document.cookie = cookie.serialize( 'tk_ai', _ui );
	}
	}

However, from poking around it looks like there's also server-side code to set the same cookie. Thoughts?

[withinboredom]

Maybe we should make a new cookie instead of fighting over the tracks anon cookie?

(I don't think that's a bad idea btw.) However, we should probably recheck the anon cookie before making the API request.

[yanirs]

We need to use a consistent anonId to match our event tracking. It looks like initialising the analytics module would ensure that the cookie exists, but is there a better way to ensure it happens?

Taking a step back, is the only purpose of saving the anonId to pass it to an API call? If so, can't we assume that the server knows the anonId? If we can't make this assumption, how would the server track assignment for anons? Rely on the passed anonId?

[withinboredom]

I've verified that it doesn't capture the anon-id from a fresh boot of calypso and an anon user. I'll think on this and get back to you.

[withinboredom]

OK, after a few more tests, it does capture the anonid WHEN the user logs in, but not before.

Thinking about it a bit more, this is what I'm going to do:

1. Continue to capture the anonid on app init. This will capture it if they come from a landing page or something.
2. Capture the anonid if we haven't already, when we request from the API.

[withinboredom]

will provide any guarantee that the cookie will exist.

There's no guarantee the cookie will exist (it gets deleted when the user is aliased as far as I can tell). All we can guarantee is "best effort".

[bperson]

( thinking more about this )

It appears the cookie gets deleted at some point...

If it can be useful: this most likely happens in w.js when the user gets identified with identifyUser ( but I'm not sure exactly when that happens anymore in Calypso' flows )

edit 1: oh well should have refreshed before commenting :D

edit 2: fwiw, if we think about NOT deleting that cookie on identifyUser, I don't think it's a good idea

[jsnmoon]

Actually, I don't think the analytics module will provide any guarantee that the cookie will exist.

Why not? analytics.initialize() calls checkForBlockedTracks(), which calls the code I referenced, though it does happen in the catch() of _loadTracksResult so I'm not sure about the order of execution. @yanirs

checkForBlockedTracks will only initialize the anonymous ID if the loadScript( '//stats.wp.com/w.js?60' ) fails.

Even if the network request does fail, it'd be too late; the analytics module is initialized as a middleware after the Redux store has already been initialized.

FWIW, landing pages use a different boot logic.

[withinboredom]

This is the original bundle size.

This is the new bundle size reusing the existing anonId cookie function.

It increases the initial bundle size by 99kb but decreases all other bundles by a total of 500kb. This is an acceptable tradeoff for me since it makes our app overall less expensive to download, at a small expense up-front.

It adds about +3s upfront on a bad (1 bar) 3g connection (but no impact on anything faster) but saves 16s on other components. Since 100kb has no impact on other speeds, it will save time on anything faster than a bad 3g speed since the other bundles get smaller.

[jsnmoon]

Code looks reasonable 👍

We'll want to figure out a way to guarantee the existence of the _ui cookie value, though; left a comment inline regarding this issue.

[jsnmoon]

Outside the scope of this PR, but this function is a duplicate of this function here.

[jsnmoon]

It looks like initialising the analytics module would ensure that the cookie exists, but is there a better way to ensure it happens?

Actually, I don't think the analytics module will provide any guarantee that the cookie will exist. We typically depend on Tracks' w.js to serializes the _ui cookie value.

I think following @yanirs' advice here makes the most sense.

Pinging @bperson and @xyu for guidance.

[withinboredom]

rebased

[yanirs]

Probably worth adding an inline comment explaining why we do this.

[withinboredom]

That's probably a good idea.

[yanirs]

I think we can move forward with merging and push any remaining work on the anonId to an issue.

[dmsnell]

Concerning the app bundle size I'd not look as much at the reduction in section bundles but whether most page views will rely on this. It's not worth 99kb in the initial bundle if people only need the code once every 1,000 page views.

If there is concern about the additional bundle size then we might be missing an opportunity to have webpack split this out on its own. That is, we might be missing some hints to webpack that these chunks should all come together in one bundle instead of being distributed in pieces and duplicated in twenty other bundles.

[jsnajdr]

It increases the initial bundle size by 99kb but decreases all other bundles by a total of 500kb. This is an acceptable tradeoff for me since it makes our app overall less expensive to download, at a small expense up-front.

The reported ICFY stats are wrong for some reason. The latest icfy-stats CI task failed by running out of memory, and the stats reported for the previous push are different from what I see locally.

This patch only adds one small reducers to state/, and doesn't import any new modules that were not already there.

I recommend rebasing onto the latest master and letting the icfy-stats task run again. The updated stats should be correct.

Increasing the initial bundle by 100kB would be a bad tradeoff, adding more than 10% to the entrypoint. Many routes would need to load extra code even when they never call it.

The app would get less expensive to download only if the user's browser wanted to download all sections' chunks. But the user usually navigates only to one, two or three sections, never through the whole Calypso. The initial load would get slower.

[dzver]

LGTM

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~178 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
entry-main                  +628 B  (+0.0%)     +152 B  (+0.0%)
entry-login                  +55 B  (+0.0%)       +8 B  (+0.0%)
entry-gutenboarding          +55 B  (+0.0%)       +8 B  (+0.0%)
entry-domains-landing        +55 B  (+0.0%)      +10 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[withinboredom]

If those stats are correct, then this is the time to download:

section	size difference (kb)	time saved (seconds @ slow 3g)	time saved (seconds @ regular 3g)
entry-jetpack-cloud	-24.76	+0.79	+0.03
entry-main	+12.53	-0.40	-0.01
entry-login	+12.39	-0.40	-0.01
entry-gutenboarding	+12.37	-0.40	-0.01
entry-domains-landing	+12.37	-0.40	-0.01
posts-custom	-185.96	+5.95	+0.21
posts	-26.72	+0.86	+0.03
post-editor	-19.94	+0.64	+0.02
activity	-19.94	+0.64	+0.02
stats	-16.20	+0.52	+0.02
comments	-6.89	+0.22	+0.01
woocommerce	+6.73	-0.22	-0.01
purchases	+6.73	-0.22	-0.01
checkout	+6.73	-0.22	-0.01
marketing	-3.00	+0.10	+0.00

It would be nice if it did this for you :) let me know where I can make a PR to add these time calculations if you think they'd be useful. I think these are all pretty reasonable (dunno what's going on with posts-custom though...)