Splitting re-usable page-level experiment functionality out of google/ads/traffic-experiments.js #8374

tlong2 · 2017-03-24T17:32:09Z

For testing amp-auto-ads for Google ad users we'd like to have page-level experiments that divert client-side.

This CL splits the generic page-level experiment selection logic out of google/ads/traffic-experiments.js, so that it can be re-used for other things.

Other details:

introduces an isTrafficEligible function in ExperimentInfo, so experiments can be easily limited to certain slices of traffic (e.g. only those tagged with amp-auto-ads)
adds unit tests for googleAdsIsA4AEnabled in traffic-experiments.js
changes the list of experiment branches in ExperimentInfo to an array, so that the types aren't being violated when more than 2 branches are defined. This also protects against random other properties that may get inserted onto the ExperimentInfo object by the compiler.

tdrl

Generally looks good, and I'm willing to approve as-is. But one suggestion (about migrating all experiments info to the new format) and one open question (about whether the page-level-experiments should move to src/, instead of ads/google/)

tdrl · 2017-04-03T15:58:57Z

ads/google/a4a/traffic-experiments.js

-    const experimentInfo = {};
-    experimentInfo[experimentName] = internalBranches;
+    const experimentInfoMap = {};
+    experimentInfoMap[experimentName] = {


The format change is reasonable, but why not just migrate the existing experiment info descriptions to the new format, rather than copying them here at runtime?

I started doing that, but the logic on line 105 that decides whether or not A4A is enabled, needs to know whether the chosen experiment was the control or the experiment, so it's not completely trivial to change it to using anonymous branches. There's probably something we can do, but would rather tackle it in a separate PR request so this one doesn't get too far reaching.

Makes sense and SGTM.

tdrl · 2017-04-03T16:02:53Z

ads/google/page-level-experiments.js

@@ -0,0 +1,133 @@
+/**
+ * Copyright 2016 The AMP HTML Authors. All Rights Reserved.


Only question is whether this should continue to live in the ads/google hierarchy or just move to the src/ hierarchy? Perhaps @lannka has an opinion? (History: When I wrote the traffic-experiments.js code in the first place, we debated whether it was better to make it local to A4A or shared. At that time, it was decided to keep it local until there was a need to expand it. This is a clear need to expand, at least to Google scope. Do we foresee that it could be valuable to make this available to other clients as well, and so move to src/?)

I imagine it would be useful outside the google scope and nothing in page-level-experiments.js is specific to google (or even ads) now so happy to move if @lannka is happy with that?

i was about to ask the same thing. feel free to move to src/experiments.js
also, in the context of src/experiments.js , all the experiment is page-level, page prefix is no longer needed in your method names.

lannka · 2017-04-04T23:04:39Z

@tlong2 this looks good.
but have you considered introducing a more generic experiment control in src/experiments.js

getExperimentBranch('experiment-a') {
 ...
}

It will be configured at build-system/global-configs/prod-config.json:

{
  "experiment-a": {
    "branch-1": 0.5,
    "branch-2": 0.5,
  }
}

It will not have isTrafficEligible though

tlong2 · 2017-04-05T17:25:53Z

@lannka, the isTrafficEligible feature is important. The immediate use case for this is to test amp-auto-ads with traffic experiments. Since very few pubs will have the tag it will be necessary to limit the experiment to only pubs with the <amp-auto-ads> tags, otherwise the data will be too diluted to be useful.

lannka · 2017-04-05T17:55:12Z

ads/google/page-level-experiments.js

+ * @visibleForTesting
+ */
+export function randomlySelectUnsetPageExperiments(win, experiments) {
+  win.pageExperimentBranches = win.pageExperimentBranches || {};


@tdrl any reason why this was set on window object?

It's page-level, rather than element-level info, so I needed some page shared state to track. (Ensures that multiple slots on the same page don't reach different conclusions about their experiment status.) Is there a better place to store such shared state?

you can just put a local var, as we did here

lannka · 2017-04-05T17:59:09Z

ads/google/page-level-experiments.js

@@ -0,0 +1,133 @@
+/**
+ * Copyright 2016 The AMP HTML Authors. All Rights Reserved.


i was about to ask the same thing. feel free to move to src/experiments.js
also, in the context of src/experiments.js , all the experiment is page-level, page prefix is no longer needed in your method names.

lannka · 2017-04-05T18:00:24Z

ads/google/page-level-experiments.js

+      continue;
+    }
+
+    if (!experiments[experimentName].isTrafficEligible(win)) {


better check the availability of isTrafficEligible function before using it.

lannka

@tlong2 this is very cool. thanks for making the change. Actually my next PR will be benefitted from your refactoring.

As commented in code, please move the logic into src/experiments.js

tlong2 · 2017-04-06T16:19:53Z

@lannka Have moved the logic to src/experiments.js and removed the page prefix. Also now checks that the isTrafficEligible function is definitely defined before calling it.

lannka

Sorry for the delay. (Was in paternity leave).
The refactoring looks great.

I'll let @tdrl do a final check to make sure it's not going to break the A4A experiments.

lannka · 2017-04-17T16:35:37Z

@tlong2 pls also resolve the conflicts

tlong2 · 2017-04-18T15:21:20Z

@lannka Conflicts resolved

lannka · 2017-04-18T17:20:47Z

seems @tdrl is OOO, @keithwrightbos can you take a look to make sure the new changes after @tdrl 's approval does not break any A4A experiment settings?

keithwrightbos

LGTM though if there is no rush and we're concerned, we could wait for Terran to return from vacation (will be back Monday)

keithwrightbos · 2017-04-18T19:16:35Z

ads/google/a4a/google-data-reporter.js

 */
 export const PROFILING_BRANCHES = {
  a4aProfilingRate: {
-    control: 'unused',
-    experiment: 'unused',
+    isTrafficEligible: () => { return true; },


This can be isTrafficEligible: () => true,

tdrl

Some comments about restructuring tests, but they're not critical. (Nice to have, if there's time, though.) Overall, LGTM.

tdrl · 2017-04-19T18:03:36Z

ads/google/a4a/test/test-traffic-experiments.js

+      const renderViaA4a = googleAdsIsA4AEnabled(
+          sandbox.win, element, 'exp_name', externalBranches, internalBranches);
+      expect(renderViaA4a).to.be.true;
+      expect(element.getAttribute(EXPERIMENT_ATTRIBUTE)).to.equal('34,2088461');


Better to use traffic-experiments.js#IsExperimentOn to check for eid 34, rather than literal string match -- the other active experiments could change outside of your test control. And use expect(IsExperimentOn(12)).to.be.false to ensure that unwanted experiments are not turned on. Finally, import traffic-experiments.js#EXTERNALLY_SELECTED_ID and check for it via IsExperimentOn rather than checking 2088461 directly. Might be simplest to roll all this into a helper function that does all these expectations and can be called from every test case. (Ditto for other eid tests below.)

tdrl · 2017-04-19T18:11:16Z

ads/google/a4a/traffic-experiments.js

-  experimentInfo[experimentName] = internalBranches;
+  const experimentInfoMap = {};
+  experimentInfoMap[experimentName] = {
+    isTrafficEligible: () => { return true; },


I think, like @keithwrightbos mentioned elsewhere, that this can just be isTrafficEligible: () => true

tdrl · 2017-04-19T18:12:12Z

ads/google/a4a/traffic-experiments.js

-    const selectedBranch = getPageExperimentBranch(win, experimentName);
-    addExperimentIdToElement(selectedBranch, element);
+    const selectedBranch = getExperimentBranch(win, experimentName);
+    if (selectedBranch) {


Could selectedBranch ever fail to be truthy here?

With the exact current combinations of the internal branches, isTrafficEligible value, and behavior of all the various functions probably no. But since anyone of those things is liable to be changed by someone at some point, we should check that it is truthy here.

…roject#8374)

ampwafflebot added the in progress label Mar 24, 2017

tdrl approved these changes Apr 3, 2017

View reviewed changes

lannka reviewed Apr 5, 2017

View reviewed changes

lannka requested changes Apr 5, 2017

View reviewed changes

lannka approved these changes Apr 17, 2017

View reviewed changes

keithwrightbos approved these changes Apr 18, 2017

View reviewed changes

tdrl approved these changes Apr 19, 2017

View reviewed changes

splitting re-usable functionality out of traffic-experiments.js

91bc14e

lannka merged commit 6038844 into ampproject:master Apr 25, 2017

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

splitting re-usable functionality out of traffic-experiments.js (ampp…

79e53a2

…roject#8374)

ampprojectbot added the PR use: In Beta / Experimental label May 1, 2017

KenneyE pushed a commit to spotxchange/amphtml that referenced this pull request May 3, 2017

splitting re-usable functionality out of traffic-experiments.js (ampp…

8f6e37a

…roject#8374)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Splitting re-usable page-level experiment functionality out of google/ads/traffic-experiments.js #8374

Splitting re-usable page-level experiment functionality out of google/ads/traffic-experiments.js #8374

tlong2 commented Mar 24, 2017

tdrl left a comment

tdrl Apr 3, 2017

tlong2 Apr 3, 2017

tdrl Apr 3, 2017

tdrl Apr 3, 2017

tlong2 Apr 3, 2017

lannka Apr 5, 2017

lannka commented Apr 4, 2017

tlong2 commented Apr 5, 2017

lannka Apr 5, 2017

tdrl Apr 5, 2017

lannka Apr 5, 2017

lannka Apr 5, 2017

lannka Apr 5, 2017

lannka left a comment

tlong2 commented Apr 6, 2017

lannka left a comment

lannka commented Apr 17, 2017

tlong2 commented Apr 18, 2017

lannka commented Apr 18, 2017

keithwrightbos left a comment

keithwrightbos Apr 18, 2017

tdrl left a comment

tdrl Apr 19, 2017

tlong2 Apr 25, 2017

tdrl Apr 19, 2017

tlong2 Apr 25, 2017

tdrl Apr 19, 2017

tlong2 Apr 25, 2017

		@@ -0,0 +1,133 @@
		/**
		* Copyright 2016 The AMP HTML Authors. All Rights Reserved.

Splitting re-usable page-level experiment functionality out of google/ads/traffic-experiments.js #8374

Splitting re-usable page-level experiment functionality out of google/ads/traffic-experiments.js #8374

Conversation

tlong2 commented Mar 24, 2017

tdrl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka commented Apr 4, 2017

tlong2 commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka left a comment

Choose a reason for hiding this comment

tlong2 commented Apr 6, 2017

lannka left a comment

Choose a reason for hiding this comment

lannka commented Apr 17, 2017

tlong2 commented Apr 18, 2017

lannka commented Apr 18, 2017

keithwrightbos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdrl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment