Merge pull request #18022 from overleaf/revert-18018-revert-17906-ab-split-test-assignments-optim-pt2

Revert "Revert "[web] Store anonymous users split test assignments in new format in session""

GitOrigin-RevId: bd48a21c17df91eda1536378d8d999a1933e83a2

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) {

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 = {

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')
   }

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)
-
-  if (!session.splitTests) {
-    session.splitTests = {}
-  }
-  if (!session.splitTests[assignment.splitTestName]) {
-    session.splitTests[assignment.splitTestName] = []
-  }
+  await _convertAnonymousAssignmentsIfNeeded(session)
 
-  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}`
-//       }
-//     }
-//   }
-// }
-
-// 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')
-// }
+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)}${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}${ID_VERSION_SEP}${versionNumber}${KEY_VALUE_SEP}${variantChar}${VARIANT_DATE_SEP}${timestamp}`
+}
+
+function _convertIdToBase64(id) {
+  return new ObjectId(id).toString('base64')
+}
 
 module.exports = {
   getAssignments: callbackify(getAssignments),

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),
+        },
       ],
     })
   })