Merge pull request #7545 from overleaf/td-split-test-data-sentry

Record split-test state in Sentry metadata from web clients

GitOrigin-RevId: 66dd195c546bd9fb0aedac52844200846c5012ca

services/web/app/src/Features/Project/ProjectController.js

@@ -443,6 +443,7 @@ const ProjectController = {
         primaryEmailCheckActive(cb) {
           SplitTestHandler.getAssignment(
             req,
+            res,
             'primary-email-check',
             (err, assignment) => {
               if (err) {
@@ -634,7 +635,11 @@ const ProjectController = {
           }
 
           // null test targeting logged in users
-          SplitTestHandler.promises.getAssignment(req, 'null-test-dashboard')
+          SplitTestHandler.promises.getAssignment(
+            req,
+            res,
+            'null-test-dashboard'
+          )
 
           res.render('project/list', viewModel)
           timer.done()
@@ -761,7 +766,7 @@ const ProjectController = {
           TpdsProjectFlusher.flushProjectToTpdsIfNeeded(projectId, cb)
         },
         sharingModalSplitTest(cb) {
-          SplitTestHandler.assignInLocalsContext(
+          SplitTestHandler.getAssignment(
             req,
             res,
             'project-share-modal-paywall',
@@ -774,7 +779,7 @@ const ProjectController = {
         },
         sharingModalNullTest(cb) {
           // null test targeting logged in users, for front-end side
-          SplitTestHandler.assignInLocalsContext(
+          SplitTestHandler.getAssignment(
             req,
             res,
             'null-test-share-modal',
@@ -788,6 +793,7 @@ const ProjectController = {
         newSourceEditorAssignment(cb) {
           SplitTestHandler.getAssignment(
             req,
+            res,
             'source-editor',
             {},
             (error, assignment) => {
@@ -803,6 +809,7 @@ const ProjectController = {
         pdfDetachAssignment(cb) {
           SplitTestHandler.getAssignment(
             req,
+            res,
             'pdf-detach',
             {},
             (error, assignment) => {
@@ -818,6 +825,7 @@ const ProjectController = {
         pdfjsAssignment(cb) {
           SplitTestHandler.getAssignment(
             req,
+            res,
             'pdfjs',
             {},
             (error, assignment) => {

services/web/app/src/Features/SplitTests/SplitTestHandler.js

@@ -20,12 +20,12 @@ const DEFAULT_ASSIGNMENT = {
 }
 
 /**
- * Get the assignment of a user to a split test by their session.
+ * Get the assignment of a user to a split test and store it in the response locals context
  *
  * @example
  * // Assign user and record an event
  *
- * const assignment = await SplitTestHandler.getAssignment(req.session, 'example-project')
+ * const assignment = await SplitTestHandler.getAssignment(req, res, 'example-project')
  * if (assignment.variant === 'awesome-new-version') {
  *   // execute my awesome change
  * }
@@ -39,29 +39,47 @@ const DEFAULT_ASSIGNMENT = {
  * })
  *
  * @param req the request
+ * @param res the Express response object
  * @param splitTestName the unique name of the split test
  * @param options {Object<sync: boolean>} - for test purposes only, to force the synchronous update of the user's profile
  * @returns {Promise<{variant: string, analytics: {segmentation: {splitTest: string, variant: string, phase: string, versionNumber: number}|{}}}>}
  */
-async function getAssignment(req, splitTestName, { sync = false } = {}) {
+async function getAssignment(req, res, splitTestName, { sync = false } = {}) {
   const query = req.query || {}
-  if (query[splitTestName]) {
-    return {
-      variant: query[splitTestName],
+  let assignment
+
+  // Check the query string for an override, ignoring an invalid value
+  const queryVariant = query[splitTestName]
+  if (queryVariant) {
+    const variants = await _getVariantNames(splitTestName)
+    if (variants.includes(queryVariant)) {
+      assignment = {
+        variant: queryVariant,
         analytics: {
           segmentation: {},
         },
       }
     }
+  }
+
+  if (!assignment) {
     const { userId, analyticsId } = AnalyticsManager.getIdsFromSession(
       req.session
     )
-  return _getAssignment(splitTestName, {
+    assignment = await _getAssignment(splitTestName, {
       analyticsId,
       userId,
       session: req.session,
       sync,
     })
+  }
+
+  LocalsHelper.setSplitTestVariant(
+    res.locals,
+    splitTestName,
+    assignment.variant
+  )
+  return assignment
 }
 
 /**
@@ -83,29 +101,6 @@ async function getAssignmentForUser(
   return _getAssignment(splitTestName, { analyticsId, userId, sync })
 }
 
-/**
- * Get the assignment of a user to a split test by their session and stores it in the locals context.
- *
- * @param req the request
- * @param res the Express response object
- * @param splitTestName the unique name of the split test
- * @param options {Object<sync: boolean>} - for test purposes only, to force the synchronous update of the user's profile
- * @returns {Promise<void>}
- */
-async function assignInLocalsContext(
-  req,
-  res,
-  splitTestName,
-  { sync = false } = {}
-) {
-  const assignment = await getAssignment(req, splitTestName, { sync })
-  LocalsHelper.setSplitTestVariant(
-    res.locals,
-    splitTestName,
-    assignment.variant
-  )
-}
-
 /**
  * Get a mapping of the active split test assignments for the given user
  */
@@ -143,6 +138,23 @@ async function getActiveAssignmentsForUser(userId) {
   return assignments
 }
 
+/**
+ * Returns an array of valid variant names for the given split test, including default
+ *
+ * @param splitTestName
+ * @returns {Promise<string[]>}
+ * @private
+ */
+async function _getVariantNames(splitTestName) {
+  const splitTest = await SplitTestCache.get(splitTestName)
+  const currentVersion = splitTest?.getCurrentVersion()
+  if (currentVersion?.active) {
+    return currentVersion.variants.map(v => v.name).concat([DEFAULT_VARIANT])
+  } else {
+    return [DEFAULT_VARIANT]
+  }
+}
+
 async function _getAssignment(
   splitTestName,
   { analyticsId, userId, session, sync }
@@ -153,7 +165,7 @@ async function _getAssignment(
 
   const splitTest = await SplitTestCache.get(splitTestName)
   const currentVersion = splitTest?.getCurrentVersion()
-  if (!splitTest || !currentVersion?.active) {
+  if (!currentVersion?.active) {
     return DEFAULT_ASSIGNMENT
   }
 
@@ -339,11 +351,9 @@ module.exports = {
   getAssignment: callbackify(getAssignment),
   getAssignmentForUser: callbackify(getAssignmentForUser),
   getActiveAssignmentsForUser: callbackify(getActiveAssignmentsForUser),
-  assignInLocalsContext: callbackify(assignInLocalsContext),
   promises: {
     getAssignment,
     getAssignmentForUser,
     getActiveAssignmentsForUser,
-    assignInLocalsContext,
   },
 }

services/web/app/src/Features/SplitTests/SplitTestMiddleware.js

@@ -5,11 +5,7 @@ function loadAssignmentsInLocals(splitTestNames) {
   return async function (req, res, next) {
     try {
       for (const splitTestName of splitTestNames) {
-        await SplitTestHandler.promises.assignInLocalsContext(
-          req,
-          res,
-          splitTestName
-        )
+        await SplitTestHandler.promises.getAssignment(req, res, splitTestName)
       }
     } catch (error) {
       logger.error(

services/web/app/src/Features/StaticPages/HomeController.js

@@ -45,7 +45,11 @@ module.exports = HomeController = {
     if (Features.hasFeature('homepage') && homepageExists) {
       try {
         const highlightSSOAssignment =
-          await SplitTestHandler.promises.getAssignment(req, 'highlight-sso')
+          await SplitTestHandler.promises.getAssignment(
+            req,
+            res,
+            'highlight-sso'
+          )
         const highlightSSO = highlightSSOAssignment.variant === 'active'
         return res.render('external/home/v2', { highlightSSO })
       } catch (err) {

services/web/app/src/Features/Subscription/SubscriptionController.js

@@ -58,7 +58,11 @@ async function plansPage(req, res) {
   AnalyticsManager.recordEventForSession(req.session, 'plans-page-view')
 
   const standardPlanNameAssignment =
-    await SplitTestHandler.promises.getAssignment(req, 'standard-plan-name')
+    await SplitTestHandler.promises.getAssignment(
+      req,
+      res,
+      'standard-plan-name'
+    )
 
   const useNewPlanName =
     standardPlanNameAssignment &&
@@ -115,6 +119,7 @@ async function paymentPage(req, res) {
       }
       const assignment = await SplitTestHandler.promises.getAssignment(
         req,
+        res,
         'payment-page'
       )
       const template =
@@ -164,6 +169,7 @@ async function userSubscriptionPage(req, res) {
 
   const assignment = await SplitTestHandler.promises.getAssignment(
     req,
+    res,
     'subscription-cancel-button'
   )
 

services/web/frontend/js/infrastructure/error-reporter.js

@@ -1,4 +1,6 @@
 // Conditionally enable Sentry based on whether the DSN token is set
+import getMeta from '../utils/meta'
+
 const reporterPromise = window.ExposedSettings.sentryDsn
   ? sentryReporter()
   : nullReporter()
@@ -52,6 +54,13 @@ function sentryReporter() {
 
         Sentry.setUser({ id: window.user_id })
 
+        const splitTestAssignments = getMeta('ol-splitTestVariants')
+        if (splitTestAssignments) {
+          for (const [name, value] of Object.entries(splitTestAssignments)) {
+            Sentry.setTag(`ol.splitTest.${name}`, value.toString())
+          }
+        }
+
         return Sentry
       })
       // If Sentry fails to load, use the null reporter instead

services/web/test/acceptance/src/PrimaryEmailCheckTests.js

@@ -1,6 +1,7 @@
 const UserHelper = require('./helpers/UserHelper')
 const Settings = require('@overleaf/settings')
 const { expect } = require('chai')
+const SplitTestManager = require('../../../app/src/Features/SplitTests/SplitTestManager')
 
 // While the split test is in progress this must be appended to URLs during tests
 const SPLIT_TEST_QUERY = '?primary-email-check=active'
@@ -8,6 +9,22 @@ const SPLIT_TEST_QUERY = '?primary-email-check=active'
 describe('PrimaryEmailCheck', function () {
   let userHelper
 
+  // Create the primary-email-check split test because this is now required for the query string override to work. See
+  // https://github.com/overleaf/internal/pull/7545#discussion_r848575736
+  before(async function () {
+    await SplitTestManager.createSplitTest('primary-email-check', {
+      active: true,
+      analyticsEnabled: true,
+      phase: 'release',
+      variants: [
+        {
+          name: 'active',
+          rolloutPercent: 0,
+        },
+      ],
+    })
+  })
+
   beforeEach(async function () {
     userHelper = await UserHelper.createUser()
     userHelper = await UserHelper.loginUser(

services/web/test/unit/src/Project/ProjectControllerTests.js

@@ -126,10 +126,8 @@ describe('ProjectController', function () {
     this.SplitTestHandler = {
       promises: {
         getAssignment: sinon.stub().resolves({ variant: 'default' }),
-        assignInLocalsContext: sinon.stub().resolves({ variant: 'default' }),
       },
       getAssignment: sinon.stub().yields(null, { variant: 'default' }),
-      assignInLocalsContext: sinon.stub().yields(null, { variant: 'default' }),
     }
 
     this.ProjectController = SandboxedModule.require(MODULE_PATH, {
@@ -1413,7 +1411,7 @@ describe('ProjectController', function () {
               done()
             }
             this.SplitTestHandler.getAssignment
-              .withArgs(this.req, 'pdf-detach')
+              .withArgs(this.req, this.res, 'pdf-detach')
               .yields(null, { variant: 'enabled' })
             this.req.query.pdf_detach = 'false'
             this.ProjectController.loadEditor(this.req, this.res)
@@ -1436,7 +1434,7 @@ describe('ProjectController', function () {
               done()
             }
             this.SplitTestHandler.getAssignment
-              .withArgs(this.req, 'pdf-detach')
+              .withArgs(this.req, this.res, 'pdf-detach')
               .yields(null, { variant: 'enabled' })
             this.ProjectController.loadEditor(this.req, this.res)
           })

services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js

@@ -14,7 +14,7 @@ describe('SplitTestMiddleware', function () {
       requires: {
         './SplitTestHandler': (this.SplitTestHandler = {
           promises: {
-            assignInLocalsContext: sinon.stub().resolves(),
+            getAssignment: sinon.stub().resolves(),
           },
         }),
       },
@@ -26,12 +26,12 @@ describe('SplitTestMiddleware', function () {
   })
 
   it('assign multiple split test variants in locals', async function () {
-    this.SplitTestHandler.promises.assignInLocalsContext
+    this.SplitTestHandler.promises.getAssignment
       .withArgs(this.req, 'ui-overhaul')
       .resolves({
         variant: 'default',
       })
-    this.SplitTestHandler.promises.assignInLocalsContext
+    this.SplitTestHandler.promises.getAssignment
       .withArgs(this.req, 'other-test')
       .resolves({
         variant: 'foobar',
@@ -44,13 +44,13 @@ describe('SplitTestMiddleware', function () {
     await middleware(this.req, this.res, this.next)
 
     sinon.assert.calledWith(
-      this.SplitTestHandler.promises.assignInLocalsContext,
+      this.SplitTestHandler.promises.getAssignment,
       this.req,
       this.res,
       'ui-overhaul'
     )
     sinon.assert.calledWith(
-      this.SplitTestHandler.promises.assignInLocalsContext,
+      this.SplitTestHandler.promises.getAssignment,
       this.req,
       this.res,
       'other-test'
@@ -63,12 +63,12 @@ describe('SplitTestMiddleware', function () {
 
     await middleware(this.req, this.res, this.next)
 
-    sinon.assert.notCalled(this.SplitTestHandler.promises.assignInLocalsContext)
+    sinon.assert.notCalled(this.SplitTestHandler.promises.getAssignment)
     sinon.assert.calledOnce(this.next)
   })
 
   it('exception thrown by assignment does not fail the request', async function () {
-    this.SplitTestHandler.promises.assignInLocalsContext
+    this.SplitTestHandler.promises.getAssignment
       .withArgs(this.req, this.res, 'some-test')
       .throws(new Error('failure'))
 
@@ -79,7 +79,7 @@ describe('SplitTestMiddleware', function () {
     await middleware(this.req, this.res, this.next)
 
     sinon.assert.calledWith(
-      this.SplitTestHandler.promises.assignInLocalsContext,
+      this.SplitTestHandler.promises.getAssignment,
       this.req,
       this.res,
       'some-test'