From 16422f972bb4da0833be87675a3d34fe694d0941 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 4 Sep 2025 09:44:51 +0200 Subject: [PATCH] Merge pull request #28265 from overleaf/jpa-double-compile [web] add c2d -> c4d double compile test GitOrigin-RevId: 186cfa867d3db5786c6d4888dfe1ca2f46a5bb0c --- .../app/src/Features/Compile/ClsiManager.js | 90 ++++++++++++++--- .../src/Compile/ClsiCookieManagerTests.js | 24 ++--- .../test/unit/src/Compile/ClsiManagerTests.js | 99 +++++++++++++------ .../src/Compile/CompileControllerTests.js | 16 +-- 4 files changed, 168 insertions(+), 61 deletions(-) diff --git a/services/web/app/src/Features/Compile/ClsiManager.js b/services/web/app/src/Features/Compile/ClsiManager.js index 27e8b9a1d5..2c710897a6 100644 --- a/services/web/app/src/Features/Compile/ClsiManager.js +++ b/services/web/app/src/Features/Compile/ClsiManager.js @@ -26,6 +26,8 @@ const Metrics = require('@overleaf/metrics') const Errors = require('../Errors/Errors') const ClsiCacheHandler = require('./ClsiCacheHandler') const { getFilestoreBlobURL } = require('../History/HistoryManager') +const SplitTestHandler = require('../SplitTests/SplitTestHandler') +const AnalyticsManager = require('../Analytics/AnalyticsManager') const VALID_COMPILERS = ['pdflatex', 'latex', 'xelatex', 'lualatex'] const OUTPUT_FILE_TIMEOUT_MS = 60000 @@ -352,15 +354,16 @@ async function _makeRequest( opts ) .then(result => { - if (result == null) { + if (result == null || !url.pathname.endsWith('/compile')) { return } - const { response: newBackendResponse } = result - Metrics.inc(`compile.newBackend.response.${newBackendResponse.status}`) + const current = json.compile + const { + body: { compile: next }, + newCompileBackendClass, + } = result const newBackendCompileTime = new Date() - newBackendStartTime - const currentStatusCode = response.status - const newStatusCode = newBackendResponse.status - const statusCodeSame = newStatusCode === currentStatusCode + const statusCodeSame = next.status === current.status const timeDifference = newBackendCompileTime - currentCompileTime logger.debug( { @@ -372,6 +375,44 @@ async function _makeRequest( }, 'both clsi requests returned' ) + if ( + current.status === 'success' && + current.status === next.status && + current.stats.isInitialCompile === next.stats.isInitialCompile && + current.stats.restoredClsiCache === next.stats.restoredClsiCache + ) { + const fraction = next.timings.compileE2E / current.timings.compileE2E + Metrics.histogram( + 'compile_backend_difference_v1', + fraction * 100, + [ + // Increment the version in the metrics name when changing the buckets. + 0, + 10, 20, 30, 40, 45, 50, 55, 60, 65, 70, 75, 80, 85, 90, 95, 100, + 105, 110, 115, 120, + ], + { path: compileBackendClass, method: newCompileBackendClass } + ) + AnalyticsManager.recordEventForUserInBackground( + userId, + 'double-compile-result', + { + projectId, + compileBackendClass, + newCompileBackendClass, + status: current.status, + compileTime: current.timings.compileE2E, + newCompileTime: next.timings.compileE2E, + clsiServerId: newClsiServerId || clsiServerId, + newClsiServerId: result.newClsiServerId, + // Successful compiles are guaranteed to have an output.pdf file. + pdfSize: current.outputFiles.find(f => f.path === 'output.pdf') + .size, + newPdfSize: next.outputFiles.find(f => f.path === 'output.pdf') + .size, + } + ) + } }) .catch(err => { logger.warn({ err }, 'Error making request to new CLSI backend') @@ -387,7 +428,7 @@ async function _makeNewBackendRequest( projectId, userId, compileGroup, - compileBackendClass, + currentCompileBackendClass, url, opts ) { @@ -398,12 +439,33 @@ async function _makeNewBackendRequest( url.toString().replace(Settings.apis.clsi.url, Settings.apis.clsi_new.url) ) + // Sample x% of premium compile projects to move up one bracket. + if (currentCompileBackendClass !== 'c2d') return null + if ( + SplitTestHandler.getPercentile(projectId, 'double-compile', 'release') >= + Settings.apis.clsi_new.sample + ) + return null + + let newCompileBackendClass + switch (currentCompileBackendClass) { + case 'n2d': + newCompileBackendClass = 'c2d' + break + case 'c2d': + newCompileBackendClass = 'c4d' + break + default: + throw new Error('unknown ?compileBackendClass') + } + url.searchParams.set('compileBackendClass', newCompileBackendClass) + const clsiServerId = await NewBackendCloudClsiCookieManager.promises.getServerId( projectId, userId, compileGroup, - compileBackendClass + newCompileBackendClass ) opts = { ...opts, @@ -440,18 +502,24 @@ async function _makeNewBackendRequest( // Some responses are empty. Ignore JSON parsing errors } timer.done() + let newClsiServerId if (CLSI_COOKIES_ENABLED) { - const newClsiServerId = _getClsiServerIdFromResponse(response) + newClsiServerId = _getClsiServerIdFromResponse(response) await NewBackendCloudClsiCookieManager.promises.setServerId( projectId, userId, compileGroup, - compileBackendClass, + newCompileBackendClass, newClsiServerId, clsiServerId ) } - return { response, body: json } + return { + response, + body: json, + newCompileBackendClass, + newClsiServerId: clsiServerId || newClsiServerId, + } } function _getCompilerUrl( diff --git a/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js b/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js index 082262c853..2fa3877dba 100644 --- a/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js +++ b/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js @@ -52,7 +52,7 @@ describe('ClsiCookieManager', function () { this.project_id, this.user_id, '', - 'e2' + 'n2d' ) this.redis.get .calledWith(`clsiserver:${this.project_id}:${this.user_id}`) @@ -84,7 +84,7 @@ describe('ClsiCookieManager', function () { this.project_id, this.user_id, '', - 'e2' + 'n2d' ) this.ClsiCookieManager.promises._populateServerIdViaRequest .calledWith(this.project_id, this.user_id) @@ -115,13 +115,13 @@ describe('ClsiCookieManager', function () { this.project_id, this.user_id, 'standard', - 'e2' + 'n2d' ) const args = this.ClsiCookieManager.promises.setServerId.args[0] args[0].should.equal(this.project_id) args[1].should.equal(this.user_id) args[2].should.equal('standard') - args[3].should.equal('e2') + args[3].should.equal('n2d') args[4].should.deep.equal(this.clsiServerId) }) @@ -131,7 +131,7 @@ describe('ClsiCookieManager', function () { this.project_id, this.user_id, '', - 'e2' + 'n2d' ) serverId.should.equal(this.clsiServerId) }) @@ -150,7 +150,7 @@ describe('ClsiCookieManager', function () { this.project_id, this.user_id, 'standard', - 'e2', + 'n2d', this.clsiServerId, null ) @@ -172,7 +172,7 @@ describe('ClsiCookieManager', function () { this.project_id, this.user_id, 'standard', - 'e2', + 'n2d', this.clsiServerId, null ) @@ -189,7 +189,7 @@ describe('ClsiCookieManager', function () { this.project_id, this.user_id, 'standard', - 'e2', + 'n2d', this.clsiServerId, null ) @@ -212,7 +212,7 @@ describe('ClsiCookieManager', function () { this.project_id, this.user_id, 'standard', - 'e2', + 'n2d', this.clsiServerId, null ) @@ -240,7 +240,7 @@ describe('ClsiCookieManager', function () { this.project_id, this.user_id, 'standard', - 'e2', + 'n2d', this.clsiServerId, null ) @@ -259,14 +259,14 @@ describe('ClsiCookieManager', function () { this.project_id, this.user_id, 'standard', - 'e2', + 'n2d', this.clsiServerId, 'previous-clsi-server-id' ) expect( this.fetchUtils.fetchStringWithResponse ).to.have.been.calledWith( - `${this.settings.apis.clsi.url}/instance-state?clsiserverid=previous-clsi-server-id&compileGroup=standard&compileBackendClass=e2`, + `${this.settings.apis.clsi.url}/instance-state?clsiserverid=previous-clsi-server-id&compileGroup=standard&compileBackendClass=n2d`, { method: 'GET', signal: sinon.match.instanceOf(AbortSignal) } ) } diff --git a/services/web/test/unit/src/Compile/ClsiManagerTests.js b/services/web/test/unit/src/Compile/ClsiManagerTests.js index 0b4a19adb9..ec49971d23 100644 --- a/services/web/test/unit/src/Compile/ClsiManagerTests.js +++ b/services/web/test/unit/src/Compile/ClsiManagerTests.js @@ -59,7 +59,21 @@ describe('ClsiManager', function () { this.newClsiServerId = 'newserver' this.rawOutputFiles = {} this.responseBody = { - compile: { status: 'success' }, + compile: { + status: 'success', + stats: { + isInitialCompile: 1, + restoredClsiCache: 1, + }, + timings: { compileE2E: 1337 }, + outputFiles: [ + { + path: 'output.pdf', + size: 42, + url: 'http://localhost:3013/snip/output.pdf', + }, + ], + }, } this.response = { ok: true, @@ -128,6 +142,7 @@ describe('ClsiManager', function () { }, inc: sinon.stub(), count: sinon.stub(), + histogram: sinon.stub(), } this.Settings = { apis: { @@ -139,8 +154,8 @@ describe('ClsiManager', function () { url: `http://${CLSI_HOST}`, submissionBackendClass: 'n2d', }, - clsi_priority: { - url: 'https://clsipremium.example.com', + clsi_new: { + sample: 100, }, }, enablePdfCaching: true, @@ -157,10 +172,17 @@ describe('ClsiManager', function () { return `${FILESTORE_URL}/history/project/${historyId}/hash/${hash}` }), } + this.SplitTestHandler = { + getPercentile: sinon.stub().returns(42), + } + this.AnalyticsManager = { + recordEventForUserInBackground: sinon.stub(), + } this.ClsiManager = SandboxedModule.require(MODULE_PATH, { requires: { '@overleaf/settings': this.Settings, + '../SplitTests/SplitTestHandler': this.SplitTestHandler, '../../models/Project': { Project: this.Project, }, @@ -175,6 +197,7 @@ describe('ClsiManager', function () { './ClsiFormatChecker': this.ClsiFormatChecker, '@overleaf/metrics': this.Metrics, '../History/HistoryManager': this.HistoryManager, + '../Analytics/AnalyticsManager': this.AnalyticsManager, }, }) }) @@ -214,7 +237,7 @@ describe('ClsiManager', function () { this.project._id, this.user_id, { - compileBackendClass: 'e2', + compileBackendClass: 'n2d', compileGroup: 'standard', timeout: this.timeout, } @@ -228,7 +251,7 @@ describe('ClsiManager', function () { url.host === CLSI_HOST && url.pathname === `/project/${this.project._id}/user/${this.user_id}/compile` && - url.searchParams.get('compileBackendClass') === 'e2' && + url.searchParams.get('compileBackendClass') === 'n2d' && url.searchParams.get('compileGroup') === 'standard' ), { @@ -308,7 +331,7 @@ describe('ClsiManager', function () { this.project._id, this.user_id, 'standard', - 'e2', + 'n2d', this.newClsiServerId ) }) @@ -351,7 +374,7 @@ describe('ClsiManager', function () { this.result = await this.ClsiManager.promises.sendRequest( this.project._id, this.user_id, - { compileBackendClass: 'e2', compileGroup: 'standard' } + { compileBackendClass: 'n2d', compileGroup: 'standard' } ) }) @@ -386,7 +409,7 @@ describe('ClsiManager', function () { { timeout: 100, incrementalCompilesEnabled: true, - compileBackendClass: 'e2', + compileBackendClass: 'n2d', compileGroup: 'priority', compileFromClsiCache: true, populateClsiCache: true, @@ -433,7 +456,7 @@ describe('ClsiManager', function () { url.hostname === CLSI_HOST && url.pathname === `/project/${this.project._id}/user/${this.user_id}/compile` && - url.searchParams.get('compileBackendClass') === 'e2' && + url.searchParams.get('compileBackendClass') === 'n2d' && url.searchParams.get('compileGroup') === 'priority' ), { @@ -790,8 +813,8 @@ describe('ClsiManager', function () { this.project._id, this.user_id, { - compileBackendClass: 'e2', - compileGroup: 'standard', + compileBackendClass: 'c2d', + compileGroup: 'priority', } ) // wait for the background task to finish @@ -806,18 +829,34 @@ describe('ClsiManager', function () { url.host === CLSI_HOST && url.pathname === `/project/${this.project._id}/user/${this.user_id}/compile` && - url.searchParams.get('compileBackendClass') === 'e2' && - url.searchParams.get('compileGroup') === 'standard' + url.searchParams.get('compileBackendClass') === 'c2d' && + url.searchParams.get('compileGroup') === 'priority' ) ) expect(this.FetchUtils.fetchStringWithResponse).to.have.been.calledWith( sinon.match( url => url.toString() === - `${this.Settings.apis.clsi_new.url}/project/${this.project._id}/user/${this.user_id}/compile?compileBackendClass=e2&compileGroup=standard` + `${this.Settings.apis.clsi_new.url}/project/${this.project._id}/user/${this.user_id}/compile?compileBackendClass=c4d&compileGroup=priority` ) ) }) + it('should record an event', function () { + expect( + this.AnalyticsManager.recordEventForUserInBackground + ).to.have.been.calledWith(this.user_id, 'double-compile-result', { + projectId: 'project-id', + compileBackendClass: 'c2d', + newCompileBackendClass: 'c4d', + status: 'success', + compileTime: 1337, + newCompileTime: 1337, + clsiServerId: 'newserver', + newClsiServerId: 'clsi-server-id', + pdfSize: 42, + newPdfSize: 42, + }) + }) }) }) @@ -852,7 +891,7 @@ describe('ClsiManager', function () { this.result = await this.ClsiManager.promises.sendExternalRequest( this.submissionId, this.clsiRequest, - { compileBackendClass: 'e2', compileGroup: 'standard' } + { compileBackendClass: 'n2d', compileGroup: 'standard' } ) }) @@ -862,7 +901,7 @@ describe('ClsiManager', function () { url => url.host === CLSI_HOST && url.pathname === `/project/${this.submissionId}/compile` && - url.searchParams.get('compileBackendClass') === 'e2' && + url.searchParams.get('compileBackendClass') === 'n2d' && url.searchParams.get('compileGroup') === 'standard' ), { @@ -927,7 +966,7 @@ describe('ClsiManager', function () { await this.ClsiManager.promises.deleteAuxFiles( this.project._id, this.user_id, - { compileBackendClass: 'e2', compileGroup: 'standard' }, + { compileBackendClass: 'n2d', compileGroup: 'standard' }, 'node-1' ) }) @@ -939,7 +978,7 @@ describe('ClsiManager', function () { url.host === CLSI_HOST && url.pathname === `/project/${this.project._id}/user/${this.user_id}` && - url.searchParams.get('compileBackendClass') === 'e2' && + url.searchParams.get('compileBackendClass') === 'n2d' && url.searchParams.get('compileGroup') === 'standard' && url.searchParams.get('clsiserverid') === 'node-1' ), @@ -977,7 +1016,7 @@ describe('ClsiManager', function () { await this.ClsiManager.promises.deleteAuxFiles( this.project._id, this.user_id, - { compileBackendClass: 'n2d', compileGroup: 'standard' }, + { compileBackendClass: 'c2d', compileGroup: 'priority' }, 'node-1' ) // wait for the background task to finish @@ -991,8 +1030,8 @@ describe('ClsiManager', function () { url.host === CLSI_HOST && url.pathname === `/project/${this.project._id}/user/${this.user_id}` && - url.searchParams.get('compileBackendClass') === 'n2d' && - url.searchParams.get('compileGroup') === 'standard' && + url.searchParams.get('compileBackendClass') === 'c2d' && + url.searchParams.get('compileGroup') === 'priority' && url.searchParams.get('clsiserverid') === 'node-1' ), { method: 'DELETE' } @@ -1003,8 +1042,8 @@ describe('ClsiManager', function () { url.host === 'compiles.somewhere.test' && url.pathname === `/project/${this.project._id}/user/${this.user_id}` && - url.searchParams.get('compileBackendClass') === 'n2d' && - url.searchParams.get('compileGroup') === 'standard' && + url.searchParams.get('compileBackendClass') === 'c4d' && + url.searchParams.get('compileGroup') === 'priority' && !url.searchParams.has('clsiserverid') ), sinon.match({ method: 'DELETE' }) @@ -1020,7 +1059,7 @@ describe('ClsiManager', function () { this.project._id, this.user_id, false, - { compileBackendClass: 'e2', compileGroup: 'standard' }, + { compileBackendClass: 'n2d', compileGroup: 'standard' }, 'node-1' ) }) @@ -1030,7 +1069,7 @@ describe('ClsiManager', function () { sinon.match( url => url.toString() === - `http://clsi.example.com/project/${this.project._id}/user/${this.user_id}/wordcount?compileBackendClass=e2&compileGroup=standard&file=main.tex&image=mock-image-name&clsiserverid=node-1` + `http://clsi.example.com/project/${this.project._id}/user/${this.user_id}/wordcount?compileBackendClass=n2d&compileGroup=standard&file=main.tex&image=mock-image-name&clsiserverid=node-1` ) ) }) @@ -1047,7 +1086,7 @@ describe('ClsiManager', function () { this.project._id, this.user_id, 'other.tex', - { compileBackendClass: 'e2', compileGroup: 'standard' }, + { compileBackendClass: 'n2d', compileGroup: 'standard' }, 'node-2' ) }) @@ -1059,7 +1098,7 @@ describe('ClsiManager', function () { url.host === CLSI_HOST && url.pathname === `/project/${this.project._id}/user/${this.user_id}/wordcount` && - url.searchParams.get('compileBackendClass') === 'e2' && + url.searchParams.get('compileBackendClass') === 'n2d' && url.searchParams.get('compileGroup') === 'standard' && url.searchParams.get('clsiserverid') === 'node-2' && url.searchParams.get('file') === 'other.tex' && @@ -1081,7 +1120,7 @@ describe('ClsiManager', function () { this.project._id, this.user_id, false, - { compileBackendClass: 'n2d', compileGroup: 'standard' }, + { compileBackendClass: 'c2d', compileGroup: 'priority' }, 'node-1' ) // wait for the background task to finish @@ -1093,14 +1132,14 @@ describe('ClsiManager', function () { sinon.match( url => url.toString() === - `http://clsi.example.com/project/${this.project._id}/user/${this.user_id}/wordcount?compileBackendClass=n2d&compileGroup=standard&file=main.tex&image=mock-image-name&clsiserverid=node-1` + `http://clsi.example.com/project/${this.project._id}/user/${this.user_id}/wordcount?compileBackendClass=c2d&compileGroup=priority&file=main.tex&image=mock-image-name&clsiserverid=node-1` ) ) expect(this.FetchUtils.fetchStringWithResponse).to.have.been.calledWith( sinon.match( url => url.toString() === - `${this.Settings.apis.clsi_new.url}/project/${this.project._id}/user/${this.user_id}/wordcount?compileBackendClass=n2d&compileGroup=standard&file=main.tex&image=mock-image-name` + `${this.Settings.apis.clsi_new.url}/project/${this.project._id}/user/${this.user_id}/wordcount?compileBackendClass=c4d&compileGroup=priority&file=main.tex&image=mock-image-name` ) ) }) diff --git a/services/web/test/unit/src/Compile/CompileControllerTests.js b/services/web/test/unit/src/Compile/CompileControllerTests.js index caf9a61a66..64b04f7299 100644 --- a/services/web/test/unit/src/Compile/CompileControllerTests.js +++ b/services/web/test/unit/src/Compile/CompileControllerTests.js @@ -712,7 +712,7 @@ describe('CompileController', function () { .stub() .resolves({ compileGroup: 'standard', - compileBackendClass: 'e2', + compileBackendClass: 'n2d', }) await this.CompileController._proxyToClsi( this.projectId, @@ -727,7 +727,7 @@ describe('CompileController', function () { it('should open a request to the CLSI', function () { this.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( - `${this.settings.apis.clsi.url}${this.url}?compileGroup=standard&compileBackendClass=e2&query=foo` + `${this.settings.apis.clsi.url}${this.url}?compileGroup=standard&compileBackendClass=n2d&query=foo` ) }) @@ -769,7 +769,7 @@ describe('CompileController', function () { .stub() .resolves({ compileGroup: 'standard', - compileBackendClass: 'e2', + compileBackendClass: 'n2d', }) await this.CompileController._proxyToClsi( this.projectId, @@ -784,7 +784,7 @@ describe('CompileController', function () { it('should open a request to the CLSI', function () { this.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( - `${this.settings.apis.clsi.url}${this.url}?compileGroup=standard&compileBackendClass=e2` + `${this.settings.apis.clsi.url}${this.url}?compileGroup=standard&compileBackendClass=n2d` ) }) @@ -800,7 +800,7 @@ describe('CompileController', function () { .stub() .resolves({ compileGroup: 'standard', - compileBackendClass: 'e2', + compileBackendClass: 'n2d', }) await this.CompileController._proxyToClsi( this.projectId, @@ -815,7 +815,7 @@ describe('CompileController', function () { it('should proxy to the standard url', function () { this.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( - `${this.settings.apis.clsi.url}${this.url}?compileGroup=standard&compileBackendClass=e2` + `${this.settings.apis.clsi.url}${this.url}?compileGroup=standard&compileBackendClass=n2d` ) }) }) @@ -826,7 +826,7 @@ describe('CompileController', function () { .stub() .resolves({ compileGroup: 'standard', - compileBackendClass: 'e2', + compileBackendClass: 'n2d', }) this.req.query = { build: 1234 } await this.CompileController._proxyToClsi( @@ -842,7 +842,7 @@ describe('CompileController', function () { it('should proxy to the standard url without the build parameter', function () { this.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( - `${this.settings.apis.clsi.url}${this.url}?compileGroup=standard&compileBackendClass=e2` + `${this.settings.apis.clsi.url}${this.url}?compileGroup=standard&compileBackendClass=n2d` ) }) })