Merge pull request #17731 from overleaf/em-promisify-error-recorder

Promisify ErrorRecorder

GitOrigin-RevId: 3736567272a09b4e3b9075118460392c1f66f0d7

services/project-history/app/js/ErrorRecorder.js

@@ -1,32 +1,9 @@
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-import { promisify } from 'util'
+import { callbackify } from 'util'
 import logger from '@overleaf/logger'
 import metrics from '@overleaf/metrics'
-import OError from '@overleaf/o-error'
 import { db } from './mongodb.js'
 
-export function record(projectId, queueSize, error, callback) {
-  if (callback == null) {
-    callback = function () {}
-  }
-  const _callback = function (mongoError) {
-    if (mongoError != null) {
-      logger.error(
-        { projectId, mongoError },
-        'failed to change project statues in mongo'
-      )
-    }
-    return callback(error || null, queueSize)
-  }
-
+async function record(projectId, queueSize, error) {
   if (error != null) {
     const errorRecord = {
       queueSize,
@@ -38,69 +15,62 @@ export function record(projectId, queueSize, error, callback) {
       { projectId, errorRecord },
       'recording failed attempt to process updates'
     )
-    return db.projectHistoryFailures.updateOne(
-      {
-        project_id: projectId,
-      },
-      {
-        $set: errorRecord,
-        $inc: {
-          attempts: 1,
+    try {
+      await db.projectHistoryFailures.updateOne(
+        { project_id: projectId },
+        {
+          $set: errorRecord,
+          $inc: { attempts: 1 },
+          $push: {
+            history: {
+              $each: [errorRecord],
+              $position: 0,
+              $slice: 10,
+            },
+          }, // only keep recent failures
         },
-        $push: {
-          history: {
-            $each: [errorRecord],
-            $position: 0,
-            $slice: 10,
-          },
-        }, // only keep recent failures
-      },
-      {
-        upsert: true,
-      },
-      _callback
-    )
+        { upsert: true }
+      )
+    } catch (mongoError) {
+      logger.error(
+        { projectId, mongoError },
+        'failed to change project statues in mongo'
+      )
+    }
+    throw error
   } else {
-    return db.projectHistoryFailures.deleteOne(
-      { project_id: projectId },
-      _callback
-    )
+    try {
+      await db.projectHistoryFailures.deleteOne({ project_id: projectId })
+    } catch (mongoError) {
+      logger.error(
+        { projectId, mongoError },
+        'failed to change project statues in mongo'
+      )
+    }
+    return queueSize
   }
 }
 
-export function setForceDebug(projectId, state, callback) {
+async function setForceDebug(projectId, state) {
   if (state == null) {
     state = true
   }
-  if (callback == null) {
-    callback = function () {}
-  }
   logger.debug({ projectId, state }, 'setting forceDebug state for project')
-  return db.projectHistoryFailures.updateOne(
+  await db.projectHistoryFailures.updateOne(
     { project_id: projectId },
     { $set: { forceDebug: state } },
-    { upsert: true },
-    callback
+    { upsert: true }
   )
 }
 
 // we only record the sync start time, and not the end time, because the
 // record should be cleared on success.
-export function recordSyncStart(projectId, callback) {
-  if (callback == null) {
-    callback = function () {}
-  }
-  return db.projectHistoryFailures.updateOne(
-    {
-      project_id: projectId,
-    },
+async function recordSyncStart(projectId) {
+  await db.projectHistoryFailures.updateOne(
+    { project_id: projectId },
     {
-      $currentDate: {
-        resyncStartedAt: true,
-      },
-      $inc: {
-        resyncAttempts: 1,
-      },
+      $currentDate: { resyncStartedAt: true },
+      $inc: { resyncAttempts: 1 },
       $push: {
         history: {
           $each: [{ resyncStartedAt: new Date() }],
@@ -109,207 +79,176 @@ export function recordSyncStart(projectId, callback) {
         },
       },
     },
-    {
-      upsert: true,
-    },
-    callback
+    { upsert: true }
   )
 }
 
-export function getFailureRecord(projectId, callback) {
-  if (callback == null) {
-    callback = function () {}
-  }
-  return db.projectHistoryFailures.findOne({ project_id: projectId }, callback)
+async function getFailureRecord(projectId) {
+  return await db.projectHistoryFailures.findOne({ project_id: projectId })
 }
 
-export function getLastFailure(projectId, callback) {
-  if (callback == null) {
-    callback = function () {}
-  }
-  return db.projectHistoryFailures.findOneAndUpdate(
+async function getLastFailure(projectId) {
+  const result = await db.projectHistoryFailures.findOneAndUpdate(
     { project_id: projectId },
     { $inc: { requestCount: 1 } }, // increment the request count every time we check the last failure
-    { projection: { error: 1, ts: 1 } },
-    (err, result) => callback(err, result && result.value)
+    { projection: { error: 1, ts: 1 } }
   )
+  return result && result.value
 }
 
-export function getFailedProjects(callback) {
-  if (callback == null) {
-    callback = function () {}
-  }
-  return db.projectHistoryFailures.find({}).toArray(function (error, results) {
-    if (error != null) {
-      return callback(OError.tag(error))
-    }
-    return callback(null, results)
-  })
+async function getFailedProjects() {
+  return await db.projectHistoryFailures.find({}).toArray()
 }
 
-export function getFailuresByType(callback) {
-  if (callback == null) {
-    callback = function () {}
-  }
-  db.projectHistoryFailures.find({}).toArray(function (error, results) {
-    if (error != null) {
-      return callback(OError.tag(error))
+async function getFailuresByType() {
+  const results = await db.projectHistoryFailures.find({}).toArray()
+  const failureCounts = {}
+  const failureAttempts = {}
+  const failureRequests = {}
+  const maxQueueSize = {}
+  // count all the failures and number of attempts by type
+  for (const result of results || []) {
+    const failureType = result.error
+    const attempts = result.attempts || 1 // allow for field to be absent
+    const requests = result.requestCount || 0
+    const queueSize = result.queueSize || 0
+    if (failureCounts[failureType] > 0) {
+      failureCounts[failureType]++
+      failureAttempts[failureType] += attempts
+      failureRequests[failureType] += requests
+      maxQueueSize[failureType] = Math.max(queueSize, maxQueueSize[failureType])
+    } else {
+      failureCounts[failureType] = 1
+      failureAttempts[failureType] = attempts
+      failureRequests[failureType] = requests
+      maxQueueSize[failureType] = queueSize
     }
-    const failureCounts = {}
-    const failureAttempts = {}
-    const failureRequests = {}
-    const maxQueueSize = {}
-    // count all the failures and number of attempts by type
-    for (const result of Array.from(results || [])) {
-      const failureType = result.error
-      const attempts = result.attempts || 1 // allow for field to be absent
-      const requests = result.requestCount || 0
-      const queueSize = result.queueSize || 0
-      if (failureCounts[failureType] > 0) {
-        failureCounts[failureType]++
-        failureAttempts[failureType] += attempts
-        failureRequests[failureType] += requests
-        maxQueueSize[failureType] = Math.max(
-          queueSize,
-          maxQueueSize[failureType]
-        )
-      } else {
-        failureCounts[failureType] = 1
-        failureAttempts[failureType] = attempts
-        failureRequests[failureType] = requests
-        maxQueueSize[failureType] = queueSize
-      }
-    }
-    return callback(
-      null,
-      failureCounts,
-      failureAttempts,
-      failureRequests,
-      maxQueueSize
-    )
-  })
+  }
+
+  return { failureCounts, failureAttempts, failureRequests, maxQueueSize }
 }
 
-export function getFailures(callback) {
-  if (callback == null) {
-    callback = function () {}
+async function getFailures() {
+  const { failureCounts, failureAttempts, failureRequests, maxQueueSize } =
+    await getFailuresByType()
+
+  let attempts, failureType, label, requests
+  const shortNames = {
+    'Error: bad response from filestore: 404': 'filestore-404',
+    'Error: bad response from filestore: 500': 'filestore-500',
+    'NotFoundError: got a 404 from web api': 'web-api-404',
+    'Error: history store a non-success status code: 413': 'history-store-413',
+    'Error: history store a non-success status code: 422': 'history-store-422',
+    'Error: history store a non-success status code: 500': 'history-store-500',
+    'Error: history store a non-success status code: 503': 'history-store-503',
+    'Error: web returned a non-success status code: 500 (attempts: 2)':
+      'web-500',
+    'Error: ESOCKETTIMEDOUT': 'socket-timeout',
+    'Error: no project found': 'no-project-found',
+    'OpsOutOfOrderError: project structure version out of order on incoming updates':
+      'incoming-project-version-out-of-order',
+    'OpsOutOfOrderError: doc version out of order on incoming updates':
+      'incoming-doc-version-out-of-order',
+    'OpsOutOfOrderError: project structure version out of order':
+      'chunk-project-version-out-of-order',
+    'OpsOutOfOrderError: doc version out of order':
+      'chunk-doc-version-out-of-order',
+    'Error: failed to extend lock': 'lock-overrun',
+    'Error: tried to release timed out lock': 'lock-overrun',
+    'Error: Timeout': 'lock-overrun',
+    'Error: sync ongoing': 'sync-ongoing',
+    'SyncError: unexpected resyncProjectStructure update': 'sync-error',
+    '[object Error]': 'unknown-error-object',
+    'UpdateWithUnknownFormatError: update with unknown format':
+      'unknown-format',
+    'Error: update with unknown format': 'unknown-format',
+    'TextOperationError: The base length of the second operation has to be the target length of the first operation':
+      'text-op-error',
+    'Error: ENOSPC: no space left on device, write': 'ENOSPC',
+    '*': 'other',
   }
-  return getFailuresByType(
-    function (
-      error,
-      failureCounts,
-      failureAttempts,
-      failureRequests,
-      maxQueueSize
-    ) {
-      let attempts, failureType, label, requests
-      if (error != null) {
-        return callback(OError.tag(error))
-      }
-
-      const shortNames = {
-        'Error: bad response from filestore: 404': 'filestore-404',
-        'Error: bad response from filestore: 500': 'filestore-500',
-        'NotFoundError: got a 404 from web api': 'web-api-404',
-        'Error: history store a non-success status code: 413':
-          'history-store-413',
-        'Error: history store a non-success status code: 422':
-          'history-store-422',
-        'Error: history store a non-success status code: 500':
-          'history-store-500',
-        'Error: history store a non-success status code: 503':
-          'history-store-503',
-        'Error: web returned a non-success status code: 500 (attempts: 2)':
-          'web-500',
-        'Error: ESOCKETTIMEDOUT': 'socket-timeout',
-        'Error: no project found': 'no-project-found',
-        'OpsOutOfOrderError: project structure version out of order on incoming updates':
-          'incoming-project-version-out-of-order',
-        'OpsOutOfOrderError: doc version out of order on incoming updates':
-          'incoming-doc-version-out-of-order',
-        'OpsOutOfOrderError: project structure version out of order':
-          'chunk-project-version-out-of-order',
-        'OpsOutOfOrderError: doc version out of order':
-          'chunk-doc-version-out-of-order',
-        'Error: failed to extend lock': 'lock-overrun',
-        'Error: tried to release timed out lock': 'lock-overrun',
-        'Error: Timeout': 'lock-overrun',
-        'Error: sync ongoing': 'sync-ongoing',
-        'SyncError: unexpected resyncProjectStructure update': 'sync-error',
-        '[object Error]': 'unknown-error-object',
-        'UpdateWithUnknownFormatError: update with unknown format':
-          'unknown-format',
-        'Error: update with unknown format': 'unknown-format',
-        'TextOperationError: The base length of the second operation has to be the target length of the first operation':
-          'text-op-error',
-        'Error: ENOSPC: no space left on device, write': 'ENOSPC',
-        '*': 'other',
-      }
 
-      // set all the known errors to zero if not present (otherwise gauges stay on their last value)
-      const summaryCounts = {}
-      const summaryAttempts = {}
-      const summaryRequests = {}
-      const summaryMaxQueueSize = {}
+  // set all the known errors to zero if not present (otherwise gauges stay on their last value)
+  const summaryCounts = {}
+  const summaryAttempts = {}
+  const summaryRequests = {}
+  const summaryMaxQueueSize = {}
+
+  for (failureType in shortNames) {
+    label = shortNames[failureType]
+    summaryCounts[label] = 0
+    summaryAttempts[label] = 0
+    summaryRequests[label] = 0
+    summaryMaxQueueSize[label] = 0
+  }
 
-      for (failureType in shortNames) {
-        label = shortNames[failureType]
-        summaryCounts[label] = 0
-        summaryAttempts[label] = 0
-        summaryRequests[label] = 0
-        summaryMaxQueueSize[label] = 0
-      }
+  // record a metric for each type of failure
+  for (failureType in failureCounts) {
+    const failureCount = failureCounts[failureType]
+    label = shortNames[failureType] || shortNames['*']
+    summaryCounts[label] += failureCount
+    summaryAttempts[label] += failureAttempts[failureType]
+    summaryRequests[label] += failureRequests[failureType]
+    summaryMaxQueueSize[label] = Math.max(
+      maxQueueSize[failureType],
+      summaryMaxQueueSize[label]
+    )
+  }
 
-      // record a metric for each type of failure
-      for (failureType in failureCounts) {
-        const failureCount = failureCounts[failureType]
-        label = shortNames[failureType] || shortNames['*']
-        summaryCounts[label] += failureCount
-        summaryAttempts[label] += failureAttempts[failureType]
-        summaryRequests[label] += failureRequests[failureType]
-        summaryMaxQueueSize[label] = Math.max(
-          maxQueueSize[failureType],
-          summaryMaxQueueSize[label]
-        )
-      }
+  for (label in summaryCounts) {
+    const count = summaryCounts[label]
+    metrics.globalGauge('failed', count, 1, { status: label })
+  }
 
-      for (label in summaryCounts) {
-        const count = summaryCounts[label]
-        metrics.globalGauge('failed', count, 1, { status: label })
-      }
+  for (label in summaryAttempts) {
+    attempts = summaryAttempts[label]
+    metrics.globalGauge('attempts', attempts, 1, { status: label })
+  }
 
-      for (label in summaryAttempts) {
-        attempts = summaryAttempts[label]
-        metrics.globalGauge('attempts', attempts, 1, { status: label })
-      }
+  for (label in summaryRequests) {
+    requests = summaryRequests[label]
+    metrics.globalGauge('requests', requests, 1, { status: label })
+  }
 
-      for (label in summaryRequests) {
-        requests = summaryRequests[label]
-        metrics.globalGauge('requests', requests, 1, { status: label })
-      }
+  for (label in summaryMaxQueueSize) {
+    const queueSize = summaryMaxQueueSize[label]
+    metrics.globalGauge('max-queue-size', queueSize, 1, { status: label })
+  }
 
-      for (label in summaryMaxQueueSize) {
-        const queueSize = summaryMaxQueueSize[label]
-        metrics.globalGauge('max-queue-size', queueSize, 1, { status: label })
-      }
+  return {
+    counts: summaryCounts,
+    attempts: summaryAttempts,
+    requests: summaryRequests,
+    maxQueueSize: summaryMaxQueueSize,
+  }
+}
 
-      return callback(null, {
-        counts: summaryCounts,
-        attempts: summaryAttempts,
-        requests: summaryRequests,
-        maxQueueSize: summaryMaxQueueSize,
-      })
-    }
-  )
+// EXPORTS
+
+const getFailedProjectsCb = callbackify(getFailedProjects)
+const getFailureRecordCb = callbackify(getFailureRecord)
+const getFailuresCb = callbackify(getFailures)
+const getLastFailureCb = callbackify(getLastFailure)
+const recordCb = callbackify(record)
+const recordSyncStartCb = callbackify(recordSyncStart)
+const setForceDebugCb = callbackify(setForceDebug)
+
+export {
+  getFailedProjectsCb as getFailedProjects,
+  getFailureRecordCb as getFailureRecord,
+  getLastFailureCb as getLastFailure,
+  getFailuresCb as getFailures,
+  recordCb as record,
+  recordSyncStartCb as recordSyncStart,
+  setForceDebugCb as setForceDebug,
 }
 
 export const promises = {
-  getFailedProjects: promisify(getFailedProjects),
-  getFailureRecord: promisify(getFailureRecord),
-  getLastFailure: promisify(getLastFailure),
-  getFailuresByType: promisify(getFailuresByType),
-  getFailures: promisify(getFailures),
-  record: promisify(record),
-  recordSyncStart: promisify(recordSyncStart),
-  setForceDebug: promisify(setForceDebug),
+  getFailedProjects,
+  getFailureRecord,
+  getLastFailure,
+  getFailures,
+  record,
+  recordSyncStart,
+  setForceDebug,
 }

services/project-history/test/unit/js/ErrorRecorder/ErrorRecorderTest.js

@@ -1,16 +1,5 @@
-/* eslint-disable
-    no-return-assign,
-    no-undef,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 import sinon from 'sinon'
+import { expect } from 'chai'
 import { strict as esmock } from 'esmock'
 import tk from 'timekeeper'
 
@@ -20,11 +9,10 @@ describe('ErrorRecorder', function () {
   beforeEach(async function () {
     this.now = new Date()
     tk.freeze(this.now)
-    this.callback = sinon.stub()
     this.db = {
       projectHistoryFailures: {
-        deleteOne: sinon.stub().yields(),
-        updateOne: sinon.stub().yields(),
+        deleteOne: sinon.stub().resolves(),
+        updateOne: sinon.stub().resolves(),
       },
     }
     this.mongodb = { db: this.db }
@@ -35,27 +23,28 @@ describe('ErrorRecorder', function () {
     })
 
     this.project_id = 'project-id-123'
-    return (this.queueSize = 445)
+    this.queueSize = 445
   })
 
   afterEach(function () {
-    return tk.reset()
+    tk.reset()
   })
 
-  return describe('record', function () {
+  describe('record', function () {
     describe('with an error', function () {
-      beforeEach(function () {
+      beforeEach(async function () {
         this.error = new Error('something bad')
-        return this.ErrorRecorder.record(
-          this.project_id,
-          this.queueSize,
-          this.error,
-          this.callback
-        )
+        await expect(
+          this.ErrorRecorder.promises.record(
+            this.project_id,
+            this.queueSize,
+            this.error
+          )
+        ).to.be.rejected
       })
 
       it('should record the error to mongo', function () {
-        return this.db.projectHistoryFailures.updateOne
+        this.db.projectHistoryFailures.updateOne
           .calledWithMatch(
             {
               project_id: this.project_id,
@@ -91,32 +80,25 @@ describe('ErrorRecorder', function () {
           )
           .should.equal(true)
       })
-
-      return it('should call the callback', function () {
-        return this.callback
-          .calledWith(this.error, this.queueSize)
-          .should.equal(true)
-      })
     })
 
-    return describe('without an error', function () {
-      beforeEach(function () {
-        return this.ErrorRecorder.record(
+    describe('without an error', function () {
+      beforeEach(async function () {
+        this.result = await this.ErrorRecorder.promises.record(
           this.project_id,
           this.queueSize,
-          this.error,
-          this.callback
+          this.error
         )
       })
 
       it('should remove any error from mongo', function () {
-        return this.db.projectHistoryFailures.deleteOne
+        this.db.projectHistoryFailures.deleteOne
           .calledWithMatch({ project_id: this.project_id })
           .should.equal(true)
       })
 
-      return it('should call the callback', function () {
-        return this.callback.calledWith(null, this.queueSize).should.equal(true)
+      it('should return the queue size', function () {
+        expect(this.result).to.equal(this.queueSize)
       })
     })
   })