From e74338e51ef6fc8b9bf5913f236e2b9579fc2b45 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Thu, 11 Apr 2024 08:33:03 -0400 Subject: [PATCH] Merge pull request #17731 from overleaf/em-promisify-error-recorder Promisify ErrorRecorder GitOrigin-RevId: 3736567272a09b4e3b9075118460392c1f66f0d7 --- .../project-history/app/js/ErrorRecorder.js | 445 ++++++++---------- .../js/ErrorRecorder/ErrorRecorderTest.js | 62 +-- 2 files changed, 214 insertions(+), 293 deletions(-) diff --git a/services/project-history/app/js/ErrorRecorder.js b/services/project-history/app/js/ErrorRecorder.js index 570110dbf9..e8995ee87c 100644 --- a/services/project-history/app/js/ErrorRecorder.js +++ b/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( +async function recordSyncStart(projectId) { + await db.projectHistoryFailures.updateOne( + { project_id: projectId }, { - 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 () {} +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 + } } - db.projectHistoryFailures.find({}).toArray(function (error, results) { - if (error != null) { - return callback(OError.tag(error)) - } - 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 } +} + +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', + } + + // 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 + } + + // 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 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 summaryMaxQueueSize) { + const queueSize = summaryMaxQueueSize[label] + metrics.globalGauge('max-queue-size', queueSize, 1, { status: label }) + } + + return { + counts: summaryCounts, + attempts: summaryAttempts, + requests: summaryRequests, + maxQueueSize: summaryMaxQueueSize, + } } -export function getFailures(callback) { - if (callback == null) { - callback = function () {} - } - return getFailuresByType( - function ( - error, - failureCounts, - failureAttempts, - failureRequests, - maxQueueSize - ) { - let attempts, failureType, label, requests - if (error != null) { - return callback(OError.tag(error)) - } +// EXPORTS - 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', - } +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) - // 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 - } - - // 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 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 summaryMaxQueueSize) { - const queueSize = summaryMaxQueueSize[label] - metrics.globalGauge('max-queue-size', queueSize, 1, { status: label }) - } - - return callback(null, { - counts: summaryCounts, - attempts: summaryAttempts, - requests: summaryRequests, - maxQueueSize: summaryMaxQueueSize, - }) - } - ) +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, } diff --git a/services/project-history/test/unit/js/ErrorRecorder/ErrorRecorderTest.js b/services/project-history/test/unit/js/ErrorRecorder/ErrorRecorderTest.js index f42cff8c01..db6d767e58 100644 --- a/services/project-history/test/unit/js/ErrorRecorder/ErrorRecorderTest.js +++ b/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) }) }) })