From da3f366643f2312166025cbab31db100d6e0fd07 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 10 Oct 2025 09:08:35 +0100 Subject: [PATCH] Merge pull request #28959 from overleaf/bg-exclude-health-checks-from-performance-logs exclude health checks from performance logs GitOrigin-RevId: 88db63e00b32b2b015ee25c7d555546ed7d9a95b --- services/clsi/app/js/CompileManager.js | 20 ++--- services/clsi/app/js/LatexMetrics.js | 28 ++++++- services/clsi/app/js/StatsManager.js | 28 ++++++- .../clsi/test/unit/js/CompileManagerTests.js | 40 +++++++++ .../clsi/test/unit/js/StatsManagerTests.js | 82 ++++++++++++++++++- 5 files changed, 181 insertions(+), 17 deletions(-) diff --git a/services/clsi/app/js/CompileManager.js b/services/clsi/app/js/CompileManager.js index 2248623c52..179227c17a 100644 --- a/services/clsi/app/js/CompileManager.js +++ b/services/clsi/app/js/CompileManager.js @@ -24,6 +24,7 @@ const { downloadOutputDotSynctexFromCompileCache, } = require('./CLSICacheHandler') const StatsManager = require('./StatsManager') +const { enableLatexMkMetrics } = require('./LatexMetrics') const { callbackifyMultiResult } = require('@overleaf/promise-utils') const COMPILE_TIME_BUCKETS = [ @@ -193,23 +194,14 @@ async function doCompile(request, stats, timings) { const compileName = getCompileName(request.project_id, request.user_id) // Record latexmk -time stats for a subset of users - const recordPerformanceMetrics = - request.user_id != null && - Settings.performanceLogSamplingPercentage > 0 && - StatsManager.sampleByHash( - request.user_id, - Settings.performanceLogSamplingPercentage - ) + const recordPerformanceMetrics = StatsManager.sampleRequest( + request, + Settings.performanceLogSamplingPercentage + ) // For selected users, define a `latexmk` property on the stats object // to collect latexmk -time stats. if (recordPerformanceMetrics) { - // To prevent any changes to the existing compile responses being sent - // to web, exclude latexmk stats from being exported by marking them - // non-enumerable. This prevents them being serialised by JSON.stringify(). - Object.defineProperty(stats, 'latexmk', { - value: {}, - enumerable: false, - }) + enableLatexMkMetrics(stats) } try { diff --git a/services/clsi/app/js/LatexMetrics.js b/services/clsi/app/js/LatexMetrics.js index e24fc96528..7837999aaa 100644 --- a/services/clsi/app/js/LatexMetrics.js +++ b/services/clsi/app/js/LatexMetrics.js @@ -97,6 +97,15 @@ const LATEX_MK_METRICS = [ ], ] +/** + * Parses latexmk stdout for metrics and adds them to the stats object. + * It iterates through a predefined list of metric matchers (LATEX_MK_METRICS), + * applies them to the stdout, and adds any successful matches to the + * `stats.latexmk` object. + * + * @param {{stdout?: string}} output - The output from the latexmk process. + * @param {{latexmk: object}} stats - The statistics object to update. This object is mutated. + */ function addLatexMkMetrics(output, stats) { for (const [stat, matcher] of LATEX_MK_METRICS) { const match = matcher(output?.stdout || '', stats.latexmk) @@ -106,4 +115,21 @@ function addLatexMkMetrics(output, stats) { } } -module.exports = { addLatexMkMetrics } +/** + * Adds a non-enumerable `latexmk` property to the stats object. + * + * This property is used to store statistics from the latexmk compilation + * process. It is made non-enumerable to prevent it from being serialized by + * `JSON.stringify()`, which means it is not sent in the compile response + * to web where it would added to the analytics events. + * + * @param {object} stats - The compile stats object to be modified. + */ +function enableLatexMkMetrics(stats) { + Object.defineProperty(stats, 'latexmk', { + value: {}, + enumerable: false, + }) +} + +module.exports = { enableLatexMkMetrics, addLatexMkMetrics } diff --git a/services/clsi/app/js/StatsManager.js b/services/clsi/app/js/StatsManager.js index 9d46b6bbb1..82ac3d5fa9 100644 --- a/services/clsi/app/js/StatsManager.js +++ b/services/clsi/app/js/StatsManager.js @@ -21,4 +21,30 @@ function sampleByHash(key, samplePercentage) { return percentile < samplePercentage } -module.exports = { sampleByHash } +const EXCLUDED_METRICS_OPTS_PATHS = new Set(['health-check', 'clsi-perf']) + +/** + * Determines whether a given request should be sampled based on user ID and sampling percentage. + * The request will not be sampled if it lacks a user_id, if its metrics path is in the exclusion list, + * or if the sampling percentage is zero or less. + * + * @param {object} request - The request object to check. + * @param {string} [request.user_id] - The ID of the user making the request. + * @param {object} [request.metricsOpts] - Metrics options for the request. + * @param {string} [request.metricsOpts.path] - The path associated with the request metrics. + * @param {number} samplingPercentage - The percentage of requests to sample (e.g., 10 for 10%). + * @returns {boolean|undefined} The result from sampleByHash if the request is eligible for sampling, otherwise undefined. + */ +function sampleRequest(request, samplingPercentage) { + if (!request.user_id) { + return + } + if (EXCLUDED_METRICS_OPTS_PATHS.has(request.metricsOpts?.path)) { + return + } + if (samplingPercentage > 0) { + return sampleByHash(request.user_id, samplingPercentage) + } +} + +module.exports = { sampleByHash, sampleRequest } diff --git a/services/clsi/test/unit/js/CompileManagerTests.js b/services/clsi/test/unit/js/CompileManagerTests.js index 30ef538ac3..fb93cbb478 100644 --- a/services/clsi/test/unit/js/CompileManagerTests.js +++ b/services/clsi/test/unit/js/CompileManagerTests.js @@ -150,6 +150,10 @@ describe('CompileManager', function () { downloadOutputDotSynctexFromCompileCache: sinon.stub().resolves(), } + this.LatexMetrics = { enableLatexMkMetrics: sinon.stub() } + + this.StatsManager = { sampleRequest: sinon.stub().returns(false) } + this.CompileManager = SandboxedModule.require(MODULE_PATH, { requires: { './LatexRunner': this.LatexRunner, @@ -171,6 +175,8 @@ describe('CompileManager', function () { './SynctexOutputParser': this.SynctexOutputParser, 'fs/promises': this.fsPromises, './CLSICacheHandler': this.CLSICacheHandler, + './LatexMetrics': this.LatexMetrics, + './StatsManager': this.StatsManager, }, }) }) @@ -275,6 +281,40 @@ describe('CompileManager', function () { }) }) + describe('with performance metric collection', function () { + it('should enable latexmk metrics when sampleRequest returns true', async function () { + this.StatsManager.sampleRequest.returns(true) + await this.CompileManager.promises.doCompileWithLock( + this.request, + {}, + {} + ) + expect(this.LatexMetrics.enableLatexMkMetrics).to.have.been.calledWith( + sinon.match.object + ) + }) + + it('should not enable latexmk metrics when sampleRequest returns false', async function () { + this.StatsManager.sampleRequest.returns(false) + await this.CompileManager.promises.doCompileWithLock( + this.request, + {}, + {} + ) + expect(this.LatexMetrics.enableLatexMkMetrics).not.to.have.been.called + }) + + it('should not enable latexmk metrics when sampleRequest returns undefined', async function () { + this.StatsManager.sampleRequest.returns(undefined) + await this.CompileManager.promises.doCompileWithLock( + this.request, + {}, + {} + ) + expect(this.LatexMetrics.enableLatexMkMetrics).not.to.have.been.called + }) + }) + describe('with draft mode', function () { beforeEach(async function () { this.request.draft = true diff --git a/services/clsi/test/unit/js/StatsManagerTests.js b/services/clsi/test/unit/js/StatsManagerTests.js index 7fb520fe94..ec262fedc8 100644 --- a/services/clsi/test/unit/js/StatsManagerTests.js +++ b/services/clsi/test/unit/js/StatsManagerTests.js @@ -1,5 +1,5 @@ const { expect } = require('chai') -const { sampleByHash } = require('../../../app/js/StatsManager') +const { sampleByHash, sampleRequest } = require('../../../app/js/StatsManager') describe('StatsManager', function () { describe('sampleByHash', function () { @@ -82,4 +82,84 @@ describe('StatsManager', function () { } }) }) + + describe('sampleRequest', function () { + it('should return undefined if there is no user_id', function () { + const request = {} + const percentage = 50 + expect(sampleRequest(request, percentage)).to.be.undefined + }) + + it('should return undefined if the path is health-check', function () { + const request = { + user_id: 'some-user', + metricsOpts: { path: 'health-check' }, + } + const percentage = 100 + expect(sampleRequest(request, percentage)).to.be.undefined + }) + + it('should return undefined if the path is clsi-perf', function () { + const request = { + user_id: 'some-user', + metricsOpts: { path: 'clsi-perf' }, + } + const percentage = 100 + expect(sampleRequest(request, percentage)).to.be.undefined + }) + + it('should return undefined for a health-check even if the user would be sampled', function () { + const request = { + user_id: 'test-key-in', // percentile 13 + metricsOpts: { path: 'health-check' }, + } + const percentage = 40 + expect(sampleRequest(request, percentage)).to.be.undefined + }) + + it('should return undefined for clsi-perf even if the user would be sampled', function () { + const request = { + user_id: 'test-key-in', // percentile 13 + metricsOpts: { path: 'clsi-perf' }, + } + const percentage = 40 + expect(sampleRequest(request, percentage)).to.be.undefined + }) + + it('should return undefined if the sampling percentage is 0', function () { + const request = { user_id: 'some-user' } + const percentage = 0 + expect(sampleRequest(request, percentage)).to.be.undefined + }) + + it('should return undefined if the sampling percentage is negative', function () { + const request = { user_id: 'some-user' } + const percentage = -10 + expect(sampleRequest(request, percentage)).to.be.undefined + }) + + it('should sample if there are no metricsOpts', function () { + const request = { user_id: 'test-key-in' } // percentile 13 + const percentage = 40 + expect(sampleRequest(request, percentage)).to.be.true + }) + + it('should sample if metricsOpts has no path', function () { + const request = { user_id: 'test-key-in', metricsOpts: {} } // percentile 13 + const percentage = 40 + expect(sampleRequest(request, percentage)).to.be.true + }) + + it('should return true for a request that should be sampled', function () { + const request = { user_id: 'test-key-in' } // percentile 13 + const percentage = 40 + expect(sampleRequest(request, percentage)).to.be.true + }) + + it('should return false for a request that should not be sampled', function () { + const request = { user_id: 'test-key-outer' } // percentile 47 + const percentage = 40 + expect(sampleRequest(request, percentage)).to.be.false + }) + }) })