From b19c56ccafe3ce8d1d587a03a59e6ce5bcf6748b Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 15 Jun 2022 15:04:50 +0100 Subject: [PATCH] Merge pull request #8396 from overleaf/jpa-split-test-mongo-user [web] implement split test assignment based on mongo user GitOrigin-RevId: d3e2dff6a5e925cfd0426e9ebfeb7b64dc803f42 --- .../src/Features/Analytics/AnalyticsHelper.js | 8 ++ .../Features/Analytics/AnalyticsManager.js | 4 +- .../Analytics/UserAnalyticsIdCache.js | 3 +- .../Collaborators/OwnershipTransferHandler.js | 8 ++ .../app/src/Features/Compile/ClsiManager.js | 39 +++++- .../src/Features/Compile/CompileController.js | 15 +++ .../src/Features/Compile/CompileManager.js | 50 +++++++- .../Features/SplitTests/SplitTestHandler.js | 32 ++++- services/web/config/settings.defaults.js | 1 + .../OwnershipTransferHandlerTests.js | 20 +++ .../test/unit/src/Compile/ClsiManagerTests.js | 70 ++++++---- .../src/Compile/CompileControllerTests.js | 3 + .../unit/src/Compile/CompileManagerTests.js | 121 +++++++++++++++++- 13 files changed, 334 insertions(+), 40 deletions(-) create mode 100644 services/web/app/src/Features/Analytics/AnalyticsHelper.js diff --git a/services/web/app/src/Features/Analytics/AnalyticsHelper.js b/services/web/app/src/Features/Analytics/AnalyticsHelper.js new file mode 100644 index 0000000000..2703a6f531 --- /dev/null +++ b/services/web/app/src/Features/Analytics/AnalyticsHelper.js @@ -0,0 +1,8 @@ +function getAnalyticsIdFromMongoUser(user) { + // ensure `analyticsId` is set to the user's `analyticsId`, and fallback to their `userId` for pre-analyticsId users + return user.analyticsId || user._id +} + +module.exports = { + getAnalyticsIdFromMongoUser, +} diff --git a/services/web/app/src/Features/Analytics/AnalyticsManager.js b/services/web/app/src/Features/Analytics/AnalyticsManager.js index 56d4e0dfaa..215d43e6d9 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsManager.js +++ b/services/web/app/src/Features/Analytics/AnalyticsManager.js @@ -7,6 +7,7 @@ const uuid = require('uuid') const _ = require('lodash') const { expressify } = require('../../util/promises') const { logger } = require('@overleaf/logger') +const { getAnalyticsIdFromMongoUser } = require('./AnalyticsHelper') const analyticsEventsQueue = Queues.getQueue('analytics-events') const analyticsEditingSessionsQueue = Queues.getQueue( @@ -240,8 +241,7 @@ async function analyticsIdMiddleware(req, res, next) { const sessionUser = SessionManager.getSessionUser(session) if (sessionUser) { - // ensure `session.analyticsId` is set to the user's `analyticsId`, and fallback to their `userId` for pre-analyticsId users - session.analyticsId = sessionUser.analyticsId || sessionUser._id + session.analyticsId = getAnalyticsIdFromMongoUser(sessionUser) } else if (!session.analyticsId) { // generate an `analyticsId` if needed session.analyticsId = uuid.v4() diff --git a/services/web/app/src/Features/Analytics/UserAnalyticsIdCache.js b/services/web/app/src/Features/Analytics/UserAnalyticsIdCache.js index c665f68bad..77e7039478 100644 --- a/services/web/app/src/Features/Analytics/UserAnalyticsIdCache.js +++ b/services/web/app/src/Features/Analytics/UserAnalyticsIdCache.js @@ -1,5 +1,6 @@ const UserGetter = require('../User/UserGetter') const { CacheLoader } = require('cache-flow') +const { getAnalyticsIdFromMongoUser } = require('./AnalyticsHelper') class UserAnalyticsIdCache extends CacheLoader { constructor() { @@ -12,7 +13,7 @@ class UserAnalyticsIdCache extends CacheLoader { async load(userId) { const user = await UserGetter.promises.getUser(userId, { analyticsId: 1 }) if (user) { - return user.analyticsId || user._id + return getAnalyticsIdFromMongoUser(user) } } diff --git a/services/web/app/src/Features/Collaborators/OwnershipTransferHandler.js b/services/web/app/src/Features/Collaborators/OwnershipTransferHandler.js index 4d86113ebe..b9ab413b56 100644 --- a/services/web/app/src/Features/Collaborators/OwnershipTransferHandler.js +++ b/services/web/app/src/Features/Collaborators/OwnershipTransferHandler.js @@ -8,6 +8,7 @@ const Errors = require('../Errors/Errors') const PrivilegeLevels = require('../Authorization/PrivilegeLevels') const TpdsProjectFlusher = require('../ThirdPartyDataStore/TpdsProjectFlusher') const ProjectAuditLogHandler = require('../Project/ProjectAuditLogHandler') +const AnalyticsManager = require('../Analytics/AnalyticsManager') module.exports = { promises: { transferOwnership }, @@ -36,6 +37,13 @@ async function transferOwnership(projectId, newOwnerId, options = {}) { throw new Errors.UserNotCollaboratorError({ info: { userId: newOwnerId } }) } + // Track the change of ownership in BigQuery. + AnalyticsManager.recordEventForUser( + previousOwnerId, + 'project-ownership-transfer', + { projectId, newOwnerId } + ) + // Transfer ownership await ProjectAuditLogHandler.promises.addEntry( projectId, diff --git a/services/web/app/src/Features/Compile/ClsiManager.js b/services/web/app/src/Features/Compile/ClsiManager.js index 8466b32aaf..1c67c95b02 100644 --- a/services/web/app/src/Features/Compile/ClsiManager.js +++ b/services/web/app/src/Features/Compile/ClsiManager.js @@ -122,8 +122,9 @@ const ClsiManager = { if (options == null) { options = {} } - const { compileGroup } = options + const { compileBackendClass, compileGroup } = options const compilerUrl = this._getCompilerUrl( + compileBackendClass, compileGroup, projectId, userId, @@ -140,8 +141,13 @@ const ClsiManager = { if (options == null) { options = {} } - const { compileGroup } = options - const compilerUrl = this._getCompilerUrl(compileGroup, projectId, userId) + const { compileBackendClass, compileGroup } = options + const compilerUrl = this._getCompilerUrl( + compileBackendClass, + compileGroup, + projectId, + userId + ) const opts = { url: compilerUrl, method: 'DELETE', @@ -236,6 +242,7 @@ const ClsiManager = { projectId, userId, req, + options.compileBackendClass, options.compileGroup, (err, response, clsiServerId) => { if (err != null) { @@ -469,7 +476,13 @@ const ClsiManager = { ) }, - _getCompilerUrl(compileGroup, projectId, userId, action) { + _getCompilerUrl( + compileBackendClass, + compileGroup, + projectId, + userId, + action + ) { const u = new URL(`/project/${projectId}`, Settings.apis.clsi.url) if (userId != null) { u.pathname += `/user/${userId}` @@ -477,12 +490,23 @@ const ClsiManager = { if (action != null) { u.pathname += `/${action}` } - u.search = new URLSearchParams({ compileGroup }).toString() + u.search = new URLSearchParams({ + compileBackendClass, + compileGroup, + }).toString() return u.href }, - _postToClsi(projectId, userId, req, compileGroup, callback) { + _postToClsi( + projectId, + userId, + req, + compileBackendClass, + compileGroup, + callback + ) { const compileUrl = this._getCompilerUrl( + compileBackendClass, compileGroup, projectId, userId, @@ -889,7 +913,7 @@ const ClsiManager = { }, wordCount(projectId, userId, file, options, clsiserverid, callback) { - const { compileGroup } = options + const { compileBackendClass, compileGroup } = options ClsiManager._buildRequest(projectId, options, (err, req) => { if (err != null) { return callback( @@ -901,6 +925,7 @@ const ClsiManager = { } const filename = file || req.compile.rootResourcePath const wordCountUrl = ClsiManager._getCompilerUrl( + compileBackendClass, compileGroup, projectId, userId, diff --git a/services/web/app/src/Features/Compile/CompileController.js b/services/web/app/src/Features/Compile/CompileController.js index 575891023d..7b8e5f9b6e 100644 --- a/services/web/app/src/Features/Compile/CompileController.js +++ b/services/web/app/src/Features/Compile/CompileController.js @@ -13,6 +13,7 @@ const ClsiCookieManager = require('./ClsiCookieManager')( Settings.apis.clsi?.backendGroupName ) const Path = require('path') +const AnalyticsManager = require('../Analytics/AnalyticsManager') const COMPILE_TIMEOUT_MS = 10 * 60 * 1000 @@ -87,6 +88,20 @@ module.exports = CompileController = { if (pdfDownloadDomain && outputUrlPrefix) { pdfDownloadDomain += outputUrlPrefix } + if (limits?.emitCompileResultEvent) { + AnalyticsManager.recordEventForSession( + req.session, + 'compile-result', + { + projectId, + ownerAnalyticsId: limits.ownerAnalyticsId, + status, + compileTime: timings?.compileE2E, + compileTimeout: limits.timeout * 1000, + clsiServerId, + } + ) + } res.json({ status, outputFiles, diff --git a/services/web/app/src/Features/Compile/CompileManager.js b/services/web/app/src/Features/Compile/CompileManager.js index a4bbcc7d78..8d38617af0 100644 --- a/services/web/app/src/Features/Compile/CompileManager.js +++ b/services/web/app/src/Features/Compile/CompileManager.js @@ -8,6 +8,8 @@ const UserGetter = require('../User/UserGetter') const ClsiManager = require('./ClsiManager') const Metrics = require('@overleaf/metrics') const rateLimiter = require('../../infrastructure/RateLimiter') +const SplitTestHandler = require('../SplitTests/SplitTestHandler') +const { getAnalyticsIdFromMongoUser } = require('../Analytics/AnalyticsHelper') module.exports = CompileManager = { compile(projectId, userId, options = {}, _callback) { @@ -140,7 +142,14 @@ module.exports = CompileManager = { } UserGetter.getUser( project.owner_ref, - { alphaProgram: 1, betaProgram: 1, features: 1 }, + { + _id: 1, + alphaProgram: 1, + analyticsId: 1, + betaProgram: 1, + features: 1, + splitTests: 1, + }, function (err, owner) { if (err) { return callback(err) @@ -150,14 +159,25 @@ module.exports = CompileManager = { if (owner && owner.alphaProgram) { ownerFeatures.compileGroup = 'alpha' } - callback(null, { + const limits = { timeout: ownerFeatures.compileTimeout || Settings.defaultFeatures.compileTimeout, compileGroup: ownerFeatures.compileGroup || Settings.defaultFeatures.compileGroup, - }) + ownerAnalyticsId: getAnalyticsIdFromMongoUser(owner), + } + CompileManager._getCompileBackendClassDetails( + owner, + limits.compileGroup, + (err, { compileBackendClass, emitCompileResultEvent }) => { + if (err) return callback(err) + limits.compileBackendClass = compileBackendClass + limits.emitCompileResultEvent = emitCompileResultEvent + callback(null, limits) + } + ) } ) } @@ -225,6 +245,30 @@ module.exports = CompileManager = { }) }, + _getCompileBackendClassDetails(owner, compileGroup, callback) { + const { defaultBackendClass } = Settings.apis.clsi + if (compileGroup === 'standard') { + return callback(null, { + compileBackendClass: defaultBackendClass, + emitCompileResultEvent: false, + }) + } + SplitTestHandler.getAssignmentForMongoUser( + owner, + 'compile-backend-class', + (err, assignment) => { + if (err) return callback(err, {}) + const { analytics, variant } = assignment + const activeForUser = analytics?.segmentation?.splitTest != null + callback(null, { + compileBackendClass: + variant === 'default' ? defaultBackendClass : variant, + emitCompileResultEvent: activeForUser, + }) + } + ) + }, + wordCount(projectId, userId, file, clsiserverid, callback) { CompileManager.getProjectCompileLimits(projectId, function (error, limits) { if (error) { diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index 758a441c0e..98f72301ef 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -8,6 +8,7 @@ const { callbackify } = require('util') const SplitTestCache = require('./SplitTestCache') const { SplitTest } = require('../../models/SplitTest') const UserAnalyticsIdCache = require('../Analytics/UserAnalyticsIdCache') +const { getAnalyticsIdFromMongoUser } = require('../Analytics/AnalyticsHelper') const DEFAULT_VARIANT = 'default' const ALPHA_PHASE = 'alpha' @@ -101,6 +102,27 @@ async function getAssignmentForUser( return _getAssignment(splitTestName, { analyticsId, userId, sync }) } +/** + * Get the assignment of a user to a split test by their pre-fetched mongo doc. + * + * @param user the user + * @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 getAssignmentForMongoUser( + user, + splitTestName, + { sync = false } = {} +) { + return _getAssignment(splitTestName, { + analyticsId: getAnalyticsIdFromMongoUser(user), + sync, + user, + userId: user._id.toString(), + }) +} + /** * Get a mapping of the active split test assignments for the given user */ @@ -157,7 +179,7 @@ async function _getVariantNames(splitTestName) { async function _getAssignment( splitTestName, - { analyticsId, userId, session, sync } + { analyticsId, user, userId, session, sync } ) { if (!analyticsId && !userId) { return DEFAULT_ASSIGNMENT @@ -179,11 +201,12 @@ async function _getAssignment( return _makeAssignment(splitTest, cachedVariant, currentVersion) } } - const user = userId && (await _getUser(userId)) + user = user || (userId && (await _getUser(userId))) const { activeForUser, selectedVariantName, phase, versionNumber } = await _getAssignmentMetadata(analyticsId, user, splitTest) if (activeForUser) { const assignmentConfig = { + user, userId, analyticsId, session, @@ -256,6 +279,7 @@ function _getVariantFromPercentile(variants, percentile) { } async function _updateVariantAssignment({ + user, userId, analyticsId, session, @@ -272,7 +296,7 @@ async function _updateVariantAssignment({ } // if the user is logged in if (userId) { - const user = await _getUser(userId) + user = user || (await _getUser(userId)) if (user) { const assignedSplitTests = user.splitTests || [] const assignmentLog = assignedSplitTests[splitTestName] || [] @@ -350,10 +374,12 @@ async function _getUser(id) { module.exports = { getAssignment: callbackify(getAssignment), + getAssignmentForMongoUser: callbackify(getAssignmentForMongoUser), getAssignmentForUser: callbackify(getAssignmentForUser), getActiveAssignmentsForUser: callbackify(getActiveAssignmentsForUser), promises: { getAssignment, + getAssignmentForMongoUser, getAssignmentForUser, getActiveAssignmentsForUser, }, diff --git a/services/web/config/settings.defaults.js b/services/web/config/settings.defaults.js index f21a55ae9a..1a5136d3ad 100644 --- a/services/web/config/settings.defaults.js +++ b/services/web/config/settings.defaults.js @@ -202,6 +202,7 @@ module.exports = { url: `http://${process.env.CLSI_HOST || 'localhost'}:3013`, // url: "http://#{process.env['CLSI_LB_HOST']}:3014" backendGroupName: undefined, + defaultBackendClass: process.env.CLSI_DEFAULT_BACKEND_CLASS || 'e2', }, realTime: { url: `http://${process.env.REALTIME_HOST || 'localhost'}:3026`, diff --git a/services/web/test/unit/src/Collaborators/OwnershipTransferHandlerTests.js b/services/web/test/unit/src/Collaborators/OwnershipTransferHandlerTests.js index 07663a13d3..89b469381e 100644 --- a/services/web/test/unit/src/Collaborators/OwnershipTransferHandlerTests.js +++ b/services/web/test/unit/src/Collaborators/OwnershipTransferHandlerTests.js @@ -70,6 +70,9 @@ describe('OwnershipTransferHandler', function () { '../Project/ProjectAuditLogHandler': this.ProjectAuditLogHandler, '../Email/EmailHandler': this.EmailHandler, './CollaboratorsHandler': this.CollaboratorsHandler, + '../Analytics/AnalyticsManager': { + recordEventForUser: (this.recordEventForUser = sinon.stub()), + }, }, }) }) @@ -192,6 +195,23 @@ describe('OwnershipTransferHandler', function () { ) }) + it('should track the change in BigQuery', async function () { + const sessionUserId = ObjectId() + await this.handler.promises.transferOwnership( + this.project._id, + this.collaborator._id, + { sessionUserId } + ) + expect(this.recordEventForUser).to.have.been.calledWith( + this.user._id, + 'project-ownership-transfer', + { + projectId: this.project._id, + newOwnerId: this.collaborator._id, + } + ) + }) + it('should write an entry in the audit log', async function () { const sessionUserId = ObjectId() await this.handler.promises.transferOwnership( diff --git a/services/web/test/unit/src/Compile/ClsiManagerTests.js b/services/web/test/unit/src/Compile/ClsiManagerTests.js index 83921763e5..303d2e842c 100644 --- a/services/web/test/unit/src/Compile/ClsiManagerTests.js +++ b/services/web/test/unit/src/Compile/ClsiManagerTests.js @@ -43,6 +43,7 @@ describe('ClsiManager', function () { }, clsi: { url: 'http://clsi.example.com', + defaultBackendClass: 'e2', }, clsi_priority: { url: 'https://clsipremium.example.com', @@ -61,6 +62,11 @@ describe('ClsiManager', function () { request: this.request, './ClsiFormatChecker': this.ClsiFormatChecker, '@overleaf/metrics': this.Metrics, + '../SplitTests/SplitTestHandler': { + getAssignment: (this.getAssignment = sinon.stub().yields(null, { + variant: 'default', + })), + }, }, }) this.project_id = 'project-id' @@ -78,7 +84,7 @@ describe('ClsiManager', function () { describe('with a successful compile', function () { beforeEach(function () { - this.ClsiManager._postToClsi = sinon.stub().callsArgWith(4, null, { + this.ClsiManager._postToClsi = sinon.stub().yields(null, { compile: { status: (this.status = 'success'), outputFiles: [ @@ -100,7 +106,7 @@ describe('ClsiManager', function () { this.ClsiManager.sendRequest( this.project_id, this.user_id, - { compileGroup: 'standard' }, + { compileBackendClass: 'e2', compileGroup: 'standard' }, this.callback ) }) @@ -113,7 +119,13 @@ describe('ClsiManager', function () { it('should send the request to the CLSI', function () { this.ClsiManager._postToClsi - .calledWith(this.project_id, this.user_id, this.request, 'standard') + .calledWith( + this.project_id, + this.user_id, + this.request, + 'e2', + 'standard' + ) .should.equal(true) }) @@ -180,7 +192,7 @@ describe('ClsiManager', function () { this.ClsiManager.sendRequest( this.project_id, this.user_id, - { compileGroup: 'standard' }, + { compileBackendClass: 'e2', compileGroup: 'standard' }, this.callback ) }) @@ -222,7 +234,7 @@ describe('ClsiManager', function () { describe('with a failed compile', function () { beforeEach(function () { - this.ClsiManager._postToClsi = sinon.stub().callsArgWith(4, null, { + this.ClsiManager._postToClsi = sinon.stub().yields(null, { compile: { status: (this.status = 'failure'), }, @@ -327,7 +339,7 @@ describe('ClsiManager', function () { this.ClsiFormatChecker.checkRecoursesForProblems = sinon .stub() .callsArgWith(1, new Error('failed')) - this.ClsiManager._postToClsi = sinon.stub().callsArgWith(4, null, { + this.ClsiManager._postToClsi = sinon.stub().yields(null, { compile: { status: (this.status = 'failure'), }, @@ -359,7 +371,7 @@ describe('ClsiManager', function () { describe('with a successful compile', function () { beforeEach(function () { - this.ClsiManager._postToClsi = sinon.stub().callsArgWith(4, null, { + this.ClsiManager._postToClsi = sinon.stub().yields(null, { compile: { status: (this.status = 'success'), outputFiles: [ @@ -381,14 +393,20 @@ describe('ClsiManager', function () { this.ClsiManager.sendExternalRequest( this.submission_id, this.clsi_request, - { compileGroup: 'standard' }, + { compileBackendClass: 'e2', compileGroup: 'standard' }, this.callback ) }) it('should send the request to the CLSI', function () { this.ClsiManager._postToClsi - .calledWith(this.submission_id, null, this.clsi_request, 'standard') + .calledWith( + this.submission_id, + null, + this.clsi_request, + 'e2', + 'standard' + ) .should.equal(true) }) @@ -423,7 +441,7 @@ describe('ClsiManager', function () { describe('with a failed compile', function () { beforeEach(function () { - this.ClsiManager._postToClsi = sinon.stub().callsArgWith(4, null, { + this.ClsiManager._postToClsi = sinon.stub().yields(null, { compile: { status: (this.status = 'failure'), }, @@ -446,7 +464,7 @@ describe('ClsiManager', function () { this.ClsiFormatChecker.checkRecoursesForProblems = sinon .stub() .callsArgWith(1, new Error('failed')) - this.ClsiManager._postToClsi = sinon.stub().callsArgWith(4, null, { + this.ClsiManager._postToClsi = sinon.stub().yields(null, { compile: { status: (this.status = 'failure'), }, @@ -480,7 +498,7 @@ describe('ClsiManager', function () { this.ClsiManager.deleteAuxFiles( this.project_id, this.user_id, - { compileGroup: 'standard' }, + { compileBackendClass: 'e2', compileGroup: 'standard' }, 'node-1', this.callback ) @@ -494,7 +512,7 @@ describe('ClsiManager', function () { 'standard', { method: 'DELETE', - url: `${this.settings.apis.clsi.url}/project/${this.project_id}/user/${this.user_id}?compileGroup=standard`, + url: `${this.settings.apis.clsi.url}/project/${this.project_id}/user/${this.user_id}?compileBackendClass=e2&compileGroup=standard`, }, 'node-1' ) @@ -568,7 +586,7 @@ describe('ClsiManager', function () { beforeEach(function (done) { this.ClsiManager._buildRequest( this.project_id, - { timeout: 100, compileGroup: 'standard' }, + { timeout: 100, compileBackendClass: 'e2', compileGroup: 'standard' }, (err, request) => { if (err != null) { return done(err) @@ -930,13 +948,14 @@ describe('ClsiManager', function () { this.project_id, this.user_id, this.req, + 'e2', 'standard', this.callback ) }) it('should send the request to the CLSI', function () { - const url = `${this.settings.apis.clsi.url}/project/${this.project_id}/user/${this.user_id}/compile?compileGroup=standard` + const url = `${this.settings.apis.clsi.url}/project/${this.project_id}/user/${this.user_id}/compile?compileBackendClass=e2&compileGroup=standard` this.ClsiManager._makeRequest .calledWith(this.project_id, this.user_id, 'standard', { method: 'POST', @@ -960,6 +979,7 @@ describe('ClsiManager', function () { this.project_id, this.user_id, this.req, + 'e2', 'standard', this.callback ) @@ -990,7 +1010,7 @@ describe('ClsiManager', function () { this.project_id, this.user_id, false, - { compileGroup: 'standard' }, + { compileBackendClass: 'e2', compileGroup: 'standard' }, 'node-1', this.callback ) @@ -1004,7 +1024,7 @@ describe('ClsiManager', function () { 'standard', { method: 'GET', - url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount?compileGroup=standard`, + url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount?compileBackendClass=e2&compileGroup=standard`, qs: { file: 'rootfile.text', image: undefined, @@ -1027,7 +1047,7 @@ describe('ClsiManager', function () { this.project_id, this.user_id, 'main.tex', - { compileGroup: 'standard' }, + { compileBackendClass: 'e2', compileGroup: 'standard' }, 'node-2', this.callback ) @@ -1041,7 +1061,7 @@ describe('ClsiManager', function () { 'standard', { method: 'GET', - url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount?compileGroup=standard`, + url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount?compileBackendClass=e2&compileGroup=standard`, qs: { file: 'main.tex', image: undefined }, json: true, }, @@ -1059,7 +1079,7 @@ describe('ClsiManager', function () { this.project_id, this.user_id, 'main.tex', - { compileGroup: 'standard' }, + { compileBackendClass: 'e2', compileGroup: 'standard' }, 'node-3', this.callback ) @@ -1073,7 +1093,7 @@ describe('ClsiManager', function () { 'standard', { method: 'GET', - url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount?compileGroup=standard`, + url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount?compileBackendClass=e2&compileGroup=standard`, qs: { file: 'main.tex', image: this.image }, json: true, }, @@ -1230,7 +1250,11 @@ describe('ClsiManager', function () { this.response = { there: 'something' } this.request.callsArgWith(1, null, this.response) this.opts = { - url: this.ClsiManager._getCompilerUrl('standard', this.project_id), + url: this.ClsiManager._getCompilerUrl( + 'e2', + 'standard', + this.project_id + ), } }) @@ -1243,7 +1267,7 @@ describe('ClsiManager', function () { () => { const args = this.request.args[0] args[0].url.should.equal( - `https://compiles.somewhere.test/project/${this.project_id}?compileGroup=standard` + `https://compiles.somewhere.test/project/${this.project_id}?compileBackendClass=e2&compileGroup=standard` ) done() } diff --git a/services/web/test/unit/src/Compile/CompileControllerTests.js b/services/web/test/unit/src/Compile/CompileControllerTests.js index fbc817e200..7117676bba 100644 --- a/services/web/test/unit/src/Compile/CompileControllerTests.js +++ b/services/web/test/unit/src/Compile/CompileControllerTests.js @@ -61,6 +61,9 @@ describe('CompileController', function () { variant: 'default', })), }, + '../Analytics/AnalyticsManager': { + recordEventForSession: sinon.stub(), + }, }, }) this.projectId = 'project-id' diff --git a/services/web/test/unit/src/Compile/CompileManagerTests.js b/services/web/test/unit/src/Compile/CompileManagerTests.js index 44b46a8c69..3bbffe028d 100644 --- a/services/web/test/unit/src/Compile/CompileManagerTests.js +++ b/services/web/test/unit/src/Compile/CompileManagerTests.js @@ -1,3 +1,4 @@ +const { expect } = require('chai') const sinon = require('sinon') const modulePath = '../../../../app/src/Features/Compile/CompileManager.js' const SandboxedModule = require('sandboxed-module') @@ -16,6 +17,7 @@ describe('CompileManager', function () { this.CompileManager = SandboxedModule.require(modulePath, { requires: { '@overleaf/settings': (this.settings = { + apis: { clsi: { defaultBackendClass: 'e2' } }, redis: { web: { host: 'localhost', port: 42 } }, rateLimit: { autoCompile: {} }, }), @@ -28,6 +30,13 @@ describe('CompileManager', function () { './ClsiManager': (this.ClsiManager = {}), '../../infrastructure/RateLimiter': this.ratelimiter, '@overleaf/metrics': this.Metrics, + '../SplitTests/SplitTestHandler': { + getAssignmentForMongoUser: (this.getAssignmentForMongoUser = sinon + .stub() + .yields(null, { + variant: 'default', + })), + }, }, }) this.project_id = 'mock-project-id-123' @@ -175,7 +184,11 @@ describe('CompileManager', function () { ) this.UserGetter.getUser = sinon .stub() - .callsArgWith(2, null, (this.user = { features: this.features })) + .callsArgWith( + 2, + null, + (this.user = { features: this.features, analyticsId: 'abc' }) + ) this.CompileManager.getProjectCompileLimits( this.project_id, this.callback @@ -191,9 +204,12 @@ describe('CompileManager', function () { it("should look up the owner's features", function () { this.UserGetter.getUser .calledWith(this.project.owner_ref, { + _id: 1, alphaProgram: 1, + analyticsId: 1, betaProgram: 1, features: 1, + splitTests: 1, }) .should.equal(true) }) @@ -203,11 +219,114 @@ describe('CompileManager', function () { .calledWith(null, { timeout: this.timeout, compileGroup: this.group, + compileBackendClass: 'e2', + ownerAnalyticsId: 'abc', + emitCompileResultEvent: false, }) .should.equal(true) }) }) + describe('compileBackendClass', function () { + beforeEach(function () { + this.features = { + compileTimeout: 42, + compileGroup: 'standard', + } + this.ProjectGetter.getProject = sinon + .stub() + .yields(null, { owner_ref: 'owner-id-123' }) + this.UserGetter.getUser = sinon + .stub() + .yields(null, { features: this.features, analyticsId: 'abc' }) + }) + + describe('with standard compile', function () { + beforeEach(function () { + this.features.compileGroup = 'standard' + }) + it('should return the default class and disable event', function (done) { + this.CompileManager.getProjectCompileLimits( + this.project_id, + (err, { compileBackendClass, emitCompileResultEvent }) => { + if (err) return done(err) + expect(compileBackendClass).to.equal('e2') + expect(emitCompileResultEvent).to.equal(false) + done() + } + ) + }) + }) + + describe('with priority compile', function () { + beforeEach(function () { + this.features.compileGroup = 'priority' + }) + describe('split test not active', function () { + beforeEach(function () { + this.getAssignmentForMongoUser.yields(null, { + analytics: { segmentation: {} }, + variant: 'default', + }) + }) + + it('should return the default class and disable event', function (done) { + this.CompileManager.getProjectCompileLimits( + this.project_id, + (err, { compileBackendClass, emitCompileResultEvent }) => { + if (err) return done(err) + expect(compileBackendClass).to.equal('e2') + expect(emitCompileResultEvent).to.equal(false) + done() + } + ) + }) + }) + + describe('split test active', function () { + describe('default variant', function () { + beforeEach(function () { + this.getAssignmentForMongoUser.yields(null, { + analytics: { segmentation: { splitTest: 'foo' } }, + variant: 'default', + }) + }) + it('should return the default class and enable event', function (done) { + this.CompileManager.getProjectCompileLimits( + this.project_id, + (err, { compileBackendClass, emitCompileResultEvent }) => { + if (err) return done(err) + expect(compileBackendClass).to.equal('e2') + expect(emitCompileResultEvent).to.equal(true) + done() + } + ) + }) + }) + + describe('c2d variant', function () { + beforeEach(function () { + this.getAssignmentForMongoUser.yields(null, { + analytics: { segmentation: { splitTest: 'foo' } }, + variant: 'c2d', + }) + }) + it('should return the c2d class and enable event', function (done) { + this.CompileManager.getProjectCompileLimits( + this.project_id, + (err, { compileBackendClass, emitCompileResultEvent }) => { + if (err) return done(err) + expect(compileBackendClass).to.equal('c2d') + expect(emitCompileResultEvent).to.equal(true) + done() + } + ) + }) + }) + }) + }) + }) + describe('deleteAuxFiles', function () { beforeEach(function () { this.CompileManager.getProjectCompileLimits = sinon