Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce experiment for new Fast Fetch signature verifier #11127

Merged
merged 3 commits into from Sep 8, 2017
Merged

Introduce experiment for new Fast Fetch signature verifier #11127

merged 3 commits into from Sep 8, 2017

Conversation

taymonbeal
Copy link
Member

@taymonbeal taymonbeal commented Aug 29, 2017

Once a traffic fraction is set, this will cause production traffic to start using the new signature verifier.

Related to #7618. Follow-up to #10674.

CC @ampproject/cloudflare

@@ -1961,8 +2000,22 @@ describe('amp-a4a', () => {
sandbox.restore();
});

it('should emit upgradeDelay lifecycle ping', () => {
it('should emit upgradeDelay lifecycle ping with legacy verifier', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the description here read "...with new verifier" and the test below it "...with legacy verifier"?

Copy link
Contributor

@keithwrightbos keithwrightbos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding off on merge until we all agree testing shows proper behavior.

} else if (/a4a-legacy-verifier/.test(win.location.hash)) {
forceExperimentBranch(win, VERIFIER_EXP_NAME, LEGACY_VERIFIER_EID);
} else {
randomlySelectUnsetExperiments(win, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't actually need to put this in the else given randomlySelectUnsetExperiments should do nothing if you've already forced the experiment on.

return win[propertyName] ||
(win[propertyName] = new LegacySignatureVerifier(win));
if (!win[propertyName]) {
if (/a4a-new-verifier/.test(win.location.hash)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let expId = (/a4a-new-verifier/.test(win.location.hash) && NEW_VERIFIER_EID) || (/a4a-legacy-verifier/.test(win.location.hash) && LEGACY_VERIFIER_EID);
if (expId) {
forceExperimentBranch(win, VERIFIER_EXP_NAME, expId);
}
randomlySelectUnset...

@keithwrightbos keithwrightbos merged commit 8d5e0a7 into ampproject:master Sep 8, 2017
@keithwrightbos keithwrightbos deleted the signature-verifier-experiment branch September 8, 2017 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants