From bee4c95c285fa913a7913db56d729ad58d5a6549 Mon Sep 17 00:00:00 2001 From: Alexandre Bourdin Date: Thu, 18 Apr 2024 15:54:16 +0200 Subject: [PATCH] Merge pull request #17907 from overleaf/ab-split-test-assignments-optim-pt1 [web] Read anonymous split test assignments in session from both old&new fields GitOrigin-RevId: 5235bb3e7d72d5ff9e89c6543b70fb80e9f1213c --- .../BetaProgram/BetaProgramController.js | 6 +- .../src/Features/Project/ProjectController.js | 5 +- .../Features/Project/ProjectListController.js | 3 +- .../Features/SplitTests/SplitTestHandler.js | 241 +++++------------ .../SplitTests/SplitTestSessionHandler.js | 250 ++++++++++++++++++ .../SplitTests/SplitTestUserGetter.js | 30 +++ .../BetaProgram/BetaProgramControllerTests.js | 8 +- .../src/Project/ProjectControllerTests.js | 7 +- .../src/Project/ProjectListControllerTests.js | 9 +- .../src/SplitTests/SplitTestHandlerTests.js | 25 +- .../SplitTestSessionHandlerTests.js | 241 +++++++++++++++++ 11 files changed, 621 insertions(+), 204 deletions(-) create mode 100644 services/web/app/src/Features/SplitTests/SplitTestSessionHandler.js create mode 100644 services/web/app/src/Features/SplitTests/SplitTestUserGetter.js create mode 100644 services/web/test/unit/src/SplitTests/SplitTestSessionHandlerTests.js diff --git a/services/web/app/src/Features/BetaProgram/BetaProgramController.js b/services/web/app/src/Features/BetaProgram/BetaProgramController.js index 67dccc3be2..d60ca93b6c 100644 --- a/services/web/app/src/Features/BetaProgram/BetaProgramController.js +++ b/services/web/app/src/Features/BetaProgram/BetaProgramController.js @@ -3,14 +3,14 @@ const OError = require('@overleaf/o-error') const UserGetter = require('../User/UserGetter') const logger = require('@overleaf/logger') const SessionManager = require('../Authentication/SessionManager') -const SplitTestHandler = require('../SplitTests/SplitTestHandler') +const SplitTestSessionHandler = require('../SplitTests/SplitTestSessionHandler') const { expressify } = require('@overleaf/promise-utils') async function optIn(req, res) { const userId = SessionManager.getLoggedInUserId(req.session) await BetaProgramHandler.promises.optIn(userId) try { - await SplitTestHandler.promises.sessionMaintenance(req, null) + await SplitTestSessionHandler.promises.sessionMaintenance(req, null) } catch (error) { logger.error( { err: error }, @@ -24,7 +24,7 @@ async function optOut(req, res) { const userId = SessionManager.getLoggedInUserId(req.session) await BetaProgramHandler.promises.optOut(userId) try { - await SplitTestHandler.promises.sessionMaintenance(req, null) + await SplitTestSessionHandler.promises.sessionMaintenance(req, null) } catch (error) { logger.error( { err: error }, diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index 7e868d6690..3f4ccdfb7a 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -31,6 +31,7 @@ const BrandVariationsHandler = require('../BrandVariations/BrandVariationsHandle const UserController = require('../User/UserController') const AnalyticsManager = require('../Analytics/AnalyticsManager') const SplitTestHandler = require('../SplitTests/SplitTestHandler') +const SplitTestSessionHandler = require('../SplitTests/SplitTestSessionHandler') const FeaturesUpdater = require('../Subscription/FeaturesUpdater') const SpellingHandler = require('../Spelling/SpellingHandler') const { hasAdminAccess } = require('../Helpers/AdminAuthorizationHelper') @@ -431,7 +432,7 @@ const ProjectController = { }, user(cb) { if (userId == null) { - SplitTestHandler.sessionMaintenance(req, null, () => {}) + SplitTestSessionHandler.sessionMaintenance(req, null, () => {}) cb(null, defaultSettingsForAnonymousUser(userId)) } else { User.updateOne( @@ -453,7 +454,7 @@ const ProjectController = { return cb(err) } logger.debug({ projectId, userId }, 'got user') - SplitTestHandler.sessionMaintenance(req, user, () => {}) + SplitTestSessionHandler.sessionMaintenance(req, user, () => {}) if (FeaturesUpdater.featuresEpochIsCurrent(user)) { return cb(null, user) } diff --git a/services/web/app/src/Features/Project/ProjectListController.js b/services/web/app/src/Features/Project/ProjectListController.js index 36a5728c22..8aa59c6e82 100644 --- a/services/web/app/src/Features/Project/ProjectListController.js +++ b/services/web/app/src/Features/Project/ProjectListController.js @@ -24,6 +24,7 @@ const LimitationsManager = require('../Subscription/LimitationsManager') const NotificationsBuilder = require('../Notifications/NotificationsBuilder') const GeoIpLookup = require('../../infrastructure/GeoIpLookup') const SplitTestHandler = require('../SplitTests/SplitTestHandler') +const SplitTestSessionHandler = require('../SplitTests/SplitTestSessionHandler') const SubscriptionLocator = require('../Subscription/SubscriptionLocator') /** @typedef {import("./types").GetProjectsRequest} GetProjectsRequest */ @@ -115,7 +116,7 @@ async function projectListPage(req, res, next) { } if (isSaas) { - await SplitTestHandler.promises.sessionMaintenance(req, user) + await SplitTestSessionHandler.promises.sessionMaintenance(req, user) try { usersBestSubscription = diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index f77f4cfbbe..fbc5bbc1f8 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -1,5 +1,4 @@ const Metrics = require('@overleaf/metrics') -const UserGetter = require('../User/UserGetter') const UserUpdater = require('../User/UserUpdater') const AnalyticsManager = require('../Analytics/AnalyticsManager') const LocalsHelper = require('./LocalsHelper') @@ -14,11 +13,12 @@ const SplitTestUtils = require('./SplitTestUtils') const Settings = require('@overleaf/settings') const SessionManager = require('../Authentication/SessionManager') const logger = require('@overleaf/logger') +const SplitTestSessionHandler = require('./SplitTestSessionHandler') +const SplitTestUserGetter = require('./SplitTestUserGetter') const DEFAULT_VARIANT = 'default' const ALPHA_PHASE = 'alpha' const BETA_PHASE = 'beta' -const CACHE_TOMBSTONE_SPLIT_TEST_NOT_ACTIVE_FOR_USER = null const DEFAULT_ASSIGNMENT = { variant: DEFAULT_VARIANT, analytics: { @@ -85,7 +85,7 @@ async function getAssignment(req, res, splitTestName, { sync = false } = {}) { session: req.session, sync, }) - _collectSessionStats(req.session) + SplitTestSessionHandler.collectSessionStats(req.session) } } } catch (error) { @@ -175,7 +175,7 @@ async function getActiveAssignmentsForUser(userId, removeArchived = false) { return {} } - const user = await _getUser(userId) + const user = await SplitTestUserGetter.promises.getUser(userId) if (user == null) { return {} } @@ -209,34 +209,6 @@ async function getActiveAssignmentsForUser(userId, removeArchived = false) { return assignments } -/** - * @param {import('express').Request} req - * @param {Object|null} user optional, prefetched user with alphaProgram and betaProgram field - * @return {Promise} - */ -async function sessionMaintenance(req, user) { - const session = req.session - const sessionUser = SessionManager.getSessionUser(session) - - Metrics.inc('split_test_session_maintenance', 1, { status: 'start' }) - if (sessionUser) { - user = user || (await _getUser(sessionUser._id)) - if ( - Boolean(sessionUser.alphaProgram) !== Boolean(user.alphaProgram) || - Boolean(sessionUser.betaProgram) !== Boolean(user.betaProgram) - ) { - Metrics.inc('split_test_session_maintenance', 1, { - status: 'program-change', - }) - sessionUser.alphaProgram = user.alphaProgram || undefined // only store if set - sessionUser.betaProgram = user.betaProgram || undefined // only store if set - session.cachedSplitTestAssignments = {} - } - } - - // TODO: After changing the split test config fetching: remove split test assignments for archived split tests -} - /** * Returns an array of valid variant names for the given split test, including default * @@ -285,18 +257,22 @@ async function _getAssignment( } if (canUseSessionCache) { - const cachedVariant = _getCachedVariantFromSession( + const cachedVariant = SplitTestSessionHandler.getCachedVariant( session, splitTest.name, currentVersion ) - if (cachedVariant === CACHE_TOMBSTONE_SPLIT_TEST_NOT_ACTIVE_FOR_USER) { - Metrics.inc('split_test_get_assignment_source', 1, { status: 'cache' }) - return DEFAULT_ASSIGNMENT - } + if (cachedVariant) { Metrics.inc('split_test_get_assignment_source', 1, { status: 'cache' }) - return _makeAssignment(splitTest, cachedVariant, currentVersion) + if ( + cachedVariant === + SplitTestSessionHandler.CACHE_TOMBSTONE_SPLIT_TEST_NOT_ACTIVE_FOR_USER + ) { + return DEFAULT_ASSIGNMENT + } else { + return _makeAssignment(splitTest, cachedVariant, currentVersion) + } } } @@ -308,11 +284,14 @@ async function _getAssignment( Metrics.inc('split_test_get_assignment_source', 1, { status: 'none' }) } - user = user || (userId && (await _getUser(userId, splitTestName))) + user = + user || + (userId && + (await SplitTestUserGetter.promises.getUser(userId, splitTestName))) const { activeForUser, selectedVariantName, phase, versionNumber } = await _getAssignmentMetadata(analyticsId, user, splitTest) if (canUseSessionCache) { - _setVariantInSession({ + SplitTestSessionHandler.setVariantInCache({ session, splitTestName, currentVersion, @@ -321,22 +300,40 @@ async function _getAssignment( }) } if (activeForUser) { - const assignmentConfig = { - user, - userId, - analyticsId, - session, - splitTestName, - variantName: selectedVariantName, - phase, - versionNumber, - } if (currentVersion.analyticsEnabled) { - if (sync === true) { - await _updateVariantAssignment(assignmentConfig) - } else { - _updateVariantAssignment(assignmentConfig) + // if the user is logged in, persist the assignment + if (userId) { + const assignmentData = { + user, + userId, + splitTestName, + phase, + versionNumber, + variantName: selectedVariantName, + } + if (sync === true) { + await _recordAssignment(assignmentData) + } else { + _recordAssignment(assignmentData) + } } + // otherwise this is an anonymous user, we store assignments in session to persist them on registration + else { + await SplitTestSessionHandler.promises.appendAssignment(session, { + splitTestId: splitTest._id, + splitTestName, + phase, + versionNumber, + variantName: selectedVariantName, + assignedAt: new Date(), + }) + } + + AnalyticsManager.setUserPropertyForAnalyticsId( + user?.analyticsId || analyticsId || userId, + `split-test-${splitTestName}-${versionNumber}`, + selectedVariantName + ) } return _makeAssignment(splitTest, selectedVariantName, currentVersion) } @@ -404,11 +401,9 @@ function _getVariantFromPercentile(variants, percentile) { } } -async function _updateVariantAssignment({ +async function _recordAssignment({ user, userId, - analyticsId, - session, splitTestName, phase, versionNumber, @@ -420,45 +415,18 @@ async function _updateVariantAssignment({ phase, assignedAt: new Date(), } - // if the user is logged in - if (userId) { - user = user || (await _getUser(userId, splitTestName)) - if (user) { - const assignedSplitTests = user.splitTests || [] - const assignmentLog = assignedSplitTests[splitTestName] || [] - const existingAssignment = _.find(assignmentLog, { versionNumber }) - if (!existingAssignment) { - await UserUpdater.promises.updateUser(userId, { - $addToSet: { - [`splitTests.${splitTestName}`]: persistedAssignment, - }, - }) - AnalyticsManager.setUserPropertyForAnalyticsId( - user.analyticsId || analyticsId || userId, - `split-test-${splitTestName}-${versionNumber}`, - variantName - ) - } - } - } - // otherwise this is an anonymous user, we store assignments in session to persist them on registration - else if (session) { - if (!session.splitTests) { - session.splitTests = {} - } - if (!session.splitTests[splitTestName]) { - session.splitTests[splitTestName] = [] - } - const existingAssignment = _.find(session.splitTests[splitTestName], { - versionNumber, - }) + user = + user || (await SplitTestUserGetter.promises.getUser(userId, splitTestName)) + if (user) { + const assignedSplitTests = user.splitTests || [] + const assignmentLog = assignedSplitTests[splitTestName] || [] + const existingAssignment = _.find(assignmentLog, { versionNumber }) if (!existingAssignment) { - session.splitTests[splitTestName].push(persistedAssignment) - AnalyticsManager.setUserPropertyForAnalyticsId( - analyticsId, - `split-test-${splitTestName}-${versionNumber}`, - variantName - ) + await UserUpdater.promises.updateUser(userId, { + $addToSet: { + [`splitTests.${splitTestName}`]: persistedAssignment, + }, + }) } } } @@ -479,63 +447,6 @@ function _makeAssignment(splitTest, variant, currentVersion) { } } -function _getCachedVariantFromSession(session, splitTestName, currentVersion) { - if (!session.cachedSplitTestAssignments) { - session.cachedSplitTestAssignments = {} - } - const cacheKey = `${splitTestName}-${currentVersion.versionNumber}` - return session.cachedSplitTestAssignments[cacheKey] -} - -function _setVariantInSession({ - session, - splitTestName, - currentVersion, - selectedVariantName, - activeForUser, -}) { - if (!session.cachedSplitTestAssignments) { - session.cachedSplitTestAssignments = {} - } - - // clean up previous entries from this split test - for (const cacheKey of Object.keys(session.cachedSplitTestAssignments)) { - // drop '-versionNumber' - const name = cacheKey.split('-').slice(0, -1).join('-') - if (name === splitTestName) { - delete session.cachedSplitTestAssignments[cacheKey] - } - } - - const cacheKey = `${splitTestName}-${currentVersion.versionNumber}` - if (activeForUser) { - session.cachedSplitTestAssignments[cacheKey] = selectedVariantName - } else { - session.cachedSplitTestAssignments[cacheKey] = - CACHE_TOMBSTONE_SPLIT_TEST_NOT_ACTIVE_FOR_USER - } -} - -async function _getUser(id, splitTestName) { - const projection = { - analyticsId: 1, - alphaProgram: 1, - betaProgram: 1, - } - if (splitTestName) { - projection[`splitTests.${splitTestName}`] = 1 - } else { - projection.splitTests = 1 - } - const user = await UserGetter.promises.getUser(id, projection) - Metrics.histogram( - 'split_test_get_user_from_mongo_size', - JSON.stringify(user).length, - [0, 100, 500, 1000, 2000, 5000, 10000, 15000, 20000, 50000, 100000] - ) - return user -} - async function _loadSplitTestInfoInLocals(locals, splitTestName, session) { const splitTest = await _getSplitTest(splitTestName) if (splitTest) { @@ -579,41 +490,16 @@ function _getNonSaasAssignment(splitTestName) { return DEFAULT_ASSIGNMENT } -function _collectSessionStats(session) { - if (session.cachedSplitTestAssignments) { - Metrics.summary( - 'split_test_session_cache_count', - Object.keys(session.cachedSplitTestAssignments).length - ) - Metrics.summary( - 'split_test_session_cache_size', - JSON.stringify(session.cachedSplitTestAssignments).length - ) - } - if (session.splitTests) { - Metrics.summary( - 'split_test_session_storage_count', - Object.keys(session.splitTests).length - ) - Metrics.summary( - 'split_test_session_storage_size', - JSON.stringify(session.splitTests).length - ) - } -} - async function _getSplitTest(name) { const splitTests = await SplitTestCache.get('') return splitTests?.get(name) } module.exports = { - getPercentile, getAssignment: callbackify(getAssignment), getAssignmentForMongoUser: callbackify(getAssignmentForMongoUser), getAssignmentForUser: callbackify(getAssignmentForUser), getActiveAssignmentsForUser: callbackify(getActiveAssignmentsForUser), - sessionMaintenance: callbackify(sessionMaintenance), setOverrideInSession, clearOverridesInSession, promises: { @@ -621,6 +507,5 @@ module.exports = { getAssignmentForMongoUser, getAssignmentForUser, getActiveAssignmentsForUser, - sessionMaintenance, }, } diff --git a/services/web/app/src/Features/SplitTests/SplitTestSessionHandler.js b/services/web/app/src/Features/SplitTests/SplitTestSessionHandler.js new file mode 100644 index 0000000000..82995c745e --- /dev/null +++ b/services/web/app/src/Features/SplitTests/SplitTestSessionHandler.js @@ -0,0 +1,250 @@ +const { callbackify } = require('util') +const _ = require('lodash') +const { ObjectId } = require('mongodb') +const logger = require('@overleaf/logger') +const Metrics = require('@overleaf/metrics') +const SessionManager = require('../Authentication/SessionManager') +const SplitTestCache = require('./SplitTestCache') +const SplitTestUtils = require('./SplitTestUtils') +const SplitTestUserGetter = require('./SplitTestUserGetter') + +const CACHE_TOMBSTONE_SPLIT_TEST_NOT_ACTIVE_FOR_USER = null + +async function getAssignments(session) { + if (!session.splitTests && !session.sta) { + return undefined + } + + // await _convertAnonymousAssignmentsIfNeeded(session) + const assignments = _.clone(session.splitTests || {}) + if (session.sta) { + const tokens = session.sta.split(';') + const splitTests = Array.from((await SplitTestCache.get('')).values()) + for (const token of tokens) { + try { + if (!token.length) { + continue + } + const [splitTestNameVersion, info] = token.split('=') + const [splitTestId64, versionStr] = splitTestNameVersion.split('_') + + const splitTest = splitTests.find( + test => + test._id.toString() === + new ObjectId(Buffer.from(splitTestId64, 'base64')).toString() + ) + if (!splitTest) { + continue + } + + const splitTestName = splitTest.name + const versionNumber = parseInt(versionStr) + const [variantChar, timestampStr36] = info.split(':') + const assignedAt = new Date(parseInt(timestampStr36, 36) * 1000) + let variantName + if (variantChar === 'd') { + variantName = 'default' + } else { + const variantIndex = parseInt(variantChar) + variantName = + SplitTestUtils.getCurrentVersion(splitTest).variants[variantIndex] + .name + } + + if (!assignments[splitTestName]) { + assignments[splitTestName] = [] + } + assignments[splitTestName].push({ + versionNumber, + variantName, + phase: 'release', // anonymous users can only be exposed to tests in release phase + assignedAt, + }) + } catch (error) { + logger.error( + { err: error, token }, + 'Failed to resolve anonymous split test assignment from session' + ) + } + } + } + + return assignments +} + +async function appendAssignment(session, assignment) { + // await _convertAnonymousAssignmentsIfNeeded(session) + + if (!session.splitTests) { + session.splitTests = {} + } + if (!session.splitTests[assignment.splitTestName]) { + session.splitTests[assignment.splitTestName] = [] + } + + const assignments = await getAssignments(session) + if ( + _.find(assignments[assignment.splitTestName], { + splitTestName: assignment.splitTestName, + versionNumber: assignment.versionNumber, + }) + ) { + // if (!session.sta) { + // session.sta = '' + // } + // const splitTests = await SplitTestCache.get('') + // const splitTest = splitTests.get(assignment.splitTestName) + // const assignmentString = _buildAssignmentString(splitTest, assignment) + // const separator = session.sta.length > 0 ? ';' : '' + // session.sta += `${separator}${assignmentString}` + session.splitTests[assignment.splitTestName].push(assignment) + } +} + +function getCachedVariant(session, splitTestName, currentVersion) { + if (!session.cachedSplitTestAssignments) { + session.cachedSplitTestAssignments = {} + } + const cacheKey = `${splitTestName}-${currentVersion.versionNumber}` + return session.cachedSplitTestAssignments[cacheKey] +} + +function setVariantInCache({ + session, + splitTestName, + currentVersion, + selectedVariantName, + activeForUser, +}) { + if (!session.cachedSplitTestAssignments) { + session.cachedSplitTestAssignments = {} + } + + // clean up previous entries from this split test + for (const cacheKey of Object.keys(session.cachedSplitTestAssignments)) { + // drop '-versionNumber' + const name = cacheKey.split('-').slice(0, -1).join('-') + if (name === splitTestName) { + delete session.cachedSplitTestAssignments[cacheKey] + } + } + + const cacheKey = `${splitTestName}-${currentVersion.versionNumber}` + if (activeForUser) { + session.cachedSplitTestAssignments[cacheKey] = selectedVariantName + } else { + session.cachedSplitTestAssignments[cacheKey] = + CACHE_TOMBSTONE_SPLIT_TEST_NOT_ACTIVE_FOR_USER + } +} + +/** + * @param {import('express').Request} req + * @param {Object|null} user optional, prefetched user with alphaProgram and betaProgram field + * @return {Promise} + */ +async function sessionMaintenance(req, user) { + const session = req.session + const sessionUser = SessionManager.getSessionUser(session) + + Metrics.inc('split_test_session_maintenance', 1, { status: 'start' }) + if (sessionUser) { + user = user || (await SplitTestUserGetter.promises.getUser(sessionUser._id)) + if ( + Boolean(sessionUser.alphaProgram) !== Boolean(user.alphaProgram) || + Boolean(sessionUser.betaProgram) !== Boolean(user.betaProgram) + ) { + Metrics.inc('split_test_session_maintenance', 1, { + status: 'program-change', + }) + sessionUser.alphaProgram = user.alphaProgram || undefined // only store if set + sessionUser.betaProgram = user.betaProgram || undefined // only store if set + session.cachedSplitTestAssignments = {} + } + } + + // TODO: After changing the split test config fetching: remove split test assignments for archived split tests +} + +function collectSessionStats(session) { + if (session.cachedSplitTestAssignments) { + Metrics.summary( + 'split_test_session_cache_count', + Object.keys(session.cachedSplitTestAssignments).length + ) + Metrics.summary( + 'split_test_session_cache_size', + JSON.stringify(session.cachedSplitTestAssignments).length + ) + } + if (session.splitTests) { + Metrics.summary( + 'split_test_session_storage_count', + (session.sta || '').split(';').length + + Object.keys(session.splitTests).length + ) + Metrics.summary( + 'split_test_session_storage_size', + (session.sta || '').length + JSON.stringify(session.splitTests).length + ) + } +} + +// async function _convertAnonymousAssignmentsIfNeeded(session) { +// if (typeof session.splitTests === 'object') { +// const sessionAssignments = session.splitTests +// const splitTests = await SplitTestCache.get('') +// session.splitTests = '' +// for (const [splitTestName, assignments] of Object.entries( +// sessionAssignments +// )) { +// const splitTest = splitTests.get(splitTestName) +// for (const assignment of assignments) { +// const assignmentString = _buildAssignmentString(splitTest, assignment) +// const separator = session.splitTests.length > 0 ? ';' : '' +// session.splitTests += `${separator}${assignmentString}` +// } +// } +// } +// } + +// function _hasExistingAssignment(session, splitTest, versionNumber) { +// if (!session.sta) { +// return false +// } +// const index = session.sta.indexOf( +// `${_convertIdToBase64(splitTest._id)}_${versionNumber}=` +// ) +// return index >= 0 +// } + +// function _buildAssignmentString(splitTest, assignment) { +// const { versionNumber, variantName, assignedAt } = assignment +// const variants = SplitTestUtils.getCurrentVersion(splitTest).variants +// const splitTestId = _convertIdToBase64(splitTest._id) +// const variantChar = +// variantName === 'default' +// ? 'd' +// : _.findIndex(variants, { name: variantName }) +// const timestamp = Math.floor(assignedAt.getTime() / 1000).toString(36) +// return `${splitTestId}_${versionNumber}=${variantChar}:${timestamp}` +// } + +// function _convertIdToBase64(id) { +// return new ObjectId(id).toString('base64') +// } + +module.exports = { + getAssignments: callbackify(getAssignments), + appendAssignment: callbackify(appendAssignment), + getCachedVariant, + setVariantInCache, + sessionMaintenance: callbackify(sessionMaintenance), + collectSessionStats, + CACHE_TOMBSTONE_SPLIT_TEST_NOT_ACTIVE_FOR_USER, + promises: { + getAssignments, + appendAssignment, + sessionMaintenance, + }, +} diff --git a/services/web/app/src/Features/SplitTests/SplitTestUserGetter.js b/services/web/app/src/Features/SplitTests/SplitTestUserGetter.js new file mode 100644 index 0000000000..cf6503c51c --- /dev/null +++ b/services/web/app/src/Features/SplitTests/SplitTestUserGetter.js @@ -0,0 +1,30 @@ +const { callbackify } = require('util') +const Metrics = require('@overleaf/metrics') +const UserGetter = require('../User/UserGetter') + +async function getUser(id, splitTestName) { + const projection = { + analyticsId: 1, + alphaProgram: 1, + betaProgram: 1, + } + if (splitTestName) { + projection[`splitTests.${splitTestName}`] = 1 + } else { + projection.splitTests = 1 + } + const user = await UserGetter.promises.getUser(id, projection) + Metrics.histogram( + 'split_test_get_user_from_mongo_size', + JSON.stringify(user).length, + [0, 100, 500, 1000, 2000, 5000, 10000, 15000, 20000, 50000, 100000] + ) + return user +} + +module.exports = { + getUser: callbackify(getUser), + promises: { + getUser, + }, +} diff --git a/services/web/test/unit/src/BetaProgram/BetaProgramControllerTests.js b/services/web/test/unit/src/BetaProgram/BetaProgramControllerTests.js index 8a27f484c0..83dbd28b4b 100644 --- a/services/web/test/unit/src/BetaProgram/BetaProgramControllerTests.js +++ b/services/web/test/unit/src/BetaProgram/BetaProgramControllerTests.js @@ -23,14 +23,14 @@ describe('BetaProgramController', function () { user: this.user, }, } - this.SplitTestHandler = { + this.SplitTestSessionHandler = { promises: { sessionMaintenance: sinon.stub(), }, } this.BetaProgramController = SandboxedModule.require(modulePath, { requires: { - '../SplitTests/SplitTestHandler': this.SplitTestHandler, + '../SplitTests/SplitTestSessionHandler': this.SplitTestSessionHandler, './BetaProgramHandler': (this.BetaProgramHandler = { promises: { optIn: sinon.stub().resolves(), @@ -76,7 +76,7 @@ describe('BetaProgramController', function () { it('should invoke the session maintenance', function (done) { this.res.callback = () => { - this.SplitTestHandler.promises.sessionMaintenance.should.have.been.calledWith( + this.SplitTestSessionHandler.promises.sessionMaintenance.should.have.been.calledWith( this.req ) done() @@ -130,7 +130,7 @@ describe('BetaProgramController', function () { it('should invoke the session maintenance', function (done) { this.res.callback = () => { - this.SplitTestHandler.promises.sessionMaintenance.should.have.been.calledWith( + this.SplitTestSessionHandler.promises.sessionMaintenance.should.have.been.calledWith( this.req, null ) diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index 99f3ac61b9..eba513384d 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -136,6 +136,8 @@ describe('ProjectController', function () { getAssignment: sinon.stub().resolves({ variant: 'default' }), }, getAssignment: sinon.stub().yields(null, { variant: 'default' }), + } + this.SplitTestSessionHandler = { sessionMaintenance: sinon.stub().yields(), } this.InstitutionsFeatures = { @@ -160,6 +162,7 @@ describe('ProjectController', function () { '@overleaf/settings': this.settings, '@overleaf/metrics': this.Metrics, '../SplitTests/SplitTestHandler': this.SplitTestHandler, + '../SplitTests/SplitTestSessionHandler': this.SplitTestSessionHandler, './ProjectDeleter': this.ProjectDeleter, './ProjectDuplicator': this.ProjectDuplicator, './ProjectCreationHandler': this.ProjectCreationHandler, @@ -536,7 +539,7 @@ describe('ProjectController', function () { it('should invoke the session maintenance for logged in user', function (done) { this.res.render = () => { - this.SplitTestHandler.sessionMaintenance.should.have.been.calledWith( + this.SplitTestSessionHandler.sessionMaintenance.should.have.been.calledWith( this.req, this.user ) @@ -548,7 +551,7 @@ describe('ProjectController', function () { it('should invoke the session maintenance for anonymous user', function (done) { this.SessionManager.getLoggedInUserId.returns(null) this.res.render = () => { - this.SplitTestHandler.sessionMaintenance.should.have.been.calledWith( + this.SplitTestSessionHandler.sessionMaintenance.should.have.been.calledWith( this.req ) done() diff --git a/services/web/test/unit/src/Project/ProjectListControllerTests.js b/services/web/test/unit/src/Project/ProjectListControllerTests.js index 3b5ffd87a0..51aef2552e 100644 --- a/services/web/test/unit/src/Project/ProjectListControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectListControllerTests.js @@ -102,10 +102,14 @@ describe('ProjectListController', function () { } this.SplitTestHandler = { promises: { - sessionMaintenance: sinon.stub().resolves(), getAssignment: sinon.stub().resolves({ variant: 'default' }), }, } + this.SplitTestSessionHandler = { + promises: { + sessionMaintenance: sinon.stub().resolves(), + }, + } this.SubscriptionViewModelBuilder = { promises: { getBestSubscription: sinon.stub().resolves({ type: 'free' }), @@ -141,6 +145,7 @@ describe('ProjectListController', function () { '@overleaf/settings': this.settings, '@overleaf/metrics': this.Metrics, '../SplitTests/SplitTestHandler': this.SplitTestHandler, + '../SplitTests/SplitTestSessionHandler': this.SplitTestSessionHandler, '../User/UserController': this.UserController, './ProjectHelper': this.ProjectHelper, '../Subscription/LimitationsManager': this.LimitationsManager, @@ -223,7 +228,7 @@ describe('ProjectListController', function () { it('should invoke the session maintenance', function (done) { this.Features.hasFeature.withArgs('saas').returns(true) this.res.render = () => { - this.SplitTestHandler.promises.sessionMaintenance.should.have.been.calledWith( + this.SplitTestSessionHandler.promises.sessionMaintenance.should.have.been.calledWith( this.req, this.user ) diff --git a/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js b/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js index 7b147f8597..cdf3bf5d82 100644 --- a/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js +++ b/services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js @@ -27,12 +27,6 @@ describe('SplitTestHandler', function () { this.cachedSplitTests.set(splitTest.name, splitTest) } - this.UserGetter = { - promises: { - getUser: sinon.stub().resolves(null), - }, - } - this.SplitTest = { find: sinon.stub().returns({ exec: sinon.stub().resolves(this.splitTests), @@ -54,11 +48,20 @@ describe('SplitTestHandler', function () { } this.AnalyticsManager = { getIdsFromSession: sinon.stub(), + setUserPropertyForAnalyticsId: sinon.stub(), } this.LocalsHelper = { setSplitTestVariant: sinon.stub(), setSplitTestInfo: sinon.stub(), } + this.SplitTestSessionHandler = { + collectSessionStats: sinon.stub(), + } + this.SplitTestUserGetter = { + promises: { + getUser: sinon.stub().resolves(null), + }, + } this.SplitTestHandler = SandboxedModule.require(MODULE_PATH, { requires: { @@ -68,6 +71,8 @@ describe('SplitTestHandler', function () { '../User/UserUpdater': {}, '../Analytics/AnalyticsManager': this.AnalyticsManager, './LocalsHelper': this.LocalsHelper, + './SplitTestSessionHandler': this.SplitTestSessionHandler, + './SplitTestUserGetter': this.SplitTestUserGetter, '@overleaf/settings': this.Settings, }, }) @@ -100,9 +105,7 @@ describe('SplitTestHandler', function () { ], }, } - this.UserGetter.promises.getUser - .withArgs(this.user._id) - .resolves(this.user) + this.SplitTestUserGetter.promises.getUser.resolves(this.user) this.assignments = await this.SplitTestHandler.promises.getActiveAssignmentsForUser( this.user._id @@ -164,9 +167,7 @@ describe('SplitTestHandler', function () { describe('with a user without assignments', function () { beforeEach(async function () { this.user = { _id: new ObjectId() } - this.UserGetter.promises.getUser - .withArgs(this.user._id) - .resolves(this.user) + this.SplitTestUserGetter.promises.getUser.resolves(this.user) this.assignments = await this.SplitTestHandler.promises.getActiveAssignmentsForUser( this.user._id diff --git a/services/web/test/unit/src/SplitTests/SplitTestSessionHandlerTests.js b/services/web/test/unit/src/SplitTests/SplitTestSessionHandlerTests.js new file mode 100644 index 0000000000..42c29d1ce8 --- /dev/null +++ b/services/web/test/unit/src/SplitTests/SplitTestSessionHandlerTests.js @@ -0,0 +1,241 @@ +const Path = require('path') +const SandboxedModule = require('sandboxed-module') +const sinon = require('sinon') +const { expect } = require('chai') + +const MODULE_PATH = Path.join( + __dirname, + '../../../../app/src/Features/SplitTests/SplitTestSessionHandler' +) + +describe('SplitTestSessionHandler', function () { + beforeEach(function () { + this.SplitTestCache = { + get: sinon.stub().resolves(), + } + this.SplitTestUserGetter = {} + this.Metrics = {} + this.SplitTestSessionHandler = SandboxedModule.require(MODULE_PATH, { + requires: { + './SplitTestCache': this.SplitTestCache, + './SplitTestUserGetter': this.SplitTestUserGetter, + '@overleaf/metrics': this.Metrics, + }, + }) + }) + + it('should read from the splitTests field', async function () { + const session = { + splitTests: { + 'anon-test-1': [ + { + variantName: 'default', + versionNumber: 1, + phase: 'release', + assignedAt: new Date(1712872800000), // 2024-04-11 22:00:00 + }, + ], + 'anon-test-2': [ + { + variantName: 'default', + versionNumber: 1, + phase: 'release', + assignedAt: new Date(1712307600000), // 2024-04-05 09:00:00 + }, + { + variantName: 'v-2', + versionNumber: 2, + phase: 'release', + assignedAt: new Date(1712581200000), // 2024-04-08 13:00:00 + }, + ], + }, + sta: ``, + } + + const assignments = + await this.SplitTestSessionHandler.promises.getAssignments(session) + expect(assignments).to.deep.equal({ + 'anon-test-1': [ + { + variantName: 'default', + versionNumber: 1, + phase: 'release', + assignedAt: new Date(1712872800000), + }, + ], + 'anon-test-2': [ + { + variantName: 'default', + versionNumber: 1, + phase: 'release', + assignedAt: new Date(1712307600000), + }, + { + variantName: 'v-2', + versionNumber: 2, + phase: 'release', + assignedAt: new Date(1712581200000), + }, + ], + }) + }) + + it('should not read from the sta field', async function () { + this.SplitTestCache.get = sinon.stub().resolves( + new Map( + Object.entries({ + 'anon-test-1': { + _id: '661f92a4669764bb03f73e37', + name: 'anon-test-1', + versions: [ + { + versionNumber: 1, + variants: [ + { + name: 'enabled', + }, + ], + }, + ], + }, + 'anon-test-2': { + _id: '661f92a9d68ea711d6bf2df4', + name: 'anon-test-2', + versions: [ + { + versionNumber: 1, + variants: [ + { + name: 'v-1', + }, + { + name: 'v-2', + }, + ], + }, + ], + }, + }) + ) + ) + const session = { + sta: `Zh+SpGaXZLsD9z43_1=d:sbrvs0;Zh+SqdaOpxHWvy30_1=d:sbtqg0;Zh+SqdaOpxHWvy30_2=1:sbsi00`, + } + + const assignments = + await this.SplitTestSessionHandler.promises.getAssignments(session) + expect(assignments).to.deep.equal({ + 'anon-test-1': [ + { + variantName: 'default', + versionNumber: 1, + phase: 'release', + assignedAt: new Date(1712829600000), + }, + ], + 'anon-test-2': [ + { + variantName: 'default', + versionNumber: 1, + phase: 'release', + assignedAt: new Date(1712916000000), + }, + { + variantName: 'v-2', + versionNumber: 2, + phase: 'release', + assignedAt: new Date(1712858400000), + }, + ], + }) + }) + + it('should merge assignments from both splitTests and sta fields', async function () { + this.SplitTestCache.get = sinon.stub().resolves( + new Map( + Object.entries({ + 'anon-test-1': { + _id: '661f92a4669764bb03f73e37', + name: 'anon-test-1', + versions: [ + { + versionNumber: 1, + variants: [ + { + name: 'enabled', + }, + ], + }, + ], + }, + 'anon-test-2': { + _id: '661f92a9d68ea711d6bf2df4', + name: 'anon-test-2', + versions: [ + { + versionNumber: 1, + variants: [ + { + name: 'v-1', + }, + { + name: 'v-2', + }, + ], + }, + ], + }, + }) + ) + ) + const session = { + splitTests: { + 'anon-test-1': [ + { + variantName: 'default', + versionNumber: 1, + phase: 'release', + assignedAt: new Date(1712872800000), + }, + ], + 'anon-test-2': [ + { + variantName: 'default', + versionNumber: 1, + phase: 'release', + assignedAt: new Date(1712307600000), + }, + ], + }, + sta: `Zh+SqdaOpxHWvy30_2=1:sbsi00`, + } + + const assignments = + await this.SplitTestSessionHandler.promises.getAssignments(session) + expect(assignments).to.deep.equal({ + 'anon-test-1': [ + { + variantName: 'default', + versionNumber: 1, + phase: 'release', + assignedAt: new Date(1712872800000), + }, + ], + 'anon-test-2': [ + { + variantName: 'default', + versionNumber: 1, + phase: 'release', + assignedAt: new Date(1712307600000), + }, + { + variantName: 'v-2', + versionNumber: 2, + phase: 'release', + assignedAt: new Date(1712858400000), + }, + ], + }) + }) +})