From 07693297347864c19afa60fd8e7cbcef187461df Mon Sep 17 00:00:00 2001 From: David <33458145+davidmcpowell@users.noreply.github.com> Date: Wed, 28 Feb 2024 11:20:53 +0000 Subject: [PATCH] Merge pull request #17268 from overleaf/dp-remove-old-mongo-metrics Remove timeAsyncMethod mongo metrics GitOrigin-RevId: 1ba3a1fd51b9d0766355c31791ae9836d832afe8 --- libraries/metrics/index.js | 1 - .../test/unit/js/timeAsyncMethodTests.js | 201 ------------------ libraries/metrics/timeAsyncMethod.js | 86 -------- .../test/unit/js/MongoManagerTests.js | 1 - .../notifications/app/js/Notifications.js | 7 +- .../test/unit/js/NotificationsTests.js | 1 - .../Features/Institutions/InstitutionsAPI.js | 13 -- .../Project/ProjectCreationHandler.js | 8 - .../Features/Spelling/LearnedWordsManager.js | 10 - .../web/app/src/Features/User/UserGetter.js | 12 -- .../src/Institutions/InstitutionsAPITests.js | 3 - .../unit/src/Project/ProjectGetterTests.js | 3 - .../src/Spelling/LearnedWordsManagerTests.js | 1 - .../test/unit/src/User/UserCreatorTests.js | 1 - .../web/test/unit/src/User/UserGetterTests.js | 3 - 15 files changed, 1 insertion(+), 350 deletions(-) delete mode 100644 libraries/metrics/test/unit/js/timeAsyncMethodTests.js delete mode 100644 libraries/metrics/timeAsyncMethod.js diff --git a/libraries/metrics/index.js b/libraries/metrics/index.js index c4cc2b8f3e..257e863b79 100644 --- a/libraries/metrics/index.js +++ b/libraries/metrics/index.js @@ -180,4 +180,3 @@ module.exports.leaked_sockets = require('./leaked_sockets') module.exports.event_loop = require('./event_loop') module.exports.memory = require('./memory') module.exports.mongodb = require('./mongodb') -module.exports.timeAsyncMethod = require('./timeAsyncMethod') diff --git a/libraries/metrics/test/unit/js/timeAsyncMethodTests.js b/libraries/metrics/test/unit/js/timeAsyncMethodTests.js deleted file mode 100644 index f39d257408..0000000000 --- a/libraries/metrics/test/unit/js/timeAsyncMethodTests.js +++ /dev/null @@ -1,201 +0,0 @@ -/* - * decaffeinate suggestions: - * 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 - */ -const chai = require('chai') -const { expect } = chai -const path = require('path') -const modulePath = path.join(__dirname, '../../../timeAsyncMethod.js') -const SandboxedModule = require('sandboxed-module') -const sinon = require('sinon') - -describe('timeAsyncMethod', function () { - beforeEach(function () { - this.Timer = { done: sinon.stub() } - this.TimerConstructor = sinon.stub().returns(this.Timer) - this.metrics = { - Timer: this.TimerConstructor, - inc: sinon.stub(), - } - this.timeAsyncMethod = SandboxedModule.require(modulePath, { - requires: { - './index': this.metrics, - }, - }) - - return (this.testObject = { - nextNumber(n, callback) { - return setTimeout(() => callback(null, n + 1), 100) - }, - }) - }) - - it('should have the testObject behave correctly before wrapping', function (done) { - return this.testObject.nextNumber(2, (err, result) => { - expect(err).to.not.exist - expect(result).to.equal(3) - return done() - }) - }) - - it('should wrap method without error', function (done) { - this.timeAsyncMethod( - this.testObject, - 'nextNumber', - 'someContext.TestObject' - ) - return done() - }) - - it('should transparently wrap method invocation in timer', function (done) { - this.timeAsyncMethod( - this.testObject, - 'nextNumber', - 'someContext.TestObject' - ) - return this.testObject.nextNumber(2, (err, result) => { - expect(err).to.not.exist - expect(result).to.equal(3) - expect(this.TimerConstructor.callCount).to.equal(1) - expect(this.Timer.done.callCount).to.equal(1) - return done() - }) - }) - - it('should increment success count', function (done) { - this.metrics.inc = sinon.stub() - this.timeAsyncMethod( - this.testObject, - 'nextNumber', - 'someContext.TestObject' - ) - return this.testObject.nextNumber(2, (err, result) => { - if (err) { - return done(err) - } - expect(this.metrics.inc.callCount).to.equal(1) - expect( - this.metrics.inc.calledWith('someContext_result', 1, { - method: 'TestObject_nextNumber', - status: 'success', - }) - ).to.equal(true) - return done() - }) - }) - - describe('when base method produces an error', function () { - beforeEach(function () { - this.metrics.inc = sinon.stub() - return (this.testObject.nextNumber = function (n, callback) { - return setTimeout(() => callback(new Error('woops')), 100) - }) - }) - - it('should propagate the error transparently', function (done) { - this.timeAsyncMethod( - this.testObject, - 'nextNumber', - 'someContext.TestObject' - ) - return this.testObject.nextNumber(2, (err, result) => { - expect(err).to.exist - expect(err).to.be.instanceof(Error) - expect(result).to.not.exist - return done() - }) - }) - - return it('should increment failure count', function (done) { - this.timeAsyncMethod( - this.testObject, - 'nextNumber', - 'someContext.TestObject' - ) - return this.testObject.nextNumber(2, (err, result) => { - expect(err).to.exist - expect(this.metrics.inc.callCount).to.equal(1) - expect( - this.metrics.inc.calledWith('someContext_result', 1, { - method: 'TestObject_nextNumber', - status: 'failed', - }) - ).to.equal(true) - return done() - }) - }) - }) - - describe('when a logger is supplied', function () { - beforeEach(function () { - return (this.logger = { debug: sinon.stub(), log: sinon.stub() }) - }) - - return it('should also call logger.debug', function (done) { - this.timeAsyncMethod( - this.testObject, - 'nextNumber', - 'someContext.TestObject', - this.logger - ) - return this.testObject.nextNumber(2, (err, result) => { - expect(err).to.not.exist - expect(result).to.equal(3) - expect(this.TimerConstructor.callCount).to.equal(1) - expect(this.Timer.done.callCount).to.equal(1) - expect(this.logger.debug.callCount).to.equal(1) - return done() - }) - }) - }) - - describe('when the wrapper cannot be applied', function () { - beforeEach(function () {}) - - return it('should raise an error', function () { - const badWrap = () => { - return this.timeAsyncMethod( - this.testObject, - 'DEFINITELY_NOT_A_REAL_METHOD', - 'someContext.TestObject' - ) - } - return expect(badWrap).to.throw( - /^.*expected object property 'DEFINITELY_NOT_A_REAL_METHOD' to be a function.*$/ - ) - }) - }) - - return describe('when the wrapped function is not using a callback', function () { - beforeEach(function () { - this.realMethod = sinon.stub().returns(42) - return (this.testObject.nextNumber = this.realMethod) - }) - - it('should not throw an error', function () { - this.timeAsyncMethod( - this.testObject, - 'nextNumber', - 'someContext.TestObject' - ) - const badCall = () => { - return this.testObject.nextNumber(2) - } - return expect(badCall).to.not.throw(Error) - }) - - return it('should call the underlying method', function () { - this.timeAsyncMethod( - this.testObject, - 'nextNumber', - 'someContext.TestObject' - ) - const result = this.testObject.nextNumber(12) - expect(this.realMethod.callCount).to.equal(1) - expect(this.realMethod.calledWith(12)).to.equal(true) - return expect(result).to.equal(42) - }) - }) -}) diff --git a/libraries/metrics/timeAsyncMethod.js b/libraries/metrics/timeAsyncMethod.js deleted file mode 100644 index 3bf0eb40d6..0000000000 --- a/libraries/metrics/timeAsyncMethod.js +++ /dev/null @@ -1,86 +0,0 @@ -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns - * DS201: Simplify complex destructure assignments - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ - -module.exports = function (obj, methodName, prefix, logger) { - let modifedMethodName - const metrics = require('./index') - - if (typeof obj[methodName] !== 'function') { - throw new Error( - `[Metrics] expected object property '${methodName}' to be a function` - ) - } - - const key = `${prefix}.${methodName}` - - const realMethod = obj[methodName] - - const splitPrefix = prefix.split('.') - const startPrefix = splitPrefix[0] - - if (splitPrefix[1] != null) { - modifedMethodName = `${splitPrefix[1]}_${methodName}` - } else { - modifedMethodName = methodName - } - return (obj[methodName] = function (...originalArgs) { - const adjustedLength = Math.max(originalArgs.length, 1) - const firstArgs = originalArgs.slice(0, adjustedLength - 1) - const callback = originalArgs[adjustedLength - 1] - - if (callback == null || typeof callback !== 'function') { - if (logger != null) { - logger.debug( - `[Metrics] expected wrapped method '${methodName}' to be invoked with a callback` - ) - } - return realMethod.apply(this, originalArgs) - } - - const timer = new metrics.Timer(startPrefix, 1, { - method: modifedMethodName, - }) - - return realMethod.call( - this, - ...Array.from(firstArgs), - function (...callbackArgs) { - const elapsedTime = timer.done() - const possibleError = callbackArgs[0] - if (possibleError != null) { - metrics.inc(`${startPrefix}_result`, 1, { - status: 'failed', - method: modifedMethodName, - }) - } else { - metrics.inc(`${startPrefix}_result`, 1, { - status: 'success', - method: modifedMethodName, - }) - } - if (logger != null) { - const loggableArgs = {} - try { - for (let idx = 0; idx < firstArgs.length; idx++) { - const arg = firstArgs[idx] - if (arg.toString().match(/^[0-9a-f]{24}$/)) { - loggableArgs[`${idx}`] = arg - } - } - } catch (error) {} - logger.debug( - { key, args: loggableArgs, elapsedTime }, - '[Metrics] timed async method call' - ) - } - return callback.apply(this, callbackArgs) - } - ) - }) -} diff --git a/services/docstore/test/unit/js/MongoManagerTests.js b/services/docstore/test/unit/js/MongoManagerTests.js index 71205d77bd..4f8001f482 100644 --- a/services/docstore/test/unit/js/MongoManagerTests.js +++ b/services/docstore/test/unit/js/MongoManagerTests.js @@ -22,7 +22,6 @@ describe('MongoManager', function () { db: this.db, ObjectId, }, - '@overleaf/metrics': { timeAsyncMethod: sinon.stub() }, '@overleaf/settings': { max_deleted_docs: 42, docstore: { archivingLockDurationMs: 5000 }, diff --git a/services/notifications/app/js/Notifications.js b/services/notifications/app/js/Notifications.js index 027ff636ce..9c4df59bd2 100644 --- a/services/notifications/app/js/Notifications.js +++ b/services/notifications/app/js/Notifications.js @@ -9,12 +9,10 @@ * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ -let Notifications const logger = require('@overleaf/logger') const { db, ObjectId } = require('./mongodb') -const metrics = require('@overleaf/metrics') -module.exports = Notifications = { +module.exports = { getUserNotifications(userId, callback) { if (callback == null) { callback = function () {} @@ -132,6 +130,3 @@ module.exports = Notifications = { db.notifications.deleteOne(searchOps, callback) }, } -;['getUserNotifications', 'addNotification'].map(method => - metrics.timeAsyncMethod(Notifications, method, 'mongo.Notifications', logger) -) diff --git a/services/notifications/test/unit/js/NotificationsTests.js b/services/notifications/test/unit/js/NotificationsTests.js index 0362b1af79..3bf145bf29 100644 --- a/services/notifications/test/unit/js/NotificationsTests.js +++ b/services/notifications/test/unit/js/NotificationsTests.js @@ -41,7 +41,6 @@ describe('Notifications Tests', function () { requires: { '@overleaf/settings': {}, './mongodb': { db: this.db, ObjectId }, - '@overleaf/metrics': { timeAsyncMethod: sinon.stub() }, }, }) diff --git a/services/web/app/src/Features/Institutions/InstitutionsAPI.js b/services/web/app/src/Features/Institutions/InstitutionsAPI.js index 12745ac239..bdb575c33c 100644 --- a/services/web/app/src/Features/Institutions/InstitutionsAPI.js +++ b/services/web/app/src/Features/Institutions/InstitutionsAPI.js @@ -1,7 +1,6 @@ const { callbackify } = require('util') const OError = require('@overleaf/o-error') const logger = require('@overleaf/logger') -const metrics = require('@overleaf/metrics') const settings = require('@overleaf/settings') const request = require('requestretry') const { promisifyAll } = require('@overleaf/promise-utils') @@ -373,18 +372,6 @@ function makeAffiliationRequest(options, callback) { callback(null, body) }) } -;[ - 'getInstitutionAffiliations', - 'getConfirmedInstitutionAffiliations', - 'getUserAffiliations', -].map(method => - metrics.timeAsyncMethod( - InstitutionsAPI, - method, - 'mongo.InstitutionsAPI', - logger - ) -) InstitutionsAPI.promises = promisifyAll(InstitutionsAPI, { without: [ diff --git a/services/web/app/src/Features/Project/ProjectCreationHandler.js b/services/web/app/src/Features/Project/ProjectCreationHandler.js index 9cd7cf5f72..5dda23eeb9 100644 --- a/services/web/app/src/Features/Project/ProjectCreationHandler.js +++ b/services/web/app/src/Features/Project/ProjectCreationHandler.js @@ -1,4 +1,3 @@ -const logger = require('@overleaf/logger') const OError = require('@overleaf/o-error') const metrics = require('@overleaf/metrics') const Settings = require('@overleaf/settings') @@ -242,10 +241,3 @@ module.exports = { createExampleProject, }, } - -metrics.timeAsyncMethod( - module.exports, - 'createBlankProject', - 'mongo.ProjectCreationHandler', - logger -) diff --git a/services/web/app/src/Features/Spelling/LearnedWordsManager.js b/services/web/app/src/Features/Spelling/LearnedWordsManager.js index 8c3a38c37a..66d80fcaa8 100644 --- a/services/web/app/src/Features/Spelling/LearnedWordsManager.js +++ b/services/web/app/src/Features/Spelling/LearnedWordsManager.js @@ -1,6 +1,4 @@ const { db } = require('../../infrastructure/mongodb') -const logger = require('@overleaf/logger') -const metrics = require('@overleaf/metrics') const { promisify } = require('util') const OError = require('@overleaf/o-error') const Settings = require('@overleaf/settings') @@ -94,11 +92,3 @@ const promises = { LearnedWordsManager.promises = promises module.exports = LearnedWordsManager -;['learnWord', 'unlearnWord', 'getLearnedWords'].map(method => - metrics.timeAsyncMethod( - LearnedWordsManager, - method, - 'mongo.LearnedWordsManager', - logger - ) -) diff --git a/services/web/app/src/Features/User/UserGetter.js b/services/web/app/src/Features/User/UserGetter.js index edade901a7..447c9b1c4f 100644 --- a/services/web/app/src/Features/User/UserGetter.js +++ b/services/web/app/src/Features/User/UserGetter.js @@ -1,7 +1,5 @@ const { callbackify } = require('util') const { db } = require('../../infrastructure/mongodb') -const metrics = require('@overleaf/metrics') -const logger = require('@overleaf/logger') const moment = require('moment') const settings = require('@overleaf/settings') const { promisifyAll } = require('@overleaf/promise-utils') @@ -305,16 +303,6 @@ const decorateFullEmails = ( return emailsData } -;[ - 'getUser', - 'getUserEmail', - 'getUserByMainEmail', - 'getUserByAnyEmail', - 'getUsers', - 'ensureUniqueEmailAddress', -].map(method => - metrics.timeAsyncMethod(UserGetter, method, 'mongo.UserGetter', logger) -) UserGetter.promises = promisifyAll(UserGetter, { without: ['getSsoUsersAtInstitution', 'getUserFullEmails'], diff --git a/services/web/test/unit/src/Institutions/InstitutionsAPITests.js b/services/web/test/unit/src/Institutions/InstitutionsAPITests.js index bf0a106a69..eb98115ec2 100644 --- a/services/web/test/unit/src/Institutions/InstitutionsAPITests.js +++ b/services/web/test/unit/src/Institutions/InstitutionsAPITests.js @@ -20,9 +20,6 @@ describe('InstitutionsAPI', function () { } this.InstitutionsAPI = SandboxedModule.require(modulePath, { requires: { - '@overleaf/metrics': { - timeAsyncMethod: sinon.stub(), - }, '@overleaf/settings': this.settings, requestretry: this.request, '@overleaf/fetch-utils': { diff --git a/services/web/test/unit/src/Project/ProjectGetterTests.js b/services/web/test/unit/src/Project/ProjectGetterTests.js index 5a85eff753..10d8de6000 100644 --- a/services/web/test/unit/src/Project/ProjectGetterTests.js +++ b/services/web/test/unit/src/Project/ProjectGetterTests.js @@ -53,9 +53,6 @@ describe('ProjectGetter', function () { this.ProjectGetter = SandboxedModule.require(modulePath, { requires: { '../../infrastructure/mongodb': { db: this.db, ObjectId }, - '@overleaf/metrics': { - timeAsyncMethod: sinon.stub(), - }, '../../models/Project': { Project: this.Project, }, diff --git a/services/web/test/unit/src/Spelling/LearnedWordsManagerTests.js b/services/web/test/unit/src/Spelling/LearnedWordsManagerTests.js index c6b89ef3c0..111b8959b2 100644 --- a/services/web/test/unit/src/Spelling/LearnedWordsManagerTests.js +++ b/services/web/test/unit/src/Spelling/LearnedWordsManagerTests.js @@ -21,7 +21,6 @@ describe('LearnedWordsManager', function () { requires: { '../../infrastructure/mongodb': { db: this.db }, '@overleaf/metrics': { - timeAsyncMethod: sinon.stub(), inc: sinon.stub(), }, '@overleaf/settings': { diff --git a/services/web/test/unit/src/User/UserCreatorTests.js b/services/web/test/unit/src/User/UserCreatorTests.js index d11df91500..ddbd48da64 100644 --- a/services/web/test/unit/src/User/UserCreatorTests.js +++ b/services/web/test/unit/src/User/UserCreatorTests.js @@ -19,7 +19,6 @@ describe('UserCreator', function () { '../../models/User': { User: this.UserModel, }, - '@overleaf/metrics': { timeAsyncMethod() {} }, '../../infrastructure/Features': (this.Features = { hasFeature: sinon.stub().returns(false), }), diff --git a/services/web/test/unit/src/User/UserGetterTests.js b/services/web/test/unit/src/User/UserGetterTests.js index d69efd113e..e76ffbd781 100644 --- a/services/web/test/unit/src/User/UserGetterTests.js +++ b/services/web/test/unit/src/User/UserGetterTests.js @@ -49,9 +49,6 @@ describe('UserGetter', function () { requires: { '../Helpers/Mongo': { normalizeQuery, normalizeMultiQuery }, '../../infrastructure/mongodb': this.Mongo, - '@overleaf/metrics': { - timeAsyncMethod: sinon.stub(), - }, '@overleaf/settings': (this.settings = { reconfirmNotificationDays: 14, }),