✨amp-experiment 1.0: Allow an `#amp-x_disable_all_experiments_` to disable all experiments by torch2424 · Pull Request #22419 · ampproject/amphtml

torch2424 · 2019-05-21T21:54:41Z

relates to #20225
relates to #21705

This allows adding a #amp-x_disable_all_experiments_ to disable all of the experiments that would be run normally by amp-experiment. Part of this, is ensuring the variants service returns an experimentToVariant with null values to ensure the analytics sets the experiment to none.

Examples

Need to test both the disable all works, and the variant gives an object where all values are null

…sable-all-experiments

torch2424 · 2019-05-30T12:47:29Z

@zhouyx This is good to go / ready for review! 😄

zhouyx

The change looks good. Two question, 1. Should the #amp-x_disable_all_experiments_ works along with amp-x-experiment=name?
2. would #amp-x-disable-all-experiments be a better name than #amp-x_disable_al_experiments_?
Thanks

zhouyx · 2019-05-30T23:27:48Z

+
+          // Ensure downstream consumers don't wait for the promise forever.
+          variantsService.init(
+            Promise.resolve(this.getEmptyExperimentToVariant_(config))


question: Can we resolve with Promise.resolve({}) instead. What is the difference between {} and this.getEmptyExperimentToVariant_(config)?

So the reason why I changed this to this.getEmptyExperimentToVariant_(config), is because doing Promise.resolve({}) ends up throwing an error when trying to send out analytics ping.

Thus, I figured people would want to test that the ping is being sent out even when experiments are disabled. And since they are being disabled with this.getEmptyExperimentToVariant_(config), it sets the param to equal none. Meaning the experiment was not applied 😄

Let me know if this makes sense. I personally think that getting a ping that essentially says "all experiments are not being applied" is better than just throwing an error from the analytics side of things.

Let me know if this makes sense. I personally think that getting a ping that essentially says "all experiments are not being applied" is better than just throwing an error from the analytics side of things.

Oh definitely! A few thoughts:

Do you think it's worth it to cache the emptyExperimentToVariant object so that we don't need to create it every time? Really depends on how many times do you expect the function to be called.

In the case where 'amp-experiment-1.0' is not enabled. Do we expect that as user error? If so on the analytics side should we throw error or still send ping says "all experiments are not being applied"?

@zhouyx

Do you think it's worth it to cache the emptyExperimentToVariant object so that we don't need to create it every time? Really depends on how many times do you expect the function to be called.

Nope, because it is and always should only be called once. 😄

In the case where 'amp-experiment-1.0' is not enabled. Do we expect that as user error? If so on the analytics side should we throw error or still send ping says "all experiments are not being applied"?

Ummmm I think we should still only throw an error on the experiment side. As this is an amp-experiment error, and not an amp-analytics error. And a ping saying all experiments are not being applied is not wrong. 😄

zhouyx · 2019-05-30T23:29:33Z

          return allocateVariant(
-            this.getAmpDoc(),
+            ampdoc,
+            viewer,


nit: Do we really need to pass viewer to the function? We could easily get the service from ampdoc.

So I decided to pass it in, for two reasons:

We need access to viewer anyways now to check for _disable_all_experiments_ on the URL. And I thought it would be better practice to pass it in to remove dependencies from our help 'variants.ts' file.

We want to be sensitive about performance. Trying to re-grab the viewer every single time would be slower than just passing it in.

Totally open to doing it inside of allocateVariant though. It is up to you 😄 But personally, I think this is the way to go for now unless you have strong feelings about it.

I don't have a strong feeling about this. So feel free to merge.
But I'd be curious to know the performance difference between passing the viewer versus getting the viewer from ampdoc.
Personally I imagine Services.viewerForDoc(ampdoc) is very cheap as it's only reading some stored value from doc.

@choumx Could you shed us some light on the performance difference between the two. 1. passing as an argument, 2. retrieving using ampdoc.

@zhouyx

Asked @kristoferbaxter on this since they are also with me in Berlin 😄

But they stated performance wise the change would be negligible, but mentioned they would personally hoist it out of the loop haha 😂

I think we all don't have strong feelings on this, so, I'll go ahead and merge, and we can resolve if needed back in California 😄

zhouyx · 2019-05-30T23:30:22Z

  <script async custom-element="amp-user-notification" src="https://cdn.ampproject.org/v0/amp-user-notification-0.1.js"></script>
  <script async src="https://cdn.ampproject.org/v0.js"></script>
  <style amp-custom>
+    amp-user-notification {


What's the change for?

This is for the manual testing example, to add a background to the "allow background color" setting for the experiment. It was just hard to see without any styling so I added that 😄

torch2424 · 2019-05-31T11:24:54Z

@zhouyx To reply to your last two questions:

"1. Should the #amp-x_disable_all_experiments_ works along with amp-x-experiment=name?"

So this is something @jeffjose and I discussed. This will conflict with any experiment named _disable_all_experiments_. Which is why we surrounded it with _ and made it very verbose. After seeing the code in how hash params are ahndled, we can totally change this to something like: amp-experiment-disable-all-experiments=true, which won't have any conflicts. But I think it may make more sense to retain the amp-x- prefix as it stays consistent. Open to either one 😄

"2. would #amp-x-disable-all-experiments be a better name than #amp-x-disable_al_experiments?"

As stated above, using underscores to make it less likely to conflict 😄 If we decided to keep the amp-x- prefeix, I'd vote for keeping underscores unless you have strong feelings about this.

torch2424 · 2019-05-31T11:25:36Z

@zhouyx Replied to all comments 😂 Totally open to making changes if you have strong feelings about something, Let me know what you think!

zhouyx · 2019-06-01T00:30:04Z

"1. Should the #amp-x_disable_all_experiments_ works along with amp-x-experiment=name?"
So this is something @jeffjose and I discussed. This will conflict with any experiment named disable_all_experiments. Which is why we surrounded it with _ and made it very verbose. After seeing the code in how hash params are ahndled, we can totally change this to something like: amp-experiment-disable-all-experiments=true, which won't have any conflicts. But I think it may make more sense to retain the amp-x- prefix as it stays consistent. Open to either one 😄

I agree that's a concern. The only thing I don't like about #amp-x_disable_all_experiments_ is that it combines hyphen and underscore and I assume it's very easy to type it wrong 😢 . But I agree it makes more sense to retain the amp-x- prefix. I don't have a strong opinion on the naming : ) I'll let you decide.

Actually I was going to ask a different question : ) Wanted to ask what if we have #amp-x_disable_all_experiments&amp-x-experiment=name, should we respect both? Or amp-x_disable_all_experiment_ can override the other value.

jeffjose · 2019-06-01T05:30:14Z

Sorry, didnt see it just in time but another naming option we can consider is i-amp-experiment-disable. We already use i-amphtml-* in CSS classes, and i- is meant to be internal.

torch2424 · 2019-06-03T15:35:56Z

@zhouyx

Wanted to ask what if we have #amp-x_disable_all_experiments&amp-x-experiment=name, should we respect both? Or amp-x_disable_all_experiment_ can override the other value.

I think we should disable all still. Since amp-x-experiment= means, hey, "disable all experiments except this one". And "amp-x_disable_all_experiments" means disable all the experiments. I think logically it would make more sense to respect the "more powerful param" for a lack of better words 😂

I agree that's a concern. The only thing I don't like about #amp-x_disable_all_experiments_ is that it combines hyphen and underscore and I assume it's very easy to type it wrong 😢 . But I agree it makes more sense to retain the amp-x- prefix. I don't have a strong opinion on the naming : ) I'll let you decide.

Yeah I agree with that 🤔 I'll go with @jeffjose solution as it avoid conflicts, and honestly should probably only be used by vendors. We can always change this before going to PROD with docs, since it would be a one line change.

On that note, will make these changes, and merge 😄 Thank you everyone!

torch2424 · 2019-06-03T16:10:56Z

@jeffjose @zhouyx

One last note, couldn't rename to i-:

And so I just renamed to amp-x-disable-all-experiments. And like I said above, we can bikeshed the name later once I get back to California 😄

zhouyx · 2019-06-03T20:27:07Z

Talked to @jeffjose about using the i-amphtml-*. i-* is supposed for internal use only, we need to avoid using that prefix as a user opt-in flag. amp-x-disable-all-experiments sounds good.

zhouyx · 2019-06-03T20:36:22Z

And not blocking this PR.

I think we should disable all still. Since amp-x-experiment= means, hey, "disable all experiments except this one". And "amp-x_disable_all_experiments" means disable all the experiments. I think logically it would make more sense to respect the "more powerful param" for a lack of better words 😂

Looking at the doc https://github.com/ampproject/amphtml/blob/master/extensions/amp-experiment/amp-experiment.md#style-a-variant, I don't think amp-x- is used to disable other experiments, but to specify variant. Please correct me if that's not the case.

I still think it makes sense to support some thing like #amp-x-disable-all&amp-x-Aexperiment='treatment1' in the future. So one could disable all experiment except experimentA. What do you think?

torch2424 · 2019-06-04T12:41:45Z

@zhouyx

Talked to @jeffjose about using the i-amphtml-. i- is supposed for internal use only, we need to avoid using that prefix as a user opt-in flag. amp-x-disable-all-experiments sounds good.

🎉 🎉 🎉

I still think it makes sense to support some thing like #amp-x-disable-all&amp-x-Aexperiment='treatment1' in the future. So one could disable all experiment except experimentA. What do you think?

Ah, well you make a good point. Especially with the docs. I don't mind implementing this in the future 😄

I went ahead and added this to the tracking issue: #21705

Going to merge this PR! Thanks for looking into this 😄 👍 🎉

torch2424 added 5 commits May 20, 2019 18:15

Allowed disabling all experiments

8e6bb83

FIxed variant tests, add amp-experiment tests

daebc29

Need to test both the disable all works, and the variant gives an object where all values are null

Fixed up some weird config errors

fc8fec2

Fixed all the tests, and added the disable all tests

3f9285a

Fixed precommit checks

f122269

torch2424 requested a review from zhouyx May 21, 2019 21:54

googlebot added the cla: yes label May 21, 2019

torch2424 added 3 commits May 30, 2019 12:50

Merge branch 'master' of https://github.com/torch2424/amphtml into di…

247bcc9

…sable-all-experiments

Fixed the merge conflict tests

1c611ec

Fixed all post merge master errors

9312d0c

zhouyx suggested changes May 30, 2019

View reviewed changes

zhouyx approved these changes Jun 1, 2019

View reviewed changes

Changed to dashes, couldn't use i-amphtml

d1f2ee8

torch2424 mentioned this pull request Jun 4, 2019

amp-experiment: Origin Trial Tracker #21705

Closed

12 tasks

torch2424 merged commit c23c179 into ampproject:master Jun 4, 2019

torch2424 deleted the disable-all-experiments branch June 4, 2019 12:41

Conversation

torch2424 commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Examples

Uh oh!

torch2424 commented May 30, 2019

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torch2424 May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torch2424 commented May 31, 2019

Uh oh!

torch2424 commented May 31, 2019

Uh oh!

zhouyx commented Jun 1, 2019

Uh oh!

jeffjose commented Jun 1, 2019

Uh oh!

torch2424 commented Jun 3, 2019

Uh oh!

torch2424 commented Jun 3, 2019

Uh oh!

zhouyx commented Jun 3, 2019

Uh oh!

zhouyx commented Jun 3, 2019

Uh oh!

torch2424 commented Jun 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

torch2424 commented May 21, 2019 •

edited

Loading

torch2424 May 31, 2019 •

edited

Loading