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

@@ -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 =
+  const recordPerformanceMetrics = StatsManager.sampleRequest(
-    request.user_id != null &&
+    request,
-    Settings.performanceLogSamplingPercentage > 0 &&
+    Settings.performanceLogSamplingPercentage
-    StatsManager.sampleByHash(
+  )
-      request.user_id,
-      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
+    enableLatexMkMetrics(stats)
-    // 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,
-    })
   }
 
   try {

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 }

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 }

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

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
+    })
+  })
 })