amp-auto-ads: Wires up a holdout experiment for AdSense to study the effect of running amp-auto-ads #9261
Conversation
ads/google/a4a/utils.js
Outdated
| */ | ||
| export function mergeExperimentIds(newIds, currentIdString) { | ||
| const newIdArr = newIds.split(',').filter(newId => { | ||
| return newId && !isNaN(parseInt(newId, 10)); |
There was a problem hiding this comment.
The standard appears to be to wrap with Number constructor and I assume we're ok with excluding 0. In addition you can just use join so this can be:
return newIds.split(',').filter(newId => Number(newId)).join(',')
ads/google/adsense-amp-auto-ads.js
Outdated
|
|
||
| /** @const {!../../src/experiments.ExperimentInfo} */ | ||
| const ADSENSE_AMP_AUTO_ADS_EXPERIMENT_INFO = { | ||
| isTrafficEligible: win => { |
There was a problem hiding this comment.
isTrafficEligible: win => win.document.getElementsByTagName('AMP-AUTO-Ads').length,
ads/google/adsense-amp-auto-ads.js
Outdated
| * @param {!Window} win | ||
| * @return {?string} | ||
| */ | ||
| export function getAdSenseAmpAutoAdsExpBranch(win) { |
There was a problem hiding this comment.
Add doc indicating this getter has the side effect of selecting into the experiment
| @@ -149,7 +149,7 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A { | |||
| jsonParameters['targeting'] || null, | |||
| jsonParameters['categoryExclusions'] || null), | |||
| }, | |||
| ]); | |||
| ], []); | |||
There was a problem hiding this comment.
Consider making experimentIds optional so this is unnecessary
| @@ -27,6 +31,16 @@ import {viewportForDoc} from '../../../src/services'; | |||
| */ | |||
| class AdNetworkConfigDef { | |||
|
|
|||
| /*eslint-disable no-unused-vars*/ | |||
There was a problem hiding this comment.
You don't need this if the variable name starts with unused (e.g. unusedWin)
| @@ -172,6 +175,10 @@ export function googleAdUrl( | |||
| if (isCanary(win)) { | |||
| queryParams.push({name: 'isc', value: '1'}); | |||
| } | |||
| let eids = adElement.getAttribute('data-experiment-id'); | |||
There was a problem hiding this comment.
Rather than passing experimentIds, can't you just set them on data-experiment-id when you create the amp-ad element?
There was a problem hiding this comment.
The point is to also capture existing ads on the page. So we could append them to the data attribute in the amp-ad-network-adsense-impl.js constructor, but potentially diverting an experiment in a constructor seems kind of nasty.
Another option would be to append them to the data attribute in the getUrl function in amp-ad-network-adsense-impl.js, but seems a bit strange for that function to have side-effects like that.
ads/google/a4a/utils.js
Outdated
| */ | ||
| export function mergeExperimentIds(newIds, currentIdString) { | ||
| const newIdArr = newIds.split(',').filter(newId => { | ||
| return newId && !isNaN(parseInt(newId, 10)); |
ads/google/a4a/utils.js
Outdated
| @@ -132,10 +132,13 @@ export function isReportingEnabled(ampElement) { | |||
| * Parameters that will be put at the end of the URL, where they may be | |||
| * elided for length reasons. Intended for parameters with potentially | |||
| * long values, like URLs. | |||
| * @param {!Array<string>} experimentIds Any experiments IDs (in addition to | |||
There was a problem hiding this comment.
Given you expect these to be numbers, why not just make this Array?
There was a problem hiding this comment.
They're handled as strings everywhere else and it's also the type expected by the mergeExperimentIds function, so would rather just leave as Array
| @@ -132,10 +132,14 @@ export function isReportingEnabled(ampElement) { | |||
| * Parameters that will be put at the end of the URL, where they may be | |||
| * elided for length reasons. Intended for parameters with potentially | |||
| * long values, like URLs. | |||
| * @param {!Array<string>=} opt_experimentIds Any experiments IDs (in addition | |||
There was a problem hiding this comment.
Given you expect these to be numbers why not be explicit: !Array=
There was a problem hiding this comment.
They're handled as strings everywhere else and it's also the type expected by the mergeExperimentIds function, so would rather just leave as Array<string>
ads/google/a4a/utils.js
Outdated
| @@ -172,6 +176,12 @@ export function googleAdUrl( | |||
| if (isCanary(win)) { | |||
| queryParams.push({name: 'isc', value: '1'}); | |||
| } | |||
| let eids = adElement.getAttribute('data-experiment-id'); | |||
| if (opt_experimentIds) { | |||
| opt_experimentIds.forEach(experimentId => { | |||
There was a problem hiding this comment.
Why not just pass array of experimentIds into mergeExperimentIds as opposed to a loop the splits/joins each time?
ads/google/a4a/utils.js
Outdated
| */ | ||
| export function mergeExperimentIds(newIds, currentIdString) { | ||
| const newIdString = newIds.filter(newId => Number(newId)).join(','); | ||
| const separator = currentIdString && newIdString ? ',' : ''; |
There was a problem hiding this comment.
This seems more readable to me...
currentIdString = currentIdString || '';
return currentIdString + (currentIdString && newIdString ? ',' : '')+newIdString;
ads/google/adsense-amp-auto-ads.js
Outdated
| /** @const {!../../src/experiments.ExperimentInfo} */ | ||
| const ADSENSE_AMP_AUTO_ADS_EXPERIMENT_INFO = { | ||
| isTrafficEligible: | ||
| win => !!win.document.getElementsByTagName('AMP-AUTO-ADS').length, |
There was a problem hiding this comment.
Not sure it matters but win.document.querySelector('AMP-AUTO-ADS') may be more performant given it returns of first instance as opposed to getting all
Only ads that use the fast-fetch code path are labelled with experiment IDs. This is fine as fast-fetch will soon be the norm for AdSense amp-ads.
Only affects AdSense code paths.