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

Revert "[web] Store anonymous users split test assignments in new format in session" GitOrigin-RevId: 2c1a95031a9d1d99b9dfef54eb4b80264a32ba0d
2024-11-21 20:47:08 -05:00 · 2024-04-18 19:08:33 +02:00 · 2024-04-18 19:08:33 +02:00 · 14bbc65e99
commit 14bbc65e99
parent bfe75c7d31
5 changed files with 169 additions and 163 deletions
--- 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) {
--- 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 = {
--- 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')
  }
--- 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),
--- 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),
+        },
      ],
    })
  })