diff --git a/services/web/app/src/Features/SplitTests/SplitTestCache.js b/services/web/app/src/Features/SplitTests/SplitTestCache.js index 557306d70b..4f81b451df 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestCache.js +++ b/services/web/app/src/Features/SplitTests/SplitTestCache.js @@ -12,9 +12,7 @@ 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.toObject()]) - ) + return new Map(splitTests.map(splitTest => [splitTest.name, splitTest])) } serialize(value) { diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index fbc5bbc1f8..1a5203f73a 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -492,7 +492,10 @@ function _getNonSaasAssignment(splitTestName) { async function _getSplitTest(name) { const splitTests = await SplitTestCache.get('') - return splitTests?.get(name) + const splitTest = splitTests?.get(name) + if (splitTest && !splitTest.archived) { + return splitTest + } } module.exports = { diff --git a/services/web/app/src/Features/SplitTests/SplitTestManager.js b/services/web/app/src/Features/SplitTests/SplitTestManager.js index 52b1cfdc2a..579fe5991c 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestManager.js +++ b/services/web/app/src/Features/SplitTests/SplitTestManager.js @@ -60,9 +60,7 @@ async function getSplitTests({ name, phase, type, active, archived }) { async function getRuntimeTests() { try { - return await SplitTest.find({ - archived: { $ne: true }, - }).exec() + return SplitTest.find({}).lean().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 e2aae8b35b..5dde68a1e7 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestSessionHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestSessionHandler.js @@ -9,63 +9,68 @@ 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) { - if (!session.splitTests && !session.sta) { + await _convertAnonymousAssignmentsIfNeeded(session) + + if (!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' - ) + 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) + + const splitTest = splitTests.find( + test => splitTestId64 === _convertIdToBase64(test._id) + ) + 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 + } + + 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 cached anonymous split test assignments from session' + ) } } @@ -73,36 +78,23 @@ 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 ( - !_.find(assignments[assignment.splitTestName], { - variantName: assignment.variantName, - versionNumber: assignment.versionNumber, - }) + !_hasExistingAssignment( + session, + assignment.splitTestId, + 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({ - variantName: assignment.variantName, - versionNumber: assignment.versionNumber, - phase: assignment.phase, - assignedAt: assignment.assignedAt, - }) + 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}` } } @@ -182,62 +174,63 @@ function collectSessionStats(session) { JSON.stringify(session.cachedSplitTestAssignments).length ) } - if (session.splitTests) { + if (session.sta) { Metrics.summary( 'split_test_session_storage_count', - (session.sta || '').split(';').length + - Object.keys(session.splitTests).length + (session.sta || '').split(';').length ) Metrics.summary( 'split_test_session_storage_size', - (session.sta || '').length + JSON.stringify(session.splitTests).length + (session.sta || '').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}` -// } -// } -// } -// } +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 + } +} -// function _hasExistingAssignment(session, splitTest, versionNumber) { -// if (!session.sta) { -// return false -// } -// const index = session.sta.indexOf( -// `${_convertIdToBase64(splitTest._id)}_${versionNumber}=` -// ) -// return index >= 0 -// } +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 _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 _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 _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 42c29d1ce8..b9216c8464 100644 --- a/services/web/test/unit/src/SplitTests/SplitTestSessionHandlerTests.js +++ b/services/web/test/unit/src/SplitTests/SplitTestSessionHandlerTests.js @@ -15,6 +15,45 @@ 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, @@ -152,43 +191,6 @@ 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': [ @@ -223,18 +225,18 @@ describe('SplitTestSessionHandler', function () { }, ], 'anon-test-2': [ - { - variantName: 'default', - versionNumber: 1, - phase: 'release', - assignedAt: new Date(1712307600000), - }, { variantName: 'v-2', versionNumber: 2, phase: 'release', assignedAt: new Date(1712858400000), }, + { + variantName: 'default', + versionNumber: 1, + phase: 'release', + assignedAt: new Date(1712307600000), + }, ], }) })