diff --git a/services/web/app/src/Features/Analytics/AnalyticsHelper.js b/services/web/app/src/Features/Analytics/AnalyticsHelper.js deleted file mode 100644 index 2703a6f531..0000000000 --- a/services/web/app/src/Features/Analytics/AnalyticsHelper.js +++ /dev/null @@ -1,8 +0,0 @@ -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 5f3fae80d2..729f98f35a 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsManager.js +++ b/services/web/app/src/Features/Analytics/AnalyticsManager.js @@ -7,7 +7,6 @@ const crypto = require('crypto') const _ = require('lodash') const { expressify } = require('@overleaf/promise-utils') const logger = require('@overleaf/logger') -const { getAnalyticsIdFromMongoUser } = require('./AnalyticsHelper') const analyticsEventsQueue = Queues.getQueue('analytics-events') const analyticsEditingSessionsQueue = Queues.getQueue( @@ -308,12 +307,11 @@ async function analyticsIdMiddleware(req, res, next) { const sessionUser = SessionManager.getSessionUser(session) if (sessionUser) { - session.analyticsId = getAnalyticsIdFromMongoUser(sessionUser) + session.analyticsId = await UserAnalyticsIdCache.get(sessionUser._id) } else if (!session.analyticsId) { // generate an `analyticsId` if needed session.analyticsId = crypto.randomUUID() } - next() } diff --git a/services/web/app/src/Features/Analytics/UserAnalyticsIdCache.js b/services/web/app/src/Features/Analytics/UserAnalyticsIdCache.js index 77e7039478..97b4e80095 100644 --- a/services/web/app/src/Features/Analytics/UserAnalyticsIdCache.js +++ b/services/web/app/src/Features/Analytics/UserAnalyticsIdCache.js @@ -1,6 +1,6 @@ const UserGetter = require('../User/UserGetter') const { CacheLoader } = require('cache-flow') -const { getAnalyticsIdFromMongoUser } = require('./AnalyticsHelper') +const { callbackify } = require('util') class UserAnalyticsIdCache extends CacheLoader { constructor() { @@ -13,7 +13,7 @@ class UserAnalyticsIdCache extends CacheLoader { async load(userId) { const user = await UserGetter.promises.getUser(userId, { analyticsId: 1 }) if (user) { - return getAnalyticsIdFromMongoUser(user) + return user.analyticsId || user._id.toString() } } @@ -24,4 +24,8 @@ class UserAnalyticsIdCache extends CacheLoader { } } -module.exports = new UserAnalyticsIdCache() +const userAnalyticsIdCache = new UserAnalyticsIdCache() +userAnalyticsIdCache.callbacks = { + get: callbackify(userAnalyticsIdCache.get).bind(userAnalyticsIdCache), +} +module.exports = userAnalyticsIdCache diff --git a/services/web/app/src/Features/Compile/CompileManager.js b/services/web/app/src/Features/Compile/CompileManager.js index 7d9f03a022..c480644a8e 100644 --- a/services/web/app/src/Features/Compile/CompileManager.js +++ b/services/web/app/src/Features/Compile/CompileManager.js @@ -9,7 +9,7 @@ const ClsiManager = require('./ClsiManager') const Metrics = require('@overleaf/metrics') const { RateLimiter } = require('../../infrastructure/RateLimiter') const SplitTestHandler = require('../SplitTests/SplitTestHandler') -const { getAnalyticsIdFromMongoUser } = require('../Analytics/AnalyticsHelper') +const UserAnalyticsIdCache = require('../Analytics/UserAnalyticsIdCache') const NEW_COMPILE_TIMEOUT_ENFORCED_CUTOFF = new Date('2023-09-18T11:00:00.000Z') const NEW_COMPILE_TIMEOUT_ENFORCED_CUTOFF_DEFAULT_BASELINE = new Date( @@ -178,70 +178,84 @@ module.exports = CompileManager = { if (owner && owner.alphaProgram) { ownerFeatures.compileGroup = 'alpha' } - const limits = { - timeout: - ownerFeatures.compileTimeout || - Settings.defaultFeatures.compileTimeout, - compileGroup: - ownerFeatures.compileGroup || - Settings.defaultFeatures.compileGroup, - ownerAnalyticsId: getAnalyticsIdFromMongoUser(owner), - } - CompileManager._getCompileBackendClassDetails( - owner, - limits.compileGroup, - (err, { compileBackendClass, showFasterCompilesFeedbackUI }) => { - if (err) return callback(err) - limits.compileBackendClass = compileBackendClass - limits.showFasterCompilesFeedbackUI = - showFasterCompilesFeedbackUI - if (compileBackendClass === 'n2d' && limits.timeout <= 60) { - // project owners with faster compiles but with <= 60 compile timeout (default) - // will have a 20s compile timeout - // The compile-timeout-20s split test exists to enable a gradual rollout - SplitTestHandler.getAssignmentForMongoUser( - owner, - 'compile-timeout-20s', - (err, assignment) => { - if (err) return callback(err) - // users who were on the 'default' servers at time of original rollout - // will have a later cutoff date for the 20s timeout in the next phase - // we check the backend class at version 8 (baseline) - const backendClassHistory = - owner.splitTests?.['compile-backend-class-n2d'] || [] - const backendClassBaselineVariant = - backendClassHistory.find(version => { - return version.versionNumber === 8 - })?.variantName - const timeoutEnforcedCutoff = - backendClassBaselineVariant === 'default' - ? NEW_COMPILE_TIMEOUT_ENFORCED_CUTOFF_DEFAULT_BASELINE - : NEW_COMPILE_TIMEOUT_ENFORCED_CUTOFF - if (assignment?.variant === '20s') { - if (owner.signUpDate > timeoutEnforcedCutoff) { - limits.timeout = 20 - callback(null, limits) - } else { - SplitTestHandler.getAssignmentForMongoUser( - owner, - 'compile-timeout-20s-existing-users', - (err, assignmentExistingUsers) => { - if (err) return callback(err) - if (assignmentExistingUsers?.variant === '20s') { - limits.timeout = 20 - } - callback(null, limits) - } - ) - } - } else { - callback(null, limits) - } - } - ) - } else { - callback(null, limits) + UserAnalyticsIdCache.callbacks.get( + owner._id, + function (err, analyticsId) { + if (err) { + return callback(err) } + const limits = { + timeout: + ownerFeatures.compileTimeout || + Settings.defaultFeatures.compileTimeout, + compileGroup: + ownerFeatures.compileGroup || + Settings.defaultFeatures.compileGroup, + ownerAnalyticsId: analyticsId, + } + CompileManager._getCompileBackendClassDetails( + owner, + limits.compileGroup, + ( + err, + { compileBackendClass, showFasterCompilesFeedbackUI } + ) => { + if (err) return callback(err) + limits.compileBackendClass = compileBackendClass + limits.showFasterCompilesFeedbackUI = + showFasterCompilesFeedbackUI + if (compileBackendClass === 'n2d' && limits.timeout <= 60) { + // project owners with faster compiles but with <= 60 compile timeout (default) + // will have a 20s compile timeout + // The compile-timeout-20s split test exists to enable a gradual rollout + SplitTestHandler.getAssignmentForMongoUser( + owner, + 'compile-timeout-20s', + (err, assignment) => { + if (err) return callback(err) + // users who were on the 'default' servers at time of original rollout + // will have a later cutoff date for the 20s timeout in the next phase + // we check the backend class at version 8 (baseline) + const backendClassHistory = + owner.splitTests?.['compile-backend-class-n2d'] || + [] + const backendClassBaselineVariant = + backendClassHistory.find(version => { + return version.versionNumber === 8 + })?.variantName + const timeoutEnforcedCutoff = + backendClassBaselineVariant === 'default' + ? NEW_COMPILE_TIMEOUT_ENFORCED_CUTOFF_DEFAULT_BASELINE + : NEW_COMPILE_TIMEOUT_ENFORCED_CUTOFF + if (assignment?.variant === '20s') { + if (owner.signUpDate > timeoutEnforcedCutoff) { + limits.timeout = 20 + callback(null, limits) + } else { + SplitTestHandler.getAssignmentForMongoUser( + owner, + 'compile-timeout-20s-existing-users', + (err, assignmentExistingUsers) => { + if (err) return callback(err) + if ( + assignmentExistingUsers?.variant === '20s' + ) { + limits.timeout = 20 + } + callback(null, limits) + } + ) + } + } else { + callback(null, limits) + } + } + ) + } else { + callback(null, limits) + } + } + ) } ) } diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index 4ed14d6c7e..2c575d7e13 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -9,7 +9,6 @@ const { callbackify } = require('util') const SplitTestCache = require('./SplitTestCache') const { SplitTest } = require('../../models/SplitTest') const UserAnalyticsIdCache = require('../Analytics/UserAnalyticsIdCache') -const { getAnalyticsIdFromMongoUser } = require('../Analytics/AnalyticsHelper') const Features = require('../../infrastructure/Features') const SplitTestUtils = require('./SplitTestUtils') const Settings = require('@overleaf/settings') @@ -142,7 +141,7 @@ async function getAssignmentForMongoUser( } return _getAssignment(splitTestName, { - analyticsId: getAnalyticsIdFromMongoUser(user), + analyticsId: await UserAnalyticsIdCache.get(user._id), sync, user, userId: user._id.toString(), diff --git a/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js b/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js index d91bb3d5bf..0eb9468e1a 100644 --- a/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js +++ b/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js @@ -304,7 +304,10 @@ describe('AnalyticsManager', function () { } }, }, - './UserAnalyticsIdCache': {}, + + './UserAnalyticsIdCache': (this.UserAnalyticsIdCache = { + get: sinon.stub().resolves(this.analyticsId), + }), crypto: { randomUUID: () => this.analyticsId, }, @@ -343,12 +346,14 @@ describe('AnalyticsManager', function () { await this.AnalyticsManager.analyticsIdMiddleware( this.req, this.res, - this.next + () => { + assert.equal(this.analyticsId, this.req.session.analyticsId) + } ) - assert.equal(this.analyticsId, this.req.session.analyticsId) }) it('sets session.analyticsId with a legacy user session without an analyticsId', async function () { + this.UserAnalyticsIdCache.get.resolves(this.userId) this.req.session.user = { _id: this.userId, analyticsId: undefined, @@ -356,26 +361,26 @@ describe('AnalyticsManager', function () { await this.AnalyticsManager.analyticsIdMiddleware( this.req, this.res, - this.next + () => { + assert.equal(this.userId, this.req.session.analyticsId) + } ) - assert.equal(this.userId, this.req.session.analyticsId) }) it('updates session.analyticsId with a legacy user session without an analyticsId if different', async function () { + this.UserAnalyticsIdCache.get.resolves(this.userId) this.req.session.user = { _id: this.userId, analyticsId: undefined, } this.req.analyticsId = 'foo' - await this.AnalyticsManager.analyticsIdMiddleware( - this.req, - this.res, - this.next - ) - assert.equal(this.userId, this.req.session.analyticsId) + this.AnalyticsManager.analyticsIdMiddleware(this.req, this.res, () => { + assert.equal(this.userId, this.req.session.analyticsId) + }) }) it('does not update session.analyticsId with a legacy user session without an analyticsId if same', async function () { + this.UserAnalyticsIdCache.get.resolves(this.userId) this.req.session.user = { _id: this.userId, analyticsId: undefined, @@ -384,16 +389,10 @@ describe('AnalyticsManager', function () { await this.AnalyticsManager.analyticsIdMiddleware( this.req, this.res, - this.next + () => { + assert.equal(this.userId, this.req.session.analyticsId) + } ) - assert.equal(this.userId, this.req.session.analyticsId) - - await this.AnalyticsManager.analyticsIdMiddleware( - this.req, - this.res, - this.next - ) - assert.equal(this.userId, this.req.session.analyticsId) }) }) }) diff --git a/services/web/test/unit/src/Compile/CompileManagerTests.js b/services/web/test/unit/src/Compile/CompileManagerTests.js index f63e1f6be5..2b49d6853f 100644 --- a/services/web/test/unit/src/Compile/CompileManagerTests.js +++ b/services/web/test/unit/src/Compile/CompileManagerTests.js @@ -37,6 +37,11 @@ describe('CompileManager', function () { './ClsiManager': (this.ClsiManager = {}), '../../infrastructure/RateLimiter': this.RateLimiter, '@overleaf/metrics': this.Metrics, + '../Analytics/UserAnalyticsIdCache': (this.UserAnalyticsIdCache = { + callbacks: { + get: sinon.stub().yields(null, 'abc'), + }, + }), '../SplitTests/SplitTestHandler': { getAssignmentForMongoUser: (this.getAssignmentForMongoUser = sinon .stub() @@ -177,7 +182,7 @@ describe('CompileManager', function () { }) describe('getProjectCompileLimits', function () { - beforeEach(function () { + beforeEach(function (done) { this.features = { compileTimeout: (this.timeout = 42), compileGroup: (this.group = 'priority'), @@ -198,7 +203,10 @@ describe('CompileManager', function () { ) this.CompileManager.getProjectCompileLimits( this.project_id, - this.callback + (err, res) => { + this.callback(err, res) + done() + } ) })