mozilla
/
fxa
зеркало из https://github.com/mozilla/fxa.git
1
0
Форкнуть 0
Код Задачи Пакеты Проекты Релизы Вики Активность

Merge pull request #3570 from mozilla/auto-createExperiment-for-manual-experiments r=@vbudhram 

refactor(experiments): Simplify how experiments and metrics are created
This commit is contained in:
Shane Tomlinson 2019-12-04 19:59:28 +00:00 коммит произвёл GitHub
Родитель 00b154a0bb 08b186f453
Коммит c2f3e1c444
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: 4AEE18F83AFDEB23
11 изменённых файлов: 146 добавлений и 223 удалений
Показать статистику Скачать Patch файл Скачать Diff файл Показать все файлы Свернуть все файлы

68
packages/fxa-content-server/app/scripts/lib/experiment.js
Просмотреть файл

@ -16,8 +16,8 @@ const UA_OVERRIDE = 'FxATester';
const STARTUP_EXPERIMENTS = {};
/**
* Experiments created manually by calling `createExperiment`,
* after the app has started.
* Experiments created manually by calling `getAndReportExperimentGroup`
* or `createExperiment` after the app has started.
*/
const MANUAL_EXPERIMENTS = {
emailMxValidation: BaseExperiment,
@ -150,11 +150,7 @@ _.extend(
// eslint-disable-next-line no-unused-vars
for (const experimentName in this._startupExperiments) {
const groupType = this.getExperimentGroup(experimentName);
if (groupType) {
this.createExperiment(experimentName, groupType);
}
this.getAndReportExperimentGroup(experimentName);
}
},
@ -173,6 +169,10 @@ _.extend(
}
const ExperimentConstructor = this._allExperiments[experimentName];
if (_.isFunction(ExperimentConstructor)) {
// force the flow model to be initialized so that
// the experiment is logged.
this.notifier.trigger('flow.initialize');
const experiment = new ExperimentConstructor();
const initResult = experiment.initialize(experimentName, {
groupType,
@ -197,11 +197,14 @@ _.extend(
},
/**
* Get the experiment group for `experimentName` the user is in.
* Get the experiment group for the user is in for `experimentName`.
* **NOTE** Does not report experiment or group to Amplitude. Use
* `getAndReportExperimentGroup` or `createExperiment` to ensure experiment
* and group name are reported.
*
* @param {String} experimentName
* @param {Object} [additionalInfo] additional info to pass to the experiment grouping rule.
* @returns {String}
* @returns {false|String}
*/
getExperimentGroup(experimentName, additionalInfo = {}) {
// can't be in an experiment group if not initialized.
@ -209,23 +212,36 @@ _.extend(
return false;
}
return this.experimentGroupingRules.choose(
experimentName,
_.extend(
{
account: this.account,
// yes, this is a hack because experiments do not have a reference
// to experimentGroupingRules internally. This allows experiments to reference other
// experiments
experimentGroupingRules: this.experimentGroupingRules,
forceExperiment: this.forceExperiment,
forceExperimentGroup: this.forceExperimentGroup,
isMetricsEnabledValue: this.metrics.isCollectionEnabled(),
uniqueUserId: this.user.get('uniqueUserId'),
},
additionalInfo
)
);
return this.experimentGroupingRules.choose(experimentName, {
account: this.account,
// yes, this is a hack because experiments do not have a reference
// to experimentGroupingRules internally. This allows experiments to reference other
// experiments
experimentGroupingRules: this.experimentGroupingRules,
forceExperiment: this.forceExperiment,
forceExperimentGroup: this.forceExperimentGroup,
isMetricsEnabledValue: this.metrics.isCollectionEnabled(),
uniqueUserId: this.user.get('uniqueUserId'),
...additionalInfo,
});
},
/**
* Get the experiment group for the user is in for `experimentName` and
* if part of an experiment, report the experiment name and group to Amplitude.
*
* @param {String} experimentName
* @param {Object} [additionalInfo] additional info to pass to the experiment grouping rule.
* @returns {false|String}
*/
getAndReportExperimentGroup(experimentName, additionalInfo) {
const groupType = this.getExperimentGroup(experimentName, additionalInfo);
if (!groupType) {
return false;
}
this.createExperiment(experimentName, groupType);
return groupType;
},
}
);

7
packages/fxa-content-server/app/scripts/views/mixins/connect-another-device-mixin.js
Просмотреть файл

@ -106,7 +106,10 @@ export default {
return;
}
const group = this.getExperimentGroup('sendSms', { account, country });
const group = this.getAndReportExperimentGroup('sendSms', {
account,
country,
});
if (!group) {
// Auth server said "OK" but user was not selected
// for the experiment, this mode is not logged in
@ -116,8 +119,6 @@ export default {
return country;
} else {
// User is eligible and a member of the experiment.
this.createExperiment('sendSms', group);
if (group === 'control') {
this.logFlowEvent(REASON_CONTROL_GROUP);
} else {

11
packages/fxa-content-server/app/scripts/views/mixins/cwts-on-signup-password-experiment-mixin.js
Просмотреть файл

@ -23,11 +23,6 @@ export default {
dependsOn: [ExperimentMixin, SyncOptionalMixin],
setInitialContext(context) {
const experimentGroup = this.getCWTSOnSignupPasswordExperimentGroup();
if (experimentGroup) {
this.createExperiment(EXPERIMENT_NAME, experimentGroup);
}
if (this.isCWTSOnSignupPasswordEnabled()) {
context.set({
engines: this._getOfferedEngines(),
@ -37,11 +32,11 @@ export default {
},
isCWTSOnSignupPasswordEnabled() {
return this.getCWTSOnSignupPasswordExperimentGroup() === 'treatment';
return this.chooseCWTSOnSignupPasswordExperimentGroup() === 'treatment';
},
getCWTSOnSignupPasswordExperimentGroup() {
return this.getExperimentGroup(EXPERIMENT_NAME, {
chooseCWTSOnSignupPasswordExperimentGroup() {
return this.getAndReportExperimentGroup(EXPERIMENT_NAME, {
email: this.getAccount().get('email'),
multiService: this.relier.get('multiService'),
service: this.relier.get('service'),

11
packages/fxa-content-server/app/scripts/views/mixins/email-mx-validation-experiment-mixin.js
Просмотреть файл

@ -9,14 +9,7 @@ export default {
dependsOn: [ExperimentMixin],
isInEmailMxValidationExperimentTreatment() {
const isInExperiment = this.isInExperiment(EXPERIMENT_NAME);
if (isInExperiment) {
const experimentGroup = this.getExperimentGroup(EXPERIMENT_NAME);
this.createExperiment(EXPERIMENT_NAME, experimentGroup);
return experimentGroup === 'treatment';
}
return false;
const experimentGroup = this.getAndReportExperimentGroup(EXPERIMENT_NAME);
return experimentGroup === 'treatment';
},
};

42
packages/fxa-content-server/app/scripts/views/mixins/experiment-mixin.js
Просмотреть файл

@ -57,37 +57,21 @@ const ExperimentMixin = {
this.experiments = null;
}
},
/**
* Create an experiment and add it to the list of active experiments.
* Only creates an experiment with `experimentName` once.
* This is useful to create an experiment that is not created
* at startup.
*
* @param {String} experimentName - name of experiment to create.
* @param {String} groupType - which group the user is in.
* @returns {Object} experiment object, if created.
*/
createExperiment(...args) {
if (this.experiments) {
// force the flow model to be initialized so that
// the experiment is logged.
this.notifier.trigger('flow.initialize');
return this.experiments.createExperiment(...args);
}
},
};
// Create some delegate methods.
['getExperimentGroup', 'isInExperiment', 'isInExperimentGroup'].forEach(
methodName => {
ExperimentMixin[methodName] = function(...args) {
if (this.experiments) {
return this.experiments[methodName](...args);
}
};
}
);
[
'getAndReportExperimentGroup',
'createExperiment',
'getExperimentGroup',
'isInExperiment',
'isInExperimentGroup',
].forEach(methodName => {
ExperimentMixin[methodName] = function(...args) {
if (this.experiments) {
return this.experiments[methodName](...args);
}
};
});
export default ExperimentMixin;

67
packages/fxa-content-server/app/scripts/views/mixins/token-code-experiment-mixin.js
Просмотреть файл

@ -1,67 +0,0 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
/**
* An TokenCodeExperimentMixin factory.
*
* @mixin TokenCodeExperimentMixin
*/
import ExperimentMixin from './experiment-mixin';
const EXPERIMENT_NAME = 'tokenCode';
/**
* Creates the mixin
*
* @returns {Object} mixin
*/
export default {
dependsOn: [ExperimentMixin],
beforeRender() {
if (this.isInTokenCodeExperiment()) {
const experimentGroup = this.getTokenCodeExperimentGroup();
this.createExperiment(EXPERIMENT_NAME, experimentGroup);
}
},
/**
* Get token code experiment group
*
* @returns {String}
*/
getTokenCodeExperimentGroup() {
return this.getExperimentGroup(
EXPERIMENT_NAME,
this._getTokenCodeExperimentSubject()
);
},
/**
* Is the user in the token code experiment?
*
* @returns {Boolean}
*/
isInTokenCodeExperiment() {
return this.isInExperiment(
EXPERIMENT_NAME,
this._getTokenCodeExperimentSubject()
);
},
/**
* Get the token code experiment choice subject
*
* @returns {Object}
* @private
*/
_getTokenCodeExperimentSubject() {
const subject = {
account: this.model.get('account'),
clientId: this.relier.get('clientId'),
isTokenCodeSupported: this.broker.getCapability('tokenCode'),
service: this.relier.get('service'),
};
return subject;
},
};

41
packages/fxa-content-server/app/tests/spec/lib/experiment.js
Просмотреть файл

@ -37,6 +37,8 @@ describe('lib/experiment', () => {
});
notifier = new Notifier();
sinon.stub(notifier, 'trigger');
metrics = new Metrics({ notifier });
expOptions = {
experimentGroupingRules,
@ -86,12 +88,13 @@ describe('lib/experiment', () => {
});
describe('createExperiment', () => {
it('creates an experiment, only once', () => {
it('creates an experiment, only once, notifies of flow.initialize', () => {
const firstExperiment = expInt.createExperiment('sendSms', 'treatment');
assert.ok(firstExperiment);
const secondExperiment = expInt.createExperiment('sendSms', 'treatment');
// It's the same object, not updated
assert.strictEqual(firstExperiment, secondExperiment);
assert.isTrue(notifier.trigger.calledOnceWith('flow.initialize'));
});
});
@ -193,6 +196,42 @@ describe('lib/experiment', () => {
});
});
describe('getAndReportExperimentGroup', () => {
const additionalInfo = {
foo: 'bar',
};
beforeEach(() => {
sinon.stub(expInt, 'createExperiment');
});
it('returns false if no group', () => {
sinon.stub(expInt, 'getExperimentGroup').returns(false);
assert.isFalse(
expInt.getAndReportExperimentGroup('exp-name', additionalInfo)
);
assert.isTrue(
expInt.getExperimentGroup.calledOnceWith('exp-name', additionalInfo)
);
assert.isFalse(expInt.createExperiment.called);
});
it('returns the group name, creates an experiment, if part of the experiment', () => {
sinon.stub(expInt, 'getExperimentGroup').returns('treatment');
assert.equal(
expInt.getAndReportExperimentGroup('exp-name', additionalInfo),
'treatment'
);
assert.isTrue(
expInt.getExperimentGroup.calledOnceWith('exp-name', additionalInfo)
);
assert.isTrue(
expInt.createExperiment.calledOnceWith('exp-name', 'treatment')
);
});
});
describe('destroy', () => {
beforeEach(() => {
expInt._activeExperiments = {

39
packages/fxa-content-server/app/tests/spec/views/mixins/connect-another-device-mixin.js
Просмотреть файл

@ -335,21 +335,20 @@ describe('views/mixins/connect-another-device-mixin', () => {
sinon
.stub(view, '_isEligibleForSms')
.callsFake(() => Promise.resolve({ country: 'GB', ok: true }));
sinon.stub(view, 'createExperiment').callsFake(() => {});
sinon.spy(notifier, 'trigger');
});
describe('user is not part of experiment', () => {
it('does not enroll the user in an experiment, logs a flow event', () => {
sinon.stub(view, 'getExperimentGroup').callsFake(() => false);
it('logs a flow event', () => {
sinon
.stub(view, 'getAndReportExperimentGroup')
.callsFake(() => false);
return view.getEligibleSmsCountry(account).then(country => {
assert.isUndefined(country);
assert.isTrue(notifier.trigger.calledOnce);
assert.isTrue(notifier.trigger.calledWith('flow.initialize'));
assert.isFalse(view.createExperiment.called);
assert.isTrue(view.logFlowEvent.calledOnce);
assert.isTrue(
view.logFlowEvent.calledWith('sms.ineligible.not_in_experiment')
@ -359,43 +358,37 @@ describe('views/mixins/connect-another-device-mixin', () => {
});
describe('country is fully rolled out', () => {
it('does not create the experiment, returns the country', () => {
sinon.stub(view, 'getExperimentGroup').callsFake(() => true);
it('returns the country', () => {
sinon
.stub(view, 'getAndReportExperimentGroup')
.callsFake(() => true);
return view.getEligibleSmsCountry(account).then(country => {
assert.equal(country, 'GB');
assert.isFalse(view.createExperiment.called);
assert.isFalse(view.logFlowEvent.called);
});
});
});
describe('in treatment group', () => {
it('creates the experiment, returns the country', () => {
sinon.stub(view, 'getExperimentGroup').callsFake(() => 'treatment');
it('returns the country', () => {
sinon
.stub(view, 'getAndReportExperimentGroup')
.callsFake(() => 'treatment');
return view.getEligibleSmsCountry(account).then(country => {
assert.equal(country, 'GB');
assert.isTrue(view.createExperiment.calledOnce);
assert.isTrue(
view.createExperiment.calledWith('sendSms', 'treatment')
);
});
});
});
describe('in control group', () => {
it('creates the experiment, does not return a country', () => {
sinon.stub(view, 'getExperimentGroup').callsFake(() => 'control');
it('does not return a country', () => {
sinon
.stub(view, 'getAndReportExperimentGroup')
.callsFake(() => 'control');
return view.getEligibleSmsCountry(account).then(country => {
assert.isUndefined(country);
assert.isTrue(view.createExperiment.calledOnce);
assert.isTrue(
view.createExperiment.calledWith('sendSms', 'control')
);
assert.isTrue(view.logFlowEvent.calledOnce);
assert.isTrue(
view.logFlowEvent.calledWith('sms.ineligible.control_group')

33
packages/fxa-content-server/app/tests/spec/views/mixins/cwts-on-signup-password-experiment-mixin.js
Просмотреть файл

@ -22,8 +22,8 @@ describe('views/mixins/cwts-on-signup-password-experiment-mixin', () => {
email: 'foomail',
});
context = new Model();
sinon.stub(context, 'set');
view = new SignupPasswordView({});
view.createExperiment = sinon.spy();
view.getAccount = () => account;
view.relier = new Model({
service: 'sync',
@ -37,62 +37,57 @@ describe('views/mixins/cwts-on-signup-password-experiment-mixin', () => {
describe('setInitialContext', () => {
it('does not initialize experiment or set context if not part of a group', () => {
sinon
.stub(view, 'getCWTSOnSignupPasswordExperimentGroup')
.stub(view, 'chooseCWTSOnSignupPasswordExperimentGroup')
.callsFake(() => false);
view.setInitialContext(context);
assert.isFalse(view.createExperiment.called);
assert.isFalse(context.set.called);
});
it('creates the experiment if part of control group, does not set context', () => {
it('does not set context if part of the control group', () => {
sinon
.stub(view, 'getCWTSOnSignupPasswordExperimentGroup')
.stub(view, 'chooseCWTSOnSignupPasswordExperimentGroup')
.callsFake(() => 'control');
view.setInitialContext(context);
assert.isTrue(
view.createExperiment.calledOnceWith('signupPasswordCWTS', 'control')
);
assert.lengthOf(Object.keys(context.toJSON()), 0);
assert.isFalse(context.set.called);
});
it('creates the experiment if part of treatment group, sets context', () => {
sinon
.stub(view, 'getCWTSOnSignupPasswordExperimentGroup')
.stub(view, 'chooseCWTSOnSignupPasswordExperimentGroup')
.callsFake(() => 'treatment');
sinon.stub(view, '_getOfferedEngines').callsFake(() => ['foo', 'bar']);
view.setInitialContext(context);
assert.isTrue(
view.createExperiment.calledOnceWith('signupPasswordCWTS', 'treatment')
context.set.calledOnceWith({
engines: ['foo', 'bar'],
isCWTSOnSignupPasswordEnabled: true,
})
);
assert.deepEqual(context.toJSON(), {
engines: ['foo', 'bar'],
isCWTSOnSignupPasswordEnabled: true,
});
});
});
describe('isCWTSOnSignupPasswordEnabled', () => {
it('returns false if not part of the experiment', () => {
sinon
.stub(view, 'getCWTSOnSignupPasswordExperimentGroup')
.stub(view, 'chooseCWTSOnSignupPasswordExperimentGroup')
.callsFake(() => false);
assert.isFalse(view.isCWTSOnSignupPasswordEnabled());
});
it('returns false if part of control group', () => {
sinon
.stub(view, 'getCWTSOnSignupPasswordExperimentGroup')
.stub(view, 'chooseCWTSOnSignupPasswordExperimentGroup')
.callsFake(() => 'control');
assert.isFalse(view.isCWTSOnSignupPasswordEnabled());
});
it('returns true if part of treatment group', () => {
sinon
.stub(view, 'getCWTSOnSignupPasswordExperimentGroup')
.stub(view, 'chooseCWTSOnSignupPasswordExperimentGroup')
.callsFake(() => 'treatment');
assert.isTrue(view.isCWTSOnSignupPasswordEnabled());
});

29
packages/fxa-content-server/app/tests/spec/views/mixins/email-mx-validation-experiment-mixin.js
Просмотреть файл

@ -9,8 +9,6 @@ import Mixin from 'views/mixins/email-mx-validation-experiment-mixin';
import sinon from 'sinon';
import TestTemplate from 'templates/test_template.mustache';
const EXPERIMENT_NAME = 'emailMxValidation';
const View = BaseView.extend({
template: TestTemplate,
});
@ -32,6 +30,7 @@ const createViewWithExperiments = experiments => {
const createMockExperiments = mock => {
const defaultMock = {
chooseExperiments: sinon.stub(),
getAndReportExperimentGroup: sinon.stub(),
createExperiment: sinon.stub(),
destroy() {},
getExperimentGroup: sinon.stub(),
@ -60,38 +59,28 @@ describe('views/mixins/email-mx-validation-experiment-mixin', () => {
it('returns false when user is not in the treatment group', () => {
const CONTROL_GROUP_NAME = 'control';
const isInExperiment = sinon.stub().returns(true);
const getExperimentGroup = sinon.stub().returns(CONTROL_GROUP_NAME);
const createExperiment = sinon.stub();
const getAndReportExperimentGroup = sinon
.stub()
.returns(CONTROL_GROUP_NAME);
view = createViewWithExperiments(
createMockExperiments({
isInExperiment,
createExperiment,
getExperimentGroup,
getAndReportExperimentGroup,
})
);
assert.isFalse(view.isInEmailMxValidationExperimentTreatment());
assert.isTrue(
createExperiment.calledOnceWith(EXPERIMENT_NAME, CONTROL_GROUP_NAME)
);
});
it('returns true when user is in the treatment group', () => {
const TREATMENT_GROUP_NAME = 'treatment';
const isInExperiment = sinon.stub().returns(true);
const getExperimentGroup = sinon.stub().returns(TREATMENT_GROUP_NAME);
const createExperiment = sinon.stub();
const getAndReportExperimentGroup = sinon
.stub()
.returns(TREATMENT_GROUP_NAME);
view = createViewWithExperiments(
createMockExperiments({
isInExperiment,
createExperiment,
getExperimentGroup,
getAndReportExperimentGroup,
})
);
assert.isTrue(view.isInEmailMxValidationExperimentTreatment());
assert.isTrue(
createExperiment.calledOnceWith(EXPERIMENT_NAME, TREATMENT_GROUP_NAME)
);
});
});
});

21
packages/fxa-content-server/app/tests/spec/views/mixins/experiment-mixin.js
Просмотреть файл

@ -30,6 +30,7 @@ describe('views/mixins/experiment-mixin', () => {
// a spy can be added to `chooseExperiments`
experiments = {
chooseExperiments: sinon.spy(),
getAndReportExperimentGroup: sinon.spy(),
createExperiment: sinon.spy(() => {
return {};
}),
@ -135,26 +136,10 @@ describe('views/mixins/experiment-mixin', () => {
});
});
describe('createExperiment', () => {
it('forces the flow model to initialize, then creates the experiment', () => {
assert.ok(view.createExperiment('experimentName', 'control'));
assert.isTrue(notifier.trigger.calledOnce);
assert.isTrue(notifier.trigger.calledWith('flow.initialize'));
assert.isTrue(experiments.createExperiment.calledOnce);
assert.isTrue(
experiments.createExperiment.calledWith('experimentName', 'control')
);
});
it('does nothing if the view has been destroyed', () => {
view.destroy();
assert.notOk(view.createExperiment('experimentName', 'control'));
});
});
describe('delegate methods', () => {
const delegateMethods = [
'getAndReportExperimentGroup',
'createExperiment',
'getExperimentGroup',
'isInExperiment',
'isInExperimentGroup',