From 35480a3c7dbedea80b74ba6ad3f67165c9bfda86 Mon Sep 17 00:00:00 2001 From: Tim Down Date: Fri, 22 Apr 2022 11:42:57 +0100 Subject: [PATCH] Merge pull request #7545 from overleaf/td-split-test-data-sentry Record split-test state in Sentry metadata from web clients GitOrigin-RevId: 66dd195c546bd9fb0aedac52844200846c5012ca --- .../src/Features/Project/ProjectController.js | 14 ++- .../Features/SplitTests/SplitTestHandler.js | 96 ++++++++++--------- .../SplitTests/SplitTestMiddleware.js | 6 +- .../Features/StaticPages/HomeController.js | 6 +- .../Subscription/SubscriptionController.js | 8 +- .../js/infrastructure/error-reporter.js | 9 ++ .../acceptance/src/PrimaryEmailCheckTests.js | 17 ++++ .../src/Project/ProjectControllerTests.js | 6 +- .../SplitTests/SplitTestMiddlewareTests.js | 16 ++-- 9 files changed, 113 insertions(+), 65 deletions(-) diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index ce0f91282d..f010050171 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/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) => { diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index 77a3048363..d35c030147 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/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} - 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], - analytics: { - segmentation: {}, - }, + 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: {}, + }, + } } } - const { userId, analyticsId } = AnalyticsManager.getIdsFromSession( - req.session + + if (!assignment) { + const { userId, analyticsId } = AnalyticsManager.getIdsFromSession( + req.session + ) + assignment = await _getAssignment(splitTestName, { + analyticsId, + userId, + session: req.session, + sync, + }) + } + + LocalsHelper.setSplitTestVariant( + res.locals, + splitTestName, + assignment.variant ) - return _getAssignment(splitTestName, { - analyticsId, - userId, - session: req.session, - sync, - }) + 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} - for test purposes only, to force the synchronous update of the user's profile - * @returns {Promise} - */ -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} + * @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, }, } diff --git a/services/web/app/src/Features/SplitTests/SplitTestMiddleware.js b/services/web/app/src/Features/SplitTests/SplitTestMiddleware.js index d08e195fb1..f7a5a1dfe6 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestMiddleware.js +++ b/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( diff --git a/services/web/app/src/Features/StaticPages/HomeController.js b/services/web/app/src/Features/StaticPages/HomeController.js index a0410f477d..acb03ecae2 100644 --- a/services/web/app/src/Features/StaticPages/HomeController.js +++ b/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) { diff --git a/services/web/app/src/Features/Subscription/SubscriptionController.js b/services/web/app/src/Features/Subscription/SubscriptionController.js index 4d8a615fc4..dc4cb3fbb5 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionController.js +++ b/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' ) diff --git a/services/web/frontend/js/infrastructure/error-reporter.js b/services/web/frontend/js/infrastructure/error-reporter.js index a5fac2cb29..062f4b8f96 100644 --- a/services/web/frontend/js/infrastructure/error-reporter.js +++ b/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 diff --git a/services/web/test/acceptance/src/PrimaryEmailCheckTests.js b/services/web/test/acceptance/src/PrimaryEmailCheckTests.js index 643b4bd4e6..0601cb0b51 100644 --- a/services/web/test/acceptance/src/PrimaryEmailCheckTests.js +++ b/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( diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index d2d9f71bea..dfd16f6018 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/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) }) diff --git a/services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js b/services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js index fbbbb21714..88acbbeced 100644 --- a/services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js +++ b/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'