From 8fa676082e319526f501b3bf1d246db40e6f01ad Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Mon, 23 Sep 2024 13:09:32 +0200 Subject: [PATCH] Merge pull request #20299 from overleaf/jpa-object-persistor-metrics-dep [object-persistor] depend on @overleaf/metrics directly GitOrigin-RevId: eb0c07af8101d44def14154abb552bc77254e074 --- libraries/object-persistor/package.json | 6 ++-- libraries/object-persistor/src/FSPersistor.js | 11 ++----- .../object-persistor/src/GcsPersistor.js | 2 -- .../src/MigrationPersistor.js | 5 ++- .../object-persistor/src/PersistorFactory.js | 15 ++------- .../object-persistor/src/PersistorHelper.js | 32 +++++++++---------- libraries/object-persistor/src/S3Persistor.js | 7 ++-- libraries/object-persistor/test/Init.js | 19 +++++++++++ .../test/unit/GcsPersistorTests.js | 3 -- package-lock.json | 7 ++-- services/filestore/app/js/PersistorManager.js | 1 - 11 files changed, 50 insertions(+), 58 deletions(-) diff --git a/libraries/object-persistor/package.json b/libraries/object-persistor/package.json index 8fe38f54d1..3aed927437 100644 --- a/libraries/object-persistor/package.json +++ b/libraries/object-persistor/package.json @@ -19,11 +19,10 @@ }, "author": "Overleaf (https://www.overleaf.com/)", "license": "AGPL-3.0", - "peerDependencies": { - "@overleaf/logger": "*" - }, "dependencies": { "@google-cloud/storage": "^6.10.1", + "@overleaf/logger": "*", + "@overleaf/metrics": "*", "@overleaf/o-error": "*", "aws-sdk": "^2.718.0", "fast-crc32c": "overleaf/node-fast-crc32c#aae6b2a4c7a7a159395df9cc6c38dfde702d6f51", @@ -32,7 +31,6 @@ "tiny-async-pool": "^1.1.0" }, "devDependencies": { - "@overleaf/logger": "*", "chai": "^4.3.6", "chai-as-promised": "^7.1.1", "mocha": "^10.2.0", diff --git a/libraries/object-persistor/src/FSPersistor.js b/libraries/object-persistor/src/FSPersistor.js index 058017038b..d806448f9c 100644 --- a/libraries/object-persistor/src/FSPersistor.js +++ b/libraries/object-persistor/src/FSPersistor.js @@ -5,6 +5,7 @@ const globCallbacks = require('glob') const Path = require('path') const { pipeline } = require('stream/promises') const { promisify } = require('util') +const Metrics = require('@overleaf/metrics') const AbstractPersistor = require('./AbstractPersistor') const { ReadError, WriteError } = require('./Errors') @@ -16,7 +17,6 @@ module.exports = class FSPersistor extends AbstractPersistor { constructor(settings = {}) { super() this.useSubdirectories = Boolean(settings.useSubdirectories) - this.metrics = settings.Metrics } async sendFile(location, target, source) { @@ -231,17 +231,12 @@ module.exports = class FSPersistor extends AbstractPersistor { transforms.push(md5Observer.transform) } - let timer - if (this.metrics) { - timer = new this.metrics.Timer('writingFile') - } + const timer = new Metrics.Timer('writingFile') try { const writeStream = fs.createWriteStream(tempFilePath) await pipeline(stream, ...transforms, writeStream) - if (timer) { - timer.done() - } + timer.done() } catch (err) { await this._cleanupTempFile(tempFilePath) throw new WriteError( diff --git a/libraries/object-persistor/src/GcsPersistor.js b/libraries/object-persistor/src/GcsPersistor.js index 54198e58b0..dabdab10ab 100644 --- a/libraries/object-persistor/src/GcsPersistor.js +++ b/libraries/object-persistor/src/GcsPersistor.js @@ -50,7 +50,6 @@ module.exports = class GcsPersistor extends AbstractPersistor { // egress from us to gcs const observeOptions = { metric: 'gcs.egress', - Metrics: this.settings.Metrics, } let sourceMd5 = opts.sourceMd5 @@ -138,7 +137,6 @@ module.exports = class GcsPersistor extends AbstractPersistor { // ingress to us from gcs const observer = new PersistorHelper.ObserverStream({ metric: 'gcs.ingress', - Metrics: this.settings.Metrics, }) const pass = new PassThrough() diff --git a/libraries/object-persistor/src/MigrationPersistor.js b/libraries/object-persistor/src/MigrationPersistor.js index d5fbfeea9f..3f44c38cee 100644 --- a/libraries/object-persistor/src/MigrationPersistor.js +++ b/libraries/object-persistor/src/MigrationPersistor.js @@ -1,5 +1,6 @@ const AbstractPersistor = require('./AbstractPersistor') const Logger = require('@overleaf/logger') +const Metrics = require('@overleaf/metrics') const Stream = require('stream') const { pipeline } = require('stream/promises') const { NotFoundError, WriteError } = require('./Errors') @@ -194,9 +195,7 @@ module.exports = class MigrationPersistor extends AbstractPersistor { }, err ) - if (this.settings.Metrics) { - this.settings.Metrics.inc('fallback.copy.failure') - } + Metrics.inc('fallback.copy.failure') try { await this.primaryPersistor.deleteObject(destBucket, destKey) diff --git a/libraries/object-persistor/src/PersistorFactory.js b/libraries/object-persistor/src/PersistorFactory.js index 5b779d2786..7923f9c06c 100644 --- a/libraries/object-persistor/src/PersistorFactory.js +++ b/libraries/object-persistor/src/PersistorFactory.js @@ -9,19 +9,14 @@ function getPersistor(backend, settings) { switch (backend) { case 'aws-sdk': case 's3': - return new S3Persistor( - Object.assign({}, settings.s3, { Metrics: settings.Metrics }) - ) + return new S3Persistor(settings.s3) case 'fs': return new FSPersistor({ useSubdirectories: settings.useSubdirectories, paths: settings.paths, - Metrics: settings.Metrics, }) case 'gcs': - return new GcsPersistor( - Object.assign({}, settings.gcs, { Metrics: settings.Metrics }) - ) + return new GcsPersistor(settings.gcs) default: throw new SettingsError('unknown backend', { backend }) } @@ -44,11 +39,7 @@ module.exports = function create(settings) { if (settings.fallback && settings.fallback.backend) { const primary = persistor const fallback = getPersistor(settings.fallback.backend, settings) - persistor = new MigrationPersistor( - primary, - fallback, - Object.assign({}, settings.fallback, { Metrics: settings.Metrics }) - ) + persistor = new MigrationPersistor(primary, fallback, settings.fallback) } return persistor diff --git a/libraries/object-persistor/src/PersistorHelper.js b/libraries/object-persistor/src/PersistorHelper.js index 015fedd0d8..88c773cba9 100644 --- a/libraries/object-persistor/src/PersistorHelper.js +++ b/libraries/object-persistor/src/PersistorHelper.js @@ -2,31 +2,31 @@ const Crypto = require('crypto') const Stream = require('stream') const { pipeline } = require('stream/promises') const Logger = require('@overleaf/logger') +const Metrics = require('@overleaf/metrics') const { WriteError, NotFoundError } = require('./Errors') -// Observes data that passes through and computes some metadata for it -// - specifically, it computes the number of bytes transferred, and optionally -// computes a cryptographic hash based on the 'hash' option. e.g., pass -// { hash: 'md5' } to compute the md5 hash of the stream -// - if 'metric' is supplied as an option, this metric will be incremented by -// the number of bytes transferred +/** + * Observes data that passes through and optionally computes hash for content. + */ class ObserverStream extends Stream.Transform { - constructor(options) { - super({ autoDestroy: true, ...options }) + /** + * @param {string} metric prefix for metrics + * @param {string} hash optional hash algorithm, e.g. 'md5' + */ + constructor({ metric, hash = '' }) { + super({ autoDestroy: true }) this.bytes = 0 - if (options.hash) { - this.hash = Crypto.createHash(options.hash) + if (hash) { + this.hash = Crypto.createHash(hash) } - if (options.metric && options.Metrics) { - const onEnd = () => { - options.Metrics.count(options.metric, this.bytes) - } - this.once('error', onEnd) - this.once('end', onEnd) + const onEnd = () => { + Metrics.count(metric, this.bytes) } + this.once('error', onEnd) + this.once('end', onEnd) } _transform(chunk, encoding, done) { diff --git a/libraries/object-persistor/src/S3Persistor.js b/libraries/object-persistor/src/S3Persistor.js index f817285e5d..de3adcb67c 100644 --- a/libraries/object-persistor/src/S3Persistor.js +++ b/libraries/object-persistor/src/S3Persistor.js @@ -7,6 +7,7 @@ if (https.globalAgent.maxSockets < 300) { https.globalAgent.maxSockets = 300 } +const Metrics = require('@overleaf/metrics') const AbstractPersistor = require('./AbstractPersistor') const PersistorHelper = require('./PersistorHelper') @@ -32,7 +33,6 @@ module.exports = class S3Persistor extends AbstractPersistor { // egress from us to S3 const observeOptions = { metric: 's3.egress', - Metrics: this.settings.Metrics, } const observer = new PersistorHelper.ObserverStream(observeOptions) @@ -120,7 +120,6 @@ module.exports = class S3Persistor extends AbstractPersistor { // ingress from S3 to us const observer = new PersistorHelper.ObserverStream({ metric: 's3.ingress', - Metrics: this.settings.Metrics, }) const pass = new PassThrough() @@ -228,9 +227,7 @@ module.exports = class S3Persistor extends AbstractPersistor { return md5 } // etag is not in md5 format - if (this.settings.Metrics) { - this.settings.Metrics.inc('s3.md5Download') - } + Metrics.inc('s3.md5Download') return await PersistorHelper.calculateStreamMd5( await this.getObjectStream(bucketName, key) ) diff --git a/libraries/object-persistor/test/Init.js b/libraries/object-persistor/test/Init.js index 0ed2aeb0d9..3fe772b36b 100644 --- a/libraries/object-persistor/test/Init.js +++ b/libraries/object-persistor/test/Init.js @@ -1,9 +1,28 @@ const SandboxedModule = require('sandboxed-module') const chai = require('chai') +const sinon = require('sinon') chai.use(require('sinon-chai')) chai.use(require('chai-as-promised')) SandboxedModule.configure({ + requires: { + '@overleaf/logger': { + debug() {}, + log() {}, + info() {}, + warn() {}, + error() {}, + err() {}, + }, + '@overleaf/metrics': { + inc: sinon.stub(), + count: sinon.stub(), + histogram: sinon.stub(), + Timer: class Timer { + done() {} + }, + }, + }, globals: { Buffer, Math, console, process, URL }, }) diff --git a/libraries/object-persistor/test/unit/GcsPersistorTests.js b/libraries/object-persistor/test/unit/GcsPersistorTests.js index 3ca2703c24..6a2b10f1e8 100644 --- a/libraries/object-persistor/test/unit/GcsPersistorTests.js +++ b/libraries/object-persistor/test/unit/GcsPersistorTests.js @@ -41,9 +41,6 @@ describe('GcsPersistorTests', function () { beforeEach(function () { Settings = { directoryKeyRegex: /^[0-9a-fA-F]{24}\/[0-9a-fA-F]{24}/, - Metrics: { - count: sinon.stub(), - }, } files = [ diff --git a/package-lock.json b/package-lock.json index 84004e84af..90313fc325 100644 --- a/package-lock.json +++ b/package-lock.json @@ -293,6 +293,8 @@ "license": "AGPL-3.0", "dependencies": { "@google-cloud/storage": "^6.10.1", + "@overleaf/logger": "*", + "@overleaf/metrics": "*", "@overleaf/o-error": "*", "aws-sdk": "^2.718.0", "fast-crc32c": "overleaf/node-fast-crc32c#aae6b2a4c7a7a159395df9cc6c38dfde702d6f51", @@ -301,7 +303,6 @@ "tiny-async-pool": "^1.1.0" }, "devDependencies": { - "@overleaf/logger": "*", "chai": "^4.3.6", "chai-as-promised": "^7.1.1", "mocha": "^10.2.0", @@ -311,9 +312,6 @@ "sinon": "^9.2.4", "sinon-chai": "^3.7.0", "typescript": "^5.0.4" - }, - "peerDependencies": { - "@overleaf/logger": "*" } }, "libraries/object-persistor/node_modules/bson": { @@ -52317,6 +52315,7 @@ "requires": { "@google-cloud/storage": "^6.10.1", "@overleaf/logger": "*", + "@overleaf/metrics": "*", "@overleaf/o-error": "*", "aws-sdk": "^2.718.0", "chai": "^4.3.6", diff --git a/services/filestore/app/js/PersistorManager.js b/services/filestore/app/js/PersistorManager.js index 3a57723be2..c6442d2ca8 100644 --- a/services/filestore/app/js/PersistorManager.js +++ b/services/filestore/app/js/PersistorManager.js @@ -1,7 +1,6 @@ const settings = require('@overleaf/settings') const persistorSettings = settings.filestore -persistorSettings.Metrics = require('@overleaf/metrics') persistorSettings.paths = settings.path const ObjectPersistor = require('@overleaf/object-persistor')