diff --git a/services/web/app/src/Features/SplitTests/SplitTestCache.js b/services/web/app/src/Features/SplitTests/SplitTestCache.js index 4f81b451df..557306d70b 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestCache.js +++ b/services/web/app/src/Features/SplitTests/SplitTestCache.js @@ -12,7 +12,9 @@ class SplitTestCache extends CacheLoader { async load() { Metrics.inc('split_test_get_split_test_from_mongo', 1, {}) const splitTests = await SplitTestManager.getRuntimeTests() - return new Map(splitTests.map(splitTest => [splitTest.name, splitTest])) + return new Map( + splitTests.map(splitTest => [splitTest.name, splitTest.toObject()]) + ) } serialize(value) { diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index 1a5203f73a..fbc5bbc1f8 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -492,10 +492,7 @@ function _getNonSaasAssignment(splitTestName) { async function _getSplitTest(name) { const splitTests = await SplitTestCache.get('') - const splitTest = splitTests?.get(name) - if (splitTest && !splitTest.archived) { - return splitTest - } + return splitTests?.get(name) } module.exports = { diff --git a/services/web/app/src/Features/SplitTests/SplitTestManager.js b/services/web/app/src/Features/SplitTests/SplitTestManager.js index 579fe5991c..52b1cfdc2a 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestManager.js +++ b/services/web/app/src/Features/SplitTests/SplitTestManager.js @@ -60,7 +60,9 @@ async function getSplitTests({ name, phase, type, active, archived }) { async function getRuntimeTests() { try { - return SplitTest.find({}).lean().exec() + return await SplitTest.find({ + archived: { $ne: true }, + }).exec() } catch (error) { throw OError.tag(error, 'Failed to get active split tests list') } diff --git a/services/web/app/src/Features/SplitTests/SplitTestSessionHandler.js b/services/web/app/src/Features/SplitTests/SplitTestSessionHandler.js index 5dde68a1e7..e2aae8b35b 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestSessionHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestSessionHandler.js @@ -9,68 +9,63 @@ const SplitTestUtils = require('./SplitTestUtils') const SplitTestUserGetter = require('./SplitTestUserGetter') const CACHE_TOMBSTONE_SPLIT_TEST_NOT_ACTIVE_FOR_USER = null -const TOKEN_SEP = ';' -// this is safe to use as a separator adjacent to a base64 string because Mongo object IDs -// do not generate any padding when converted (24 hex digits = 12 bytes => multiple of 6), -// thus do not contain any trailing `=` -const KEY_VALUE_SEP = '=' -const ID_VERSION_SEP = '_' -const VARIANT_DATE_SEP = ':' async function getAssignments(session) { - await _convertAnonymousAssignmentsIfNeeded(session) - - if (!session.sta) { + if (!session.splitTests && !session.sta) { return undefined } - const assignments = {} - const tokens = session.sta.split(TOKEN_SEP) - const splitTests = Array.from((await SplitTestCache.get('')).values()) - for (const token of tokens) { - try { - if (!token.length) { - continue - } - const [splitTestNameVersion, info] = token.split(KEY_VALUE_SEP) - const [splitTestId64, versionStr] = - splitTestNameVersion.split(ID_VERSION_SEP) + // 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 => splitTestId64 === _convertIdToBase64(test._id) - ) - if (!splitTest) { - continue - } + 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(VARIANT_DATE_SEP) - 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 - } + 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] = [] + 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' + ) } - 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 cached anonymous split test assignments from session' - ) } } @@ -78,23 +73,36 @@ async function getAssignments(session) { } async function appendAssignment(session, assignment) { - await _convertAnonymousAssignmentsIfNeeded(session) + // await _convertAnonymousAssignmentsIfNeeded(session) + if (!session.splitTests) { + session.splitTests = {} + } + if (!session.splitTests[assignment.splitTestName]) { + session.splitTests[assignment.splitTestName] = [] + } + + const assignments = await getAssignments(session) if ( - !_hasExistingAssignment( - session, - assignment.splitTestId, - assignment.versionNumber - ) + !_.find(assignments[assignment.splitTestName], { + variantName: assignment.variantName, + 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 ? TOKEN_SEP : '' - session.sta += `${separator}${assignmentString}` + // 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({ + variantName: assignment.variantName, + versionNumber: assignment.versionNumber, + phase: assignment.phase, + assignedAt: assignment.assignedAt, + }) } } @@ -174,63 +182,62 @@ function collectSessionStats(session) { JSON.stringify(session.cachedSplitTestAssignments).length ) } - if (session.sta) { + if (session.splitTests) { Metrics.summary( 'split_test_session_storage_count', - (session.sta || '').split(';').length + (session.sta || '').split(';').length + + Object.keys(session.splitTests).length ) Metrics.summary( 'split_test_session_storage_size', - (session.sta || '').length + (session.sta || '').length + JSON.stringify(session.splitTests).length ) } } -async function _convertAnonymousAssignmentsIfNeeded(session) { - if (session.splitTests) { - const splitTests = await SplitTestCache.get('') - if (!session.sta) { - session.sta = '' - } - for (const [splitTestName, assignments] of Object.entries( - session.splitTests - )) { - const splitTest = splitTests.get(splitTestName) - for (const assignment of assignments) { - const assignmentString = _buildAssignmentString(splitTest, assignment) - const separator = session.sta.length > 0 ? TOKEN_SEP : '' - session.sta += `${separator}${assignmentString}` - } - } - delete session.splitTests - } -} +// 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)}${ID_VERSION_SEP}${versionNumber}=` - ) - return index >= 0 -} +// 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}${ID_VERSION_SEP}${versionNumber}${KEY_VALUE_SEP}${variantChar}${VARIANT_DATE_SEP}${timestamp}` -} +// 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') -} +// function _convertIdToBase64(id) { +// return new ObjectId(id).toString('base64') +// } module.exports = { getAssignments: callbackify(getAssignments), diff --git a/services/web/test/unit/src/SplitTests/SplitTestSessionHandlerTests.js b/services/web/test/unit/src/SplitTests/SplitTestSessionHandlerTests.js index b9216c8464..42c29d1ce8 100644 --- a/services/web/test/unit/src/SplitTests/SplitTestSessionHandlerTests.js +++ b/services/web/test/unit/src/SplitTests/SplitTestSessionHandlerTests.js @@ -15,45 +15,6 @@ describe('SplitTestSessionHandler', function () { } this.SplitTestUserGetter = {} this.Metrics = {} - - 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', - }, - ], - }, - ], - }, - }) - ) - ) - this.SplitTestSessionHandler = SandboxedModule.require(MODULE_PATH, { requires: { './SplitTestCache': this.SplitTestCache, @@ -191,6 +152,43 @@ describe('SplitTestSessionHandler', function () { }) 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': [ @@ -225,18 +223,18 @@ describe('SplitTestSessionHandler', function () { }, ], 'anon-test-2': [ - { - variantName: 'v-2', - versionNumber: 2, - phase: 'release', - assignedAt: new Date(1712858400000), - }, { variantName: 'default', versionNumber: 1, phase: 'release', assignedAt: new Date(1712307600000), }, + { + variantName: 'v-2', + versionNumber: 2, + phase: 'release', + assignedAt: new Date(1712858400000), + }, ], }) })