Browse Source

[web] add metrics for mongo access in split test system (#32920)

GitOrigin-RevId: cd93401bace60c003a63914e2898cf1f0defdabc
Jakob Ackermann 3 months ago
parent
commit
4ce5620b1d

+ 1 - 1
libraries/metrics/http.js

@@ -75,4 +75,4 @@ class RequestLogger {
   }
 }
 
-module.exports = { monitor, RequestLogger }
+module.exports = { monitor, RequestLogger, getRoutePath }

+ 13 - 3
services/web/app/src/Features/Analytics/AnalyticsManager.mjs

@@ -65,7 +65,10 @@ async function recordEventForUser(userId, event, segmentation) {
   if (_isAnalyticsDisabled() || _isSmokeTestUser(userId)) {
     return
   }
-  const analyticsId = await UserAnalyticsIdCache.get(userId)
+  const analyticsId = await UserAnalyticsIdCache.getWithMetrics(
+    userId,
+    `recordEventForUser:${event}`
+  )
   if (analyticsId) {
     _recordEvent({ analyticsId, userId, event, segmentation, isLoggedIn: true })
   }
@@ -113,7 +116,10 @@ async function setUserPropertyForUser(userId, propertyName, propertyValue) {
 
   _checkPropertyValue(propertyValue)
 
-  const analyticsId = await UserAnalyticsIdCache.get(userId)
+  const analyticsId = await UserAnalyticsIdCache.getWithMetrics(
+    userId,
+    `setUserPropertyForUser:${propertyName}`
+  )
   if (analyticsId) {
     await _setUserProperty({ analyticsId, propertyName, propertyValue })
   }
@@ -447,7 +453,11 @@ async function analyticsIdMiddleware(req, res, next) {
   const sessionUser = SessionManager.getSessionUser(session)
 
   if (sessionUser) {
-    session.analyticsId = await UserAnalyticsIdCache.get(sessionUser._id)
+    session.analyticsId = await UserAnalyticsIdCache.getWithMetrics(
+      sessionUser._id,
+      // Do not drill down further, this middleware is on all endpoints.
+      'analyticsIdMiddleware'
+    )
   } else if (!session.analyticsId) {
     // generate an `analyticsId` if needed
     session.analyticsId = crypto.randomUUID()

+ 14 - 0
services/web/app/src/Features/Analytics/UserAnalyticsIdCache.mjs

@@ -1,6 +1,7 @@
 import UserGetter from '../User/UserGetter.mjs'
 import { CacheLoader } from 'cache-flow'
 import { callbackify } from 'node:util'
+import Metrics from '@overleaf/metrics'
 
 class UserAnalyticsIdCache extends CacheLoader {
   constructor() {
@@ -22,6 +23,19 @@ class UserAnalyticsIdCache extends CacheLoader {
       return userId.toString()
     }
   }
+
+  get() {
+    throw new Error('use UserAnalyticsIdCache.getWithMetrics')
+  }
+
+  async getWithMetrics(userId, path) {
+    const { value, cached } = await this.getWithMetadata(userId)
+    Metrics.inc('user_analytics_id_cache', 1, {
+      status: cached ? 'hit' : 'miss',
+      path,
+    })
+    return value
+  }
 }
 
 const userAnalyticsIdCache = new UserAnalyticsIdCache()

+ 4 - 1
services/web/app/src/Features/Compile/CompileManager.mjs

@@ -138,7 +138,10 @@ async function _getUserCompileLimits(userId) {
     ownerFeatures.compileGroup = 'alpha'
   }
 
-  const analyticsId = await UserAnalyticsIdCache.get(owner._id)
+  const analyticsId = await UserAnalyticsIdCache.getWithMetrics(
+    owner._id,
+    '_getUserCompileLimits'
+  )
 
   const compileGroup =
     ownerFeatures.compileGroup || Settings.defaultFeatures.compileGroup

+ 20 - 4
services/web/app/src/Features/SplitTests/SplitTestHandler.mjs

@@ -120,7 +120,10 @@ async function getAssignmentForUser(
       return _getNonSaasAssignment(splitTestName)
     }
 
-    const analyticsId = await UserAnalyticsIdCache.get(userId)
+    const analyticsId = await UserAnalyticsIdCache.getWithMetrics(
+      userId,
+      `getAssignmentForUser:${splitTestName}`
+    )
     return _getAssignment(splitTestName, { analyticsId, userId, sync })
   } catch (error) {
     logger.error({ err: error }, 'Failed to get split test assignment for user')
@@ -227,7 +230,11 @@ async function getActiveAssignmentsForUser(
     return {}
   }
 
-  const user = await SplitTestUserGetter.promises.getUser(userId)
+  const user = await SplitTestUserGetter.promises.getUser(
+    userId,
+    null,
+    'getActiveAssignmentsForUser'
+  )
   if (user == null) {
     return {}
   }
@@ -402,7 +409,11 @@ async function _getAssignment(
   user =
     user ||
     (userId &&
-      (await SplitTestUserGetter.promises.getUser(userId, splitTestName)))
+      (await SplitTestUserGetter.promises.getUser(
+        userId,
+        splitTestName,
+        `_getAssignment:${splitTestName}`
+      )))
   const metadata = await _getAssignmentMetadata(analyticsId, user, splitTest)
   const { activeForUser, selectedVariantName, phase, versionNumber } = metadata
 
@@ -651,7 +662,12 @@ async function _recordAssignment({
     assignedAt: new Date(),
   }
   user =
-    user || (await SplitTestUserGetter.promises.getUser(userId, splitTestName))
+    user ||
+    (await SplitTestUserGetter.promises.getUser(
+      userId,
+      splitTestName,
+      `_recordAssignment:${splitTestName}`
+    ))
   if (user) {
     const assignedSplitTests = user.splitTests || []
     const assignmentLog = assignedSplitTests[splitTestName] || []

+ 7 - 1
services/web/app/src/Features/SplitTests/SplitTestSessionHandler.mjs

@@ -164,7 +164,13 @@ async function sessionMaintenance(req, user) {
 
   Metrics.inc('split_test_session_maintenance', 1, { status: 'start' })
   if (sessionUser) {
-    user = user || (await SplitTestUserGetter.promises.getUser(sessionUser._id))
+    user =
+      user ||
+      (await SplitTestUserGetter.promises.getUser(
+        sessionUser._id,
+        null,
+        `sessionMaintenance:${Metrics.http.getRoutePath(req)}`
+      ))
     if (
       Boolean(sessionUser.alphaProgram) !== Boolean(user.alphaProgram) ||
       Boolean(sessionUser.betaProgram) !== Boolean(user.betaProgram)

+ 2 - 1
services/web/app/src/Features/SplitTests/SplitTestUserGetter.mjs

@@ -2,7 +2,8 @@ import { callbackify } from 'node:util'
 import Metrics from '@overleaf/metrics'
 import UserGetter from '../User/UserGetter.mjs'
 
-async function getUser(id, splitTestName) {
+async function getUser(id, splitTestName, path) {
+  Metrics.inc('split_test_get_user', 1, { path })
   const projection = {
     analyticsId: 1,
     alphaProgram: 1,

+ 4 - 1
services/web/scripts/stripe/finalize-stripe-subscription-migration.mjs

@@ -437,7 +437,10 @@ async function processMigration(input, commit) {
   }
 
   // 7. If commit mode, perform migration
-  const analyticsId = await UserAnalyticsIdCache.get(overleafUserId)
+  const analyticsId = await UserAnalyticsIdCache.getWithMetrics(
+    overleafUserId,
+    'script' // no-op, metrics are not collected from scripts.
+  )
   const mongoUser = await User.findOne({
     _id: overleafUserId,
   }).exec()

+ 3 - 0
services/web/test/unit/bootstrap.mjs

@@ -58,6 +58,9 @@ vi.mock('@overleaf/metrics', () => {
           return 1
         }
       },
+      http: {
+        getRoutePath: sinon.stub().returns('project_Project_id'),
+      },
       prom: { Counter: sinon.stub(), Histogram: sinon.stub() },
       mongodb: { monitor: sinon.stub() },
     },

+ 5 - 5
services/web/test/unit/src/Analytics/AnalyticsManager.test.mjs

@@ -92,7 +92,7 @@ describe('AnalyticsManager', function () {
       '../../../../app/src/Features/Analytics/UserAnalyticsIdCache',
       () => ({
         default: (ctx.UserAnalyticsIdCache = {
-          get: sinon.stub().resolves(ctx.analyticsId),
+          getWithMetrics: sinon.stub().resolves(ctx.analyticsId),
         }),
       })
     )
@@ -391,7 +391,7 @@ describe('AnalyticsManager', function () {
         '../../../../app/src/Features/Analytics/UserAnalyticsIdCache',
         () => ({
           default: (ctx.UserAnalyticsIdCache = {
-            get: sinon.stub().resolves(ctx.analyticsId),
+            getWithMetrics: sinon.stub().resolves(ctx.analyticsId),
           }),
         })
       )
@@ -439,7 +439,7 @@ describe('AnalyticsManager', function () {
     })
 
     it('sets session.analyticsId with a legacy user session without an analyticsId', async function (ctx) {
-      ctx.UserAnalyticsIdCache.get.resolves(ctx.userId)
+      ctx.UserAnalyticsIdCache.getWithMetrics.resolves(ctx.userId)
       ctx.req.session.user = {
         _id: ctx.userId,
         analyticsId: undefined,
@@ -450,7 +450,7 @@ describe('AnalyticsManager', function () {
     })
 
     it('updates session.analyticsId with a legacy user session without an analyticsId if different', async function (ctx) {
-      ctx.UserAnalyticsIdCache.get.resolves(ctx.userId)
+      ctx.UserAnalyticsIdCache.getWithMetrics.resolves(ctx.userId)
       ctx.req.session.user = {
         _id: ctx.userId,
         analyticsId: undefined,
@@ -462,7 +462,7 @@ describe('AnalyticsManager', function () {
     })
 
     it('does not update session.analyticsId with a legacy user session without an analyticsId if same', async function (ctx) {
-      ctx.UserAnalyticsIdCache.get.resolves(ctx.userId)
+      ctx.UserAnalyticsIdCache.getWithMetrics.resolves(ctx.userId)
       ctx.req.session.user = {
         _id: ctx.userId,
         analyticsId: undefined,

+ 1 - 1
services/web/test/unit/src/Compile/CompileManager.test.mjs

@@ -68,7 +68,7 @@ describe('CompileManager', function () {
       '../../../../app/src/Features/Analytics/UserAnalyticsIdCache',
       () => ({
         default: (ctx.UserAnalyticsIdCache = {
-          get: sinon.stub().resolves('abc'),
+          getWithMetrics: sinon.stub().resolves('abc'),
         }),
       })
     )