鉁⊿tarted a config parser for the amp-experiment 1.0 format #21523
Conversation
| * @package | ||
| * @restricted | ||
| */ | ||
| init(variants) { | ||
| this.variantsDeferred_.resolve(variants); | ||
| variants.then(this.variantsDeferred_.resolve); |
There was a problem hiding this comment.
I believe you want to do
variants.then(result => this.viriantsDeferred_.resolve(result)) ?
| @@ -119,7 +117,7 @@ export function allocateVariant(ampdoc, experimentName, config) { | |||
| // enumeration is implementation (browser) dependent. | |||
| const variantNames = Object.keys(config['variants']).sort(); | |||
| for (let i = 0; i < variantNames.length; i++) { | |||
| upperBound += config['variants'][variantNames[i]]; | |||
| upperBound += config['variants'][variantNames[i]].weight; | |||
There was a problem hiding this comment.
config is a JsonObject. We will need to use ['weight'] : ) Singlepass~
| variantName | ||
| ); | ||
|
|
||
| const percentage = variant.weight; |
| userAssert( | ||
| typeof percentage === 'number' && percentage > 0 && percentage < 100, | ||
| percentage && percentage > 0 && percentage < 100, |
There was a problem hiding this comment.
can we move the assert to #assertVariant() as well?
|
|
||
| // Assert the variant mutations | ||
| userAssert( | ||
| isArray(variant['mutations']), |
There was a problem hiding this comment.
what will be included in the mutations? Is it still required if it is an empty array?
There was a problem hiding this comment.
check the length of the array as well
|
|
||
| for (const name in experiments) { | ||
| if (experiments[name]) { | ||
| const variantObject = config[name]['variants'][experiments[name]]; |
|
@zhouyx I have a few questions about the comments you made, so I'll go ahead and wait till Monday, and make all the fixes at once 馃槃 No worries if you are not in the office Monday, as I could ask them later in the review 馃憤 |
|
@zhouyx Also, when you finish the review, feel free to mention me here so I get the github notification 馃槀 |
|
@zhouyx Made all requested changes, and fixed/added appropriate tests 馃槃 Please give this another review once you get the chance! |
| @@ -49,18 +50,34 @@ export class AmpExperiment extends AMP.BaseElement { | |||
| /** @private @const {!Promise<!Object<string, ?string>>} */ | |||
| const experimentVariants = Promise.all(variants) | |||
| .then(() => results) | |||
| .then(this.addToBody_.bind(this)); | |||
| .then(this.applyExperimentVariants_.bind(this, config)); | |||
There was a problem hiding this comment.
I found this.applyExperimentVariants requires two params? Did I miss anything
There was a problem hiding this comment.
Discussed offline, this does do two :)
| * @return {!Promise<!Object<string, ?string>>} a promise of the original | ||
| * param passed in | ||
| * @private | ||
| */ | ||
| addToBody_(experiments) { | ||
| applyExperimentVariants_(config, experimentToVariant) { |
There was a problem hiding this comment.
Do we really need experimentToVariant? I think config contains all the information.
There was a problem hiding this comment.
Add comment here on experimentToVariant
| throw e; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** @return {!JsonObject} [description] */ | ||
| /** | ||
| * The experiment config can be represented as: |
| // and apply mutations | ||
| // Placehodler to pass linting for code review | ||
| // and keep PRs small | ||
| body.setAttribute(experimentName, JSON.stringify(variantObject)); |
There was a problem hiding this comment.
We were using ATTR_PREFIX, could you let me know why we decide to skid the prefix? It sounds very dangerous as we won't have control of the experiment name.
There was a problem hiding this comment.
And could you explain a bit why are we setting the value to JSON.stringify(variantObject)? Thanks
There was a problem hiding this comment.
Discussed offline, make a better NOOP.
| @@ -143,12 +149,8 @@ describes.realWin('amp-experiment', { | |||
| 'experiment-2': 'variant-d', | |||
| 'experiment-3': null, | |||
| }); | |||
| expectBodyHasAttributes({ | |||
There was a problem hiding this comment.
do we still want to test the attributes appended to body?
There was a problem hiding this comment.
Nope we dont, the body thing was just a NOOP for linting
|
@zhouyx Made requested changes / replied to comments! 馃帀 This is good to go 馃槃 |
|
Look great! Thank you for the change 馃帀 |
|
@zhouyx Yay! Thanks for the review! 馃槃 Merging... |
relates to #20225
relates to #21484
This starts the config parser for the 1.0 format (see #20225). Most importantly here, this preserves the analytics, but also passes an object down to
applyMutations_()which can then use a mutation service to start ingestion of mutations (later PR).Example
First image had some testing code to show things work