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

@@ -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",

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
+    const timer = new Metrics.Timer('writingFile')
-    if (this.metrics) {
-      timer = new this.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(

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()

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) {
+      Metrics.inc('fallback.copy.failure')
-        this.settings.Metrics.inc('fallback.copy.failure')
-      }
 
       try {
         await this.primaryPersistor.deleteObject(destBucket, destKey)

libraries/object-persistor/src/PersistorFactory.js

@@ -9,19 +9,14 @@ function getPersistor(backend, settings) {
   switch (backend) {
     case 'aws-sdk':
     case 's3':
-      return new S3Persistor(
+      return new S3Persistor(settings.s3)
-        Object.assign({}, settings.s3, { Metrics: settings.Metrics })
-      )
     case 'fs':
       return new FSPersistor({
         useSubdirectories: settings.useSubdirectories,
         paths: settings.paths,
-        Metrics: settings.Metrics,
       })
     case 'gcs':
-      return new GcsPersistor(
+      return new GcsPersistor(settings.gcs)
-        Object.assign({}, settings.gcs, { Metrics: settings.Metrics })
-      )
     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(
+    persistor = new MigrationPersistor(primary, fallback, settings.fallback)
-      primary,
-      fallback,
-      Object.assign({}, settings.fallback, { Metrics: settings.Metrics })
-    )
   }
 
   return persistor

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
+ * Observes data that passes through and optionally computes hash for content.
-//   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
 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) {
+    if (hash) {
-      this.hash = Crypto.createHash(options.hash)
+      this.hash = Crypto.createHash(hash)
     }
 
-    if (options.metric && options.Metrics) {
+    const onEnd = () => {
-      const onEnd = () => {
+      Metrics.count(metric, this.bytes)
-        options.Metrics.count(options.metric, this.bytes)
-      }
-      this.once('error', onEnd)
-      this.once('end', onEnd)
     }
+    this.once('error', onEnd)
+    this.once('end', onEnd)
   }
 
   _transform(chunk, encoding, done) {

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) {
+      Metrics.inc('s3.md5Download')
-        this.settings.Metrics.inc('s3.md5Download')
-      }
       return await PersistorHelper.calculateStreamMd5(
         await this.getObjectStream(bucketName, key)
       )

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 },
 })

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 = [

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",

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')