From 48aa14159163217deb75181e4d7435f325e163e5 Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Thu, 5 Dec 2019 15:46:14 +0000
Subject: [PATCH] Add metric for s3 ingress

---
 .../app/coffee/S3PersistorManager.coffee      |  3 ++
 .../acceptance/coffee/SendingFileTest.coffee  | 38 ++++++++++++++++---
 .../coffee/S3PersistorManagerTests.coffee     |  1 +
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/services/filestore/app/coffee/S3PersistorManager.coffee b/services/filestore/app/coffee/S3PersistorManager.coffee
index 14ab1bffe5..b575001fb3 100644
--- a/services/filestore/app/coffee/S3PersistorManager.coffee
+++ b/services/filestore/app/coffee/S3PersistorManager.coffee
@@ -132,6 +132,9 @@ module.exports =
 				logger.log({bucketName: bucketName, key: key }, "error getting file from s3: #{statusCode}")
 				return callback(new Error("Got non-200 response from S3: #{statusCode} #{statusMessage}"), null)
 			stream = response.httpResponse.createUnbufferedStream()
+			stream.on 'data', (data) ->
+				metrics.count 's3.ingress', data.byteLength
+
 			callback(null, stream)
 
 		s3Request.on 'error', (err) =>
diff --git a/services/filestore/test/acceptance/coffee/SendingFileTest.coffee b/services/filestore/test/acceptance/coffee/SendingFileTest.coffee
index fb03697cb0..cd1fa167f5 100644
--- a/services/filestore/test/acceptance/coffee/SendingFileTest.coffee
+++ b/services/filestore/test/acceptance/coffee/SendingFileTest.coffee
@@ -9,6 +9,7 @@ fs = require("fs")
 request = require("request")
 settings = require("settings-sharelatex")
 FilestoreApp = require "./FilestoreApp"
+async = require('async')
 
 
 getMetric = (filestoreUrl, metric, cb) ->
@@ -33,10 +34,19 @@ describe "Filestore", ->
 
 	beforeEach (done)->
 		FilestoreApp.ensureRunning =>
-			fs.unlink @localFileWritePath, =>
-				getMetric @filestoreUrl, 's3_egress', (metric) =>
-					@previousEgress = metric
-					done()
+			async.parallel [
+				(cb) =>
+					fs.unlink @localFileWritePath, () ->
+						cb()
+				(cb) =>
+					getMetric @filestoreUrl, 's3_egress', (metric) =>
+						@previousEgress = metric
+						cb()
+				(cb) =>
+					getMetric @filestoreUrl, 's3_ingress', (metric) =>
+						@previousIngress = metric
+						cb()
+			], done
 
 	it "should send a 200 for status endpoint", (done)->
 		request "#{@filestoreUrl}/status", (err, response, body)->
@@ -83,7 +93,14 @@ describe "Filestore", ->
 				body.should.equal @constantFileContent
 				done()
 
-		it "should be able to get back the first 8 bytes of the file", (done) ->
+		it "should record an ingress metric when downloading the file", (done)->
+			@timeout(1000 * 10)
+			request.get @fileUrl, () =>
+				getMetric @filestoreUrl, 's3_ingress', (metric) =>
+					expect(metric - @previousIngress).to.equal @constantFileContent.length
+					done()
+
+		it "should be able to get back the first 9 bytes of the file", (done) ->
 			@timeout(1000 * 10)
 			options =
 				uri: @fileUrl
@@ -93,6 +110,17 @@ describe "Filestore", ->
 				body.should.equal 'hello wor'
 				done()
 
+		it "should record an ingress metric for a partial download", (done)->
+			@timeout(1000 * 10)
+			options =
+				uri: @fileUrl
+				headers:
+					'Range': 'bytes=0-8'
+			request.get options, ()=>
+				getMetric @filestoreUrl, 's3_ingress', (metric) =>
+					expect(metric - @previousIngress).to.equal 9
+					done()
+
 		it "should be able to get back bytes 4 through 10 of the file", (done) ->
 			@timeout(1000 * 10)
 			options =
diff --git a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee
index 8a1b2e4d49..a5ab5c2932 100644
--- a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee
+++ b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee
@@ -62,6 +62,7 @@ describe "S3PersistorManagerTests", ->
 		describe "success", ->
 			beforeEach () ->
 				@expectedStream = { expectedStream: true }
+				@expectedStream.on = sinon.stub()
 				@s3Request.send.callsFake () =>
 					@s3EventHandlers.httpHeaders(200, {}, @s3Response, "OK")
 				@s3Response.httpResponse.createUnbufferedStream.returns(@expectedStream)