From def383574ea03f8e2899d0825071c25d63b4a7ec Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Wed, 4 Mar 2020 16:17:36 +0000
Subject: [PATCH] Simplify getMeteredStream to record metric directly

---
 services/filestore/app/js/GcsPersistor.js          | 10 ++--------
 services/filestore/app/js/PersistorHelper.js       |  9 +++++----
 services/filestore/app/js/S3Persistor.js           | 14 ++------------
 .../filestore/test/unit/js/FSPersistorTests.js     |  3 ++-
 4 files changed, 11 insertions(+), 25 deletions(-)

diff --git a/services/filestore/app/js/GcsPersistor.js b/services/filestore/app/js/GcsPersistor.js
index dd2c44d5ba..9ddf608e90 100644
--- a/services/filestore/app/js/GcsPersistor.js
+++ b/services/filestore/app/js/GcsPersistor.js
@@ -1,5 +1,4 @@
 const settings = require('settings-sharelatex')
-const metrics = require('metrics-sharelatex')
 const fs = require('fs')
 const { promisify } = require('util')
 const Stream = require('stream')
@@ -79,9 +78,7 @@ async function sendStream(bucketName, key, readStream, sourceMd5) {
 
     const meteredStream = PersistorHelper.getMeteredStream(
       readStream,
-      (_, byteCount) => {
-        metrics.count('gcs.egress', byteCount)
-      }
+      'gcs.egress'
     )
 
     const writeOptions = {
@@ -129,10 +126,7 @@ async function getFileStream(bucketName, key, opts = {}) {
     .file(key)
     .createReadStream(opts)
 
-  const meteredStream = PersistorHelper.getMeteredStream(stream, (_, bytes) => {
-    // ignore the error parameter and just log the byte count
-    metrics.count('gcs.ingress', bytes)
-  })
+  const meteredStream = PersistorHelper.getMeteredStream(stream, 'gcs.ingress')
 
   try {
     await PersistorHelper.waitForStreamReady(stream)
diff --git a/services/filestore/app/js/PersistorHelper.js b/services/filestore/app/js/PersistorHelper.js
index 409f3182f1..a19311e889 100644
--- a/services/filestore/app/js/PersistorHelper.js
+++ b/services/filestore/app/js/PersistorHelper.js
@@ -1,4 +1,5 @@
 const crypto = require('crypto')
+const metrics = require('metrics-sharelatex')
 const meter = require('stream-meter')
 const Stream = require('stream')
 const logger = require('logger-sharelatex')
@@ -54,16 +55,16 @@ async function verifyMd5(persistor, bucket, key, sourceMd5, destMd5 = null) {
 
 // returns the next stream in the pipeline, and calls the callback with the byte count
 // when the stream finishes or receives an error
-function getMeteredStream(stream, callback) {
+function getMeteredStream(stream, metricName) {
   const meteredStream = meter()
 
   pipeline(stream, meteredStream)
     .then(() => {
-      callback(null, meteredStream.bytes)
+      metrics.count(metricName, meteredStream.bytes)
     })
-    .catch(err => {
+    .catch(() => {
       // on error, just send how many bytes we received before the stream stopped
-      callback(err, meteredStream.bytes)
+      metrics.count(metricName, meteredStream.bytes)
     })
 
   return meteredStream
diff --git a/services/filestore/app/js/S3Persistor.js b/services/filestore/app/js/S3Persistor.js
index 7b69ad5d26..e50a9e3030 100644
--- a/services/filestore/app/js/S3Persistor.js
+++ b/services/filestore/app/js/S3Persistor.js
@@ -4,7 +4,6 @@ http.globalAgent.maxSockets = 300
 https.globalAgent.maxSockets = 300
 
 const settings = require('settings-sharelatex')
-const metrics = require('metrics-sharelatex')
 
 const PersistorHelper = require('./PersistorHelper')
 
@@ -75,10 +74,7 @@ async function sendStream(bucketName, key, readStream, sourceMd5) {
 
     const meteredStream = PersistorHelper.getMeteredStream(
       readStream,
-      (_, byteCount) => {
-        // ignore the error parameter and just log the byte count
-        metrics.count('s3.egress', byteCount)
-      }
+      's3.egress'
     )
 
     // if we have an md5 hash, pass this to S3 to verify the upload
@@ -143,13 +139,7 @@ async function getFileStream(bucketName, key, opts) {
     .getObject(params)
     .createReadStream()
 
-  const meteredStream = PersistorHelper.getMeteredStream(
-    stream,
-    (_, byteCount) => {
-      // ignore the error parameter and just log the byte count
-      metrics.count('s3.ingress', byteCount)
-    }
-  )
+  const meteredStream = PersistorHelper.getMeteredStream(stream, 's3.ingress')
 
   try {
     await PersistorHelper.waitForStreamReady(stream)
diff --git a/services/filestore/test/unit/js/FSPersistorTests.js b/services/filestore/test/unit/js/FSPersistorTests.js
index 0a09869bc0..4dd5a2fa11 100644
--- a/services/filestore/test/unit/js/FSPersistorTests.js
+++ b/services/filestore/test/unit/js/FSPersistorTests.js
@@ -73,7 +73,8 @@ describe('FSPersistorTests', function() {
         crypto,
         // imported by PersistorHelper but otherwise unused here
         'stream-meter': {},
-        'logger-sharelatex': {}
+        'logger-sharelatex': {},
+        'metrics-sharelatex': {}
       },
       globals: { console }
     })