From 86c86635dbbc7ccab48031c5b1ab55809e62af73 Mon Sep 17 00:00:00 2001 From: Ian Bicking Date: Fri, 13 Jan 2017 14:14:21 -0600 Subject: [PATCH] Fix #2077, design and implement an A/B testing system People are put into tests by the server at login time Tests may stick to the shots created when the test is in place, then viewers will be associated with that test Tests each map to a GA field (cdX for some value of X) --- addon/lib/req.js | 4 +++ addon/lib/shooter.js | 12 +++++++- addon/lib/user.js | 28 ++++++++++++++---- server/src/ab-tests.js | 45 +++++++++++++++++++++++++---- server/src/ga-activation.js | 10 ++++++- server/src/pages/shot/controller.js | 16 +++++++++- server/src/pages/shotindex/view.js | 1 - server/src/reactrender.js | 6 ++-- server/src/server.js | 34 +++++++++++++++++----- server/src/users.js | 26 ++++++++++++----- shared/shot.js | 20 ++++++++++++- 11 files changed, 168 insertions(+), 34 deletions(-) diff --git a/addon/lib/req.js b/addon/lib/req.js index cbc7b536e9..9c5de3733d 100644 --- a/addon/lib/req.js +++ b/addon/lib/req.js @@ -69,6 +69,10 @@ exports.sendEvent = function (action, label, options) { options = options || {}; options.applicationName = "firefox"; options.applicationVersion = self.version; + let abTests = user.getAbTests(); + for (let testName in abTests) { + options[abTests[testName].gaField] = abTests[testName].value; + } let showOptions = Object.keys(options).length > 2; console.info(`sendEvent ${event}/${action}/${label || 'none'} ${showOptions ? JSON.stringify(options) : ""}`); exports.request(`${main.getBackend()}/event`, { diff --git a/addon/lib/shooter.js b/addon/lib/shooter.js index 860ec80cfb..1fc3b51592 100644 --- a/addon/lib/shooter.js +++ b/addon/lib/shooter.js @@ -16,7 +16,7 @@ const { Class } = require('sdk/core/heritage'); const { watchPromise, watchFunction, watchWorker } = require("./errors"); const clipboard = require("sdk/clipboard"); const { AbstractShot } = require("./shared/shot"); -const { getDeviceIdInfo } = require("./user"); +const { getDeviceIdInfo, getAbTests } = require("./user"); const { URL } = require("sdk/url"); const notifications = require("sdk/notifications"); const { randomString } = require("./randomstring"); @@ -107,6 +107,16 @@ const ShotContext = Class({ url: this.tabUrl, deviceId: deviceIdInfo.deviceId }); + let shotAbTests = {}; + let userAbTests = getAbTests(); + for (let testName in userAbTests) { + if (userAbTests[testName].shotField) { + shotAbTests[testName] = userAbTests[testName].value; + } + } + if (Object.keys(shotAbTests).length) { + this.shot.abTests = shotAbTests; + } this._deregisters = []; this._workerActive = false; this.watchTab("pageshow", function (tab) { diff --git a/addon/lib/user.js b/addon/lib/user.js index 8dcbbf00f3..8b62b1b20a 100644 --- a/addon/lib/user.js +++ b/addon/lib/user.js @@ -12,6 +12,7 @@ const { hasCookieForBackend } = require("./get-cookies"); let initialized = false; let sentryPublicDSN = "https://d58fc53f92cd47bba7266ad6444514d8@sentry.prod.mozaws.net/74"; +let abTests = {}; function getDeviceIdInfo() { let info = prefs.deviceIdInfo || "null"; @@ -34,6 +35,21 @@ exports.getSentryPublicDSN = function() { return sentryPublicDSN; }; +function setVariablesFromServer(responseJson) { + // FIXME: should send Raven/Sentry messages in case of any error + try { + sentryPublicDSN = responseJson.sentryPublicDSN; + console.info("got sentry DSN response from server"); + } catch (e) { + console.error("Error looking for the sentry DSN", e); + } + try { + abTests = responseJson.abTests; + } catch (e) { + console.error("Error looking for the A/B tests", e); + } +} + exports.deleteEverything = function () { let backend = require("./main").getBackend(); setDeviceIdInfo(null); @@ -111,12 +127,7 @@ exports.initialize = function (backend, reason) { } initialized = true; console.info("logged in with cookie:", !!response.headers["Set-Cookie"]); - try { - sentryPublicDSN = response.json.sentryPublicDSN; - console.info("got sentry DSN response from server"); - } catch (e) { - console.error("Error looking for the sentry DSN", e); - } + setVariablesFromServer(response.json); // The only other thing we do is preload the cookies resolve(); }) @@ -136,6 +147,7 @@ function saveLogin(backend, info) { if (response.status == 200) { console.info("Registered login with cookie:", !!response.headers["Set-Cookie"]); initialized = true; + setVariablesFromServer(response.json); resolve(); } else { console.error("Error registering:", response.status, response.statusText, response.text); @@ -294,3 +306,7 @@ exports.OAuthHandler = class OAuthHandler { }); } }; + +exports.getAbTests = function () { + return abTests; +}; diff --git a/server/src/ab-tests.js b/server/src/ab-tests.js index ba3af2d563..e2c3329ddf 100644 --- a/server/src/ab-tests.js +++ b/server/src/ab-tests.js @@ -1,37 +1,54 @@ // Note: these get turned into Test objects: +let allTests = {}; + +/* Example of how this could be set (until we have real tests to serve as docs): */ +/* let allTests = { autoOpenSharePanel: { description: "Open the share panel immediately after shot creation", + // Any actions the user does will have this GA field set: gaField: "cd3", + // If the user creates a shot, and then someone else VIEWS that shot, then + // this field will be set in the viewers events: shotField: "cd4", - version: 2, + // If you make updates (like add an option) and increment this, then users + // who were previously in the control group may get put into a new group: + version: 1, + // Keep the user in control if they aren't in control in any of these tests: exclude: ["highlightButtonOnInstall", "myShotsDisplay"], + // These are the actual allowed A/B options (control is never specified): options: [ - {name: "autoopen", probability: 0.9} + // The name of the option, and its probabilty (e.g., 10% chance of getting + // into this group) + {name: "autoopen", probability: 0.1} ] }, highlightButtonOnInstall: { description: "Highlight the Page Shot button when Page Shot is installed", gaField: "cd5", - version: 2, + version: 1, exclude: ["autoOpenSharePanel", "myShotsDisplay"], options: [ - {name: "uitour", probability: 0.9} + {name: "uitour", probability: 0.1} ] }, myShotsDisplay: { description: "Show My Shots button/CTA differently", gaField: "cd6", - version: 2, + version: 1, exclude: ["highlightButtonOnInstall", "autoOpenSharePanel"], options: [ + // Note no one will end up in control in this example: {name: "intropopup", probability: 0.9}, {name: "blink", probability: 0.1} ] } }; +*/ -let deprecatedTests = ["exampleTest"]; +// Any test names listed here will get removed from the A/B tests. Tests should +// be moved here once we are uninterested in any future data from the test: +let deprecatedTests = []; class Test { constructor(options) { @@ -161,3 +178,19 @@ function getRandom() { } return Math.random(); } + +/** Given a test name and test value that were set when a Shot was created (not + set on the user), return the GA field that should be set for someone viewing + the shot (or null if nothing should be set) */ +exports.shotGaFieldForValue = function (testName, testValue) { + if (deprecatedTests.includes(testName)) { + // Silently ignore deprecated tests + return null; + } + let test = allTests[testName]; + if (! test) { + console.error("Test name", testName, "is not known"); + return null; + } + return test.shotField || null; +}; diff --git a/server/src/ga-activation.js b/server/src/ga-activation.js index 44735092c5..44ca3b8e3f 100644 --- a/server/src/ga-activation.js +++ b/server/src/ga-activation.js @@ -66,6 +66,7 @@ const gaJs = ` ${stubGaJs} return; } + window.abTests = __ABTESTS__; window.GoogleAnalyticsObject = "ga"; window.ga = window.ga || function () { (window.ga.q = window.ga.q || []).push(arguments); @@ -127,6 +128,12 @@ window.sendEvent = function (action, label, options) { options = label; label = undefined; } + options = options || {}; + if (window.abTests) { + for (var testName in window.abTests) { + options[window.abTests[testName].gaField] = window.abTests[testName].value; + } + } console.debug("sendEvent", event + "/" + action + (label ? "/" + label : "") || "none", options || "no-options"); ga("send", "event", event, action, label, options); }; @@ -134,7 +141,7 @@ window.sendEvent = function (action, label, options) { const idRegex = /^[a-zA-Z0-9_.,-]+$/; -exports.makeGaActivationString = function (gaId, userId, hashLocation) { +exports.makeGaActivationString = function (gaId, userId, abTests, hashLocation) { if (gaId === "") { // Don't enable ga if no id was provided return stubGaJs; @@ -151,5 +158,6 @@ exports.makeGaActivationString = function (gaId, userId, hashLocation) { } let script = gaJs.replace(/__GA_ID__/g, gaId).replace(/__USER_ID__/g, userId); script = script.replace(/__HASH_LOCATION__/g, hashLocation ? "true" : "false"); + script = script.replace(/__ABTESTS__/g, JSON.stringify(abTests)); return script; }; diff --git a/server/src/pages/shot/controller.js b/server/src/pages/shot/controller.js index 1ffd2f7728..ba60f2875e 100644 --- a/server/src/pages/shot/controller.js +++ b/server/src/pages/shot/controller.js @@ -3,6 +3,7 @@ const sendEvent = require("../../browser-send-event.js"); const page = require("./page").page; const { AbstractShot } = require("../../../shared/shot"); +const { shotGaFieldForValue } = require("../../ab-tests.js"); // This represents the model we are rendering: let model; @@ -17,7 +18,20 @@ exports.launch = function (data) { model.shot.expireTime = new Date(model.expireTime); model.shot.deleted = model.deleted; model.controller = exports; - + if (model.shot.abTests) { + for (let testName in model.shot.abTests) { + let testValue = model.shot.abTests[testName]; + if (testValue) { + let shotFieldName = shotGaFieldForValue(testName, testValue); + if (shotFieldName) { + window.abTests[testName + "_shot"] = { + gaField: shotFieldName, + value: testValue + }; + } + } + } + } if (firstSet) { document.addEventListener("helper-ready", function onHelperReady(e) { document.removeEventListener("helper-ready", onHelperReady, false); diff --git a/server/src/pages/shotindex/view.js b/server/src/pages/shotindex/view.js index f1c82d2367..be6bcd7945 100644 --- a/server/src/pages/shotindex/view.js +++ b/server/src/pages/shotindex/view.js @@ -121,7 +121,6 @@ class Body extends React.Component { onClickDelete(shot, event) { event.stopPropagation(); event.preventDefault(); - console.log("deleting", shot.title, event); sendEvent("start-delete", "my-shots", {useBeacon: true}); if (window.confirm(`Delete ${shot.title}?`)) { sendEvent("delete", "my-shots-popup-confirm", {useBeacon: true}); diff --git a/server/src/reactrender.js b/server/src/reactrender.js index ae872ccec4..b3c725780a 100644 --- a/server/src/reactrender.js +++ b/server/src/reactrender.js @@ -13,13 +13,15 @@ exports.render = function (req, res, page) { authenticated: !!req.deviceId, sentryPublicDSN: req.config.sentryPublicDSN, backend: req.backend, - gitRevision: getGitRevision() + gitRevision: getGitRevision(), + abTests: req.abTests }, jsonModel); serverModel = Object.assign({ authenticated: !!req.deviceId, sentryPublicDSN: req.config.sentryPublicDSN, staticLink: req.staticLink, - staticLinkWithHost: req.staticLinkWithHost + staticLinkWithHost: req.staticLinkWithHost, + abTests: req.abTests }, serverModel); if (req.query.data == "json") { if (req.query.pretty !== undefined) { diff --git a/server/src/server.js b/server/src/server.js index 2e810799da..486e83fbb5 100644 --- a/server/src/server.js +++ b/server/src/server.js @@ -84,6 +84,7 @@ const { captureRavenException, sendRavenMessage, addRavenRequestHandler, addRavenErrorHandler } = require("./ravenclient"); const { errorResponse, simpleResponse, jsResponse } = require("./responses"); const selfPackage = require("./package.json"); +const { b64EncodeJson, b64DecodeJson } = require("./b64"); const PROXY_HEADER_WHITELIST = { "content-type": true, @@ -224,6 +225,13 @@ app.use(function (req, res, next) { req.userAnalytics = req.userAnalytics.debug(); } } + let abTests = cookies.get("abtests", {signed: true}); + if (abTests) { + abTests = b64DecodeJson(abTests); + } else { + abTests = {}; + } + req.abTests = abTests; req.backend = `${req.protocol}://${req.headers.host}`; req.config = config; next(); @@ -264,7 +272,7 @@ function sendGaActivation(req, res, hashPage) { promise = Promise.resolve(""); } promise.then((userUuid) => { - let script = gaActivation.makeGaActivationString(config.gaId, userUuid, hashPage); + let script = gaActivation.makeGaActivationString(config.gaId, userUuid, req.abTests, hashPage); jsResponse(res, script); }).catch((e) => { errorResponse(res, "Error creating user UUID:", e); @@ -467,11 +475,17 @@ app.post("/api/register", function (req, res) { secret: vars.secret, nickname: vars.nickname || null, avatarurl: vars.avatarurl || null - }, canUpdate).then(function (ok) { - if (ok) { + }, canUpdate).then(function (userAbTests) { + if (userAbTests) { let cookies = new Cookies(req, res, {keys: dbschema.getKeygrip()}); cookies.set("user", vars.deviceId, {signed: true}); - simpleResponse(res, "Created", 200); + cookies.set("abtests", b64EncodeJson(userAbTests), {signed: true}); + let responseJson = { + ok: "User created", + sentryPublicDSN: config.sentryPublicDSN, + abTests: userAbTests + }; + simpleResponse(res, JSON.stringify(responseJson), 200); } else { sendRavenMessage(req, "Attempted to register existing user", { extra: { @@ -521,11 +535,13 @@ app.post("/api/login", function (req, res) { throw e; } } - checkLogin(vars.deviceId, vars.secret, deviceInfo.addonVersion).then((ok) => { - if (ok) { + checkLogin(vars.deviceId, vars.secret, deviceInfo.addonVersion).then((userAbTests) => { + if (userAbTests) { let cookies = new Cookies(req, res, {keys: dbschema.getKeygrip()}); cookies.set("user", vars.deviceId, {signed: true}); - simpleResponse(res, JSON.stringify({"ok": "User logged in", "sentryPublicDSN": config.sentryPublicDSN}), 200); + cookies.set("abtests", b64EncodeJson(userAbTests), {signed: true}); + let responseJson = {"ok": "User logged in", "sentryPublicDSN": config.sentryPublicDSN, abTests: userAbTests}; + simpleResponse(res, JSON.stringify(responseJson), 200); if (config.gaId) { let userAnalytics = ua(config.gaId, vars.deviceId, {strictCidFormat: false}); userAnalytics.event({ @@ -535,7 +551,7 @@ app.post("/api/login", function (req, res) { ni: true }).send(); } - } else if (ok === null) { + } else if (! userAbTests) { simpleResponse(res, '{"error": "No such user"}', 404); } else { sendRavenMessage(req, "Invalid login"); @@ -554,6 +570,8 @@ app.post("/api/unload", function (req, res) { // This erases the session cookie: cookies.set("user"); cookies.set("user.sig"); + cookies.set("abtests"); + cookies.set("abtests.sig"); simpleResponse(res, "Noted", 200); }); diff --git a/server/src/users.js b/server/src/users.js index c638f90cf8..a0e0760ec1 100644 --- a/server/src/users.js +++ b/server/src/users.js @@ -4,6 +4,7 @@ const errors = require("../shared/errors"); const { request } = require("./helpers"); const crypto = require("crypto"); const mozlog = require("mozlog")("users"); +const abTests = require("./ab-tests"); function hashMatches(hash, secret) { let parts = hash.split(/:/g); @@ -36,25 +37,31 @@ function createNonce() { exports.checkLogin = function (deviceId, secret, addonVersion) { return db.select( - `SELECT secret_hashed FROM devices WHERE id = $1`, + `SELECT secret_hashed, ab_tests FROM devices WHERE id = $1`, [deviceId] ).then((rows) => { if (! rows.length) { return null; } + let userAbTests = {}; + if (rows[0].ab_tests) { + userAbTests = JSON.parse(rows[0].ab_tests); + } + userAbTests = abTests.updateAbTests(userAbTests); if (hashMatches(rows[0].secret_hashed, secret)) { db.update( `UPDATE devices SET last_login = NOW(), session_count = session_count + 1, - last_addon_version = $1 - WHERE id = $2 + last_addon_version = $1, + ab_tests = $2 + WHERE id = $3 `, - [addonVersion, deviceId] + [addonVersion, JSON.stringify(userAbTests), deviceId] ).catch((error) => { mozlog.error("error-updating-devices-table", {err: error}); }); - return true; + return userAbTests; } else { return false; } @@ -71,8 +78,9 @@ exports.registerLogin = function (deviceId, data, canUpdate) { VALUES ($1, $2, $3, $4)`, [deviceId, createHash(data.secret), data.nickname || null, data.avatarurl || null] ).then((inserted) => { + let userAbTests = abTests.updateAbTests({}); if (inserted) { - return true; + return userAbTests; } if (canUpdate) { if (! data.secret) { @@ -84,7 +92,11 @@ exports.registerLogin = function (deviceId, data, canUpdate) { WHERE id = $4`, [secretHashed, data.nickname || null, data.avatarurl || null, deviceId] ).then((rowCount) => { - return !! rowCount; + if (rowCount) { + return userAbTests; + } else { + return false; + } }); } else { return false; diff --git a/shared/shot.js b/shared/shot.js index 625577d752..04cd03232b 100644 --- a/shared/shot.js +++ b/shared/shot.js @@ -237,6 +237,7 @@ class AbstractShot { this.fullScreenThumbnail = attrs.fullScreenThumbnail || null; this.isPublic = attrs.isPublic === undefined || attrs.isPublic === null ? null : !! attrs.isPublic; this.showPage = attrs.showPage || false; + this.abTests = attrs.abTests || null; this.resources = attrs.resources || {}; this._clips = {}; if (attrs.clips) { @@ -808,13 +809,30 @@ ${options.addBody || ""} this._dirty("resources"); } + get abTests() { + return this._abTests; + } + set abTests(val) { + if (val === null || val === undefined) { + this._abTests = null; + this._dirty("abTests"); + return; + } + assert(typeof val == "object", "abTests should be an object, not:", typeof val); + assert(! Array.isArray(val), "abTests should not be an Array"); + for (let name in val) { + assert(val[name] && typeof val[name] == "string", `abTests.${name} should be a string:`, typeof val[name]); + } + this._abTests = val; + } + } AbstractShot.prototype.REGULAR_ATTRS = (` deviceId url docTitle ogTitle userTitle createdDate createdDevice favicon comments hashtags images readable head body htmlAttrs bodyAttrs headAttrs siteName openGraph twitterCard documentSize -fullScreenThumbnail isPublic resources showPage +fullScreenThumbnail isPublic resources showPage abTests `).split(/\s+/g); // Attributes that will be accepted in the constructor, but ignored/dropped