search

ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

Remove support for document-level experiments (use origin experiments instead) #15827

Open mrjoro opened 6 years ago

mrjoro commented 6 years ago

Plan

After the design review (#15347) we have the following plan to resolve this issue:

Proposal

The original proposal was to do one of the following (in order of preference):

Background

AMP currently supports document-level experiments, i.e. by adding a meta "amp-experiments-opt-in" on a page the page author can enable experiments that are whitelisted for document level experiments (specified in allow-doc-opt-in in prod-config.json).

There are three main types of experiments we support that a developer can opt into:

Document-level vs. origin trial

The biggest issue with document-level experiments is that we have no good way to know everyone who is using the document-level experiment or to notify them of upcoming breaking changes. This means that a breaking change in the experiment carries the risk that a page in production breaks. In contrast, with origin trials the whitelisting process allows us to (a) require developers using the experiment to acknowledge the risks involved in using an experimental feature and (b) collect information that we could use to notify developers using the experimental feature in the case a breaking change will be made.

The main use case we've had for document-level experiments thus far has been cases where a few developers are working closely with the AMP community to refine an upcoming feature. This case is served well by the origin-trial case, and the number of these cases is not very large.

Other use cases for document-level experiments

The document-level experiment I2I (#6869) has an additional use case for the amp-inabox. I don't have complete context for that use case so I'd like some input from @lannka whether origin trials is sufficient for that case. If not we may still be able to move to the "document-level experiments don't allow breaking changes" policy.

/cc @aghassemi @cathyxz @choumx @cramforce @jridgewell @kristoferbaxter

lannka commented 6 years ago

@mrjoro sorry for not noticing this earlier. The inabox use case is to allow ad server to run AMP experiment at serving time. They put this doc-level opt-in meta tag into the AMPHTML creative.

The biggest issue with document-level experiments is that we have no good way to know everyone who is using the document-level experiment or to notify them of upcoming breaking changes.

One thing to clarify: this doc-level opt-in does not apply to all existing experiment flags. We have a whitelist to control the experiments can be opt-in at doc-level. So we should be thoughtful before whitelisting an experiment.

Anyway, I think origin trial is a good replacement for the major use case. We can limit the doc-level opt-in to only A4A runtime (inabox).

mrjoro commented 6 years ago

Would origin trial work for the inabox case?

My understanding of the origin trial is that we check a signed version of the domain+experiment id, but I'm not sure how the domain is checked. Would this use the domain of the ad for inabox? @lannka @choumx

lannka commented 6 years ago

I thought origin trial does not allow doc level opt-in. But correct me if I'm wrong.

So the use case is A/B testing, meaning ad server want to turn on an experiment on some traffic but not all.

mrjoro commented 6 years ago

The origin experiment is enabled by the publisher adding a tag (the "amp-experiment-token" meta tag if I'm reading the deleted code correctly) to the page. For A/B testing I believe the publisher could include/exclude this tag just as they can for the document-level experiment "amp-experiments-opt-in today. @choumx correct me if I'm wrong.

Also looking at the deleted code it looks like we use getSourceOrigin(win.location) for the domain. @lannka would this work as expected in the inabox case?

lannka commented 6 years ago

getSourceOrigin(win.location) does work on non-CDN URLs as well.

lannka commented 6 years ago

Another thing about the origin trial experiment is that its check call is async. (@choumx correct me if I'm wrong. )

It's a bit more difficult to use (sometimes not usable, depending on the code structure) than a sync call.

jridgewell commented 6 years ago

If security is not a necessity, we can roll our own signature verifier thats sync. This means origins will be able to forge a origin-trial, though.

mrjoro commented 6 years ago

I'm not too worried about domains forging access to origin trial experiments.

They'll have to be consciously working around the experiment system (since they'd still have to generate the token on a per-domain basis--they couldn't simply copy and paste another domain's token), so if they end up broken without being notified they shouldn't be too surprised.

I don't think we have use cases where it's really important to not allow this. If we do end up encountering that use case perhaps we can add support for it.

lannka commented 6 years ago

That addressed all my concerns :-)

mrjoro commented 6 years ago

Great! So the plan should be:

ampprojectbot commented 6 years ago

This issue hasn't been updated in awhile. @mrjoro Do you have any updates?

ampprojectbot commented 5 years ago

This issue hasn't been updated in awhile. @mrjoro Do you have any updates?

mrjoro commented 5 years ago

Now that origin experiments are back, are we ready to add the comment to prod-config.json to remind people not to add new document-level experiments (in preparation for removing support for them)?

/cc @lannka @choumx @ampproject/wg-runtime

dreamofabear commented 5 years ago

SGTM

mrjoro commented 5 years ago

I realized I never added the comment about document-level experiments being deprecated, and then I realized that JSON doesn't allow comments. :)

It also looks like people are continuing to add document-level experiments.

Should we just rely on the OWNERS who can approve changes to prod-config.json to not allow new document-level experiments?

This would be @erwinmombay @rsimha @choumx @danielrozenberg @cramforce @dvoytenko @jridgewell

jridgewell commented 5 years ago

We could also add a presubmit check. @rsimha

mrjoro commented 5 years ago

Presubmit check SGTM. Anyone objections?

mrjoro commented 4 years ago

What's the best way to add a presubmit check for this?

/cc @ampproject/wg-infra

lannka commented 4 years ago

FYI, as of now, there is only one active doc-level experiment left: amp-next-page.

samouri commented 3 years ago

Is there still more work to do here? Did we end up making the presubmit check? What is stopping us from removing the amp-next-page doc level experiment, i.e. what are the blockers?

mrjoro commented 3 years ago

@lannka is the amp-next-page experiment still active?

@rsimha do you know if a presubmit check was ever added for this?

Once that is removed, it should be a matter of adding a presubmit check to ensure no new allow-doc-opt-in experiments are added in prod-config.json.

We'd also want to update the experiment documentation to remove the reference to document opt-ins.

On the plus side, it looks like no new document-level experiments have been added in over a year without any changes. :)

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.