From d1e587a51ee074ae9ce27cb9ee76d827d4404cf9 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Fri, 21 Jun 2019 10:57:43 +0100 Subject: [PATCH] Merge pull request #1864 from overleaf/ta-analytics-backoff-strategy Update Analytics Backoff Strategy GitOrigin-RevId: 9a3dc11ee19ff03432730a36617208ac7f58c5be --- .../Features/Analytics/AnalyticsController.js | 63 ++---- .../Features/Analytics/AnalyticsManager.js | 182 ++++++++---------- .../src/Features/Analytics/AnalyticsProxy.js | 49 ++--- .../src/Features/Analytics/AnalyticsRouter.js | 9 +- .../infrastructure/FaultTolerantRequest.js | 85 ++++++++ .../src/Analytics/AnalyticsControllerTests.js | 45 ++--- .../src/Analytics/AnalyticsManagerTests.js | 115 +++++++++++ .../FaultTolerantRequestTests.js | 118 ++++++++++++ 8 files changed, 447 insertions(+), 219 deletions(-) create mode 100644 services/web/app/src/infrastructure/FaultTolerantRequest.js create mode 100644 services/web/test/unit/src/Analytics/AnalyticsManagerTests.js create mode 100644 services/web/test/unit/src/infrastructure/FaultTolerantRequestTests.js diff --git a/services/web/app/src/Features/Analytics/AnalyticsController.js b/services/web/app/src/Features/Analytics/AnalyticsController.js index af2fe87e7d..c471ea79b6 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsController.js +++ b/services/web/app/src/Features/Analytics/AnalyticsController.js @@ -1,39 +1,21 @@ -/* eslint-disable - camelcase, - handle-callback-err, - max-len, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * 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 - */ -let AnalyticsController const AnalyticsManager = require('./AnalyticsManager') const Errors = require('../Errors/Errors') const AuthenticationController = require('../Authentication/AuthenticationController') const InstitutionsAPI = require('../Institutions/InstitutionsAPI') const GeoIpLookup = require('../../infrastructure/GeoIpLookup') -module.exports = AnalyticsController = { +module.exports = { updateEditingSession(req, res, next) { const userId = AuthenticationController.getLoggedInUserId(req) const { projectId } = req.params let countryCode = null - if (userId != null) { - return GeoIpLookup.getDetails(req.ip, function(err, geoDetails) { - if ( - (geoDetails != null ? geoDetails.country_code : undefined) != null && - geoDetails.country_code !== '' - ) { + if (userId) { + GeoIpLookup.getDetails(req.ip, function(err, geoDetails) { + if (!err && geoDetails && geoDetails.country_code) { countryCode = geoDetails.country_code } - return AnalyticsManager.updateEditingSession( + AnalyticsManager.updateEditingSession( userId, projectId, countryCode, @@ -41,34 +23,29 @@ module.exports = AnalyticsController = { ) }) } else { - return res.send(204) + res.send(204) } }, recordEvent(req, res, next) { - const user_id = + const userId = AuthenticationController.getLoggedInUserId(req) || req.sessionID - return AnalyticsManager.recordEvent( - user_id, - req.params.event, - req.body, - error => respondWith(error, res, next) + AnalyticsManager.recordEvent(userId, req.params.event, req.body, error => + respondWith(error, res, next) ) }, licences(req, res, next) { - const { resource_id, start_date, end_date, lag } = req.query - return InstitutionsAPI.getInstitutionLicences( - resource_id, - start_date, - end_date, - lag, + InstitutionsAPI.getInstitutionLicences( + req.query.resource_id, + req.query.start_date, + req.query.end_date, + req.query.lag, function(error, licences) { - if (error != null) { + if (error) { return next(error) - } else { - return res.send(licences) } + res.send(licences) } ) } @@ -77,10 +54,10 @@ module.exports = AnalyticsController = { var respondWith = function(error, res, next) { if (error instanceof Errors.ServiceNotConfiguredError) { // ignore, no-op - return res.send(204) - } else if (error != null) { - return next(error) + res.send(204) + } else if (error) { + next(error) } else { - return res.send(204) + res.send(204) } } diff --git a/services/web/app/src/Features/Analytics/AnalyticsManager.js b/services/web/app/src/Features/Analytics/AnalyticsManager.js index ee212ea513..1018daca50 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsManager.js +++ b/services/web/app/src/Features/Analytics/AnalyticsManager.js @@ -1,119 +1,106 @@ -/* eslint-disable - camelcase, - handle-callback-err, - max-len, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS103: Rewrite code to no longer use __guard__ - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const settings = require('settings-sharelatex') const logger = require('logger-sharelatex') -const _ = require('underscore') -const request = require('requestretry') +const request = require('request') +const FaultTolerantRequest = require('../../infrastructure/FaultTolerantRequest') const Errors = require('../Errors/Errors') -const isProduction = - (process.env['NODE_ENV'] || '').toLowerCase() === 'production' -const isTest = process.env['MOCHA_GREP'] !== undefined - -const makeFaultTolerantRequest = function(userId, options, callback) { +// check that the request should be made: ignore smoke test user and ensure the +// analytics service is configured +const checkAnalyticsRequest = function(userId) { if ( - userId + '' === - (settings.smokeTest != null ? settings.smokeTest.userId : undefined) + '' + settings.smokeTest && + settings.smokeTest.userId && + settings.smokeTest.userId.toString() === userId.toString() ) { - return callback() + // ignore smoke test user + return { error: null, skip: true } } - options = Object.assign(options, { - delayStrategy: exponentialBackoffStrategy(), - timeout: 30000 - }) + if (!settings.apis.analytics) { + return { + error: new Errors.ServiceNotConfiguredError( + 'Analytics service not configured' + ), + skip: true + } + } + return { error: null, skip: false } +} + +// prepare the request: set `fromv2` param and full URL +const prepareAnalyticsRequest = function(options) { if (settings.overleaf != null) { options.qs = Object.assign({}, options.qs, { fromV2: 1 }) } - makeRequest(options, function(err) { - if (err != null) { - return logger.err({ err }, 'Request to analytics failed') - } - }) + const urlPath = options.url + options.url = `${settings.apis.analytics.url}${urlPath}` - return callback() // Do not wait for all the attempts + options.timeout = options.timeout || 30000 + + return options } -var makeRequest = function(opts, callback) { - if ( - __guard__( - settings.apis != null ? settings.apis.analytics : undefined, - x => x.url - ) != null - ) { - const urlPath = opts.url - opts.url = `${settings.apis.analytics.url}${urlPath}` - return request(opts, callback) - } else { - return callback( - new Errors.ServiceNotConfiguredError('Analytics service not configured') - ) +// make the request to analytics after checking and preparing it. +// request happens asynchronously in the background and will be retried on error +const makeAnalyticsBackgroundRequest = function(userId, options, callback) { + let { error, skip } = checkAnalyticsRequest(userId) + if (error || skip) { + return callback(error) } + prepareAnalyticsRequest(options) + + // With the tweaked parameter values (BACKOFF_BASE=3000, BACKOFF_MULTIPLIER=3): + // - the 6th attempt (maxAttempts=6) will run after 5.5 to 11.5 minutes + // - the 9th attempt (maxAttempts=9) will run after 86 to 250 minutes + options.maxAttempts = options.maxAttempts || 9 + options.backoffBase = options.backoffBase || 3000 + options.backoffMultiplier = options.backoffMultiplier || 3 + + FaultTolerantRequest.backgroundRequest(options, callback) } -// Set an exponential backoff to retry calls to analytics. First retry will -// happen after 4s, then 8, 16, 32, 64... -var exponentialBackoffStrategy = function() { - let attempts = 1 // This won't be called until there has been 1 failure - - return function() { - attempts += 1 - return exponentialBackoffDelay(attempts) +// make synchronous request to analytics without retries after checking and +// preparing it. +const makeAnalyticsRequest = function(userId, options, callback) { + let { error, skip } = checkAnalyticsRequest(userId) + if (error || skip) { + return callback(error) } -} + prepareAnalyticsRequest(options) -var exponentialBackoffDelay = function(attempts) { - const delay = Math.pow(2, attempts) * 1000 - - if (isProduction && !isTest) { - logger.warn( - 'Error comunicating with the analytics service. ' + - `Will try again attempt ${attempts} in ${delay}ms` - ) - } - - return delay + request(options, callback) } module.exports = { - identifyUser(user_id, old_user_id, callback) { - if (callback == null) { - callback = function(error) {} + identifyUser(userId, oldUserId, callback) { + if (!callback) { + // callback is optional + callback = () => {} } + const opts = { body: { - old_user_id + old_user_id: oldUserId }, json: true, method: 'POST', - timeout: 1000, - url: `/user/${user_id}/identify` + url: `/user/${userId}/identify` } - return makeRequest(opts, callback) + makeAnalyticsBackgroundRequest(userId, opts, callback) }, - recordEvent(user_id, event, segmentation, callback) { + recordEvent(userId, event, segmentation, callback) { if (segmentation == null) { + // segmentation is optional segmentation = {} } - if (callback == null) { - callback = function(error) {} + if (!callback) { + // callback is optional + callback = () => {} } + const opts = { body: { event, @@ -121,17 +108,18 @@ module.exports = { }, json: true, method: 'POST', - url: `/user/${user_id}/event`, - maxAttempts: 7 // Give up after ~ 8min + url: `/user/${userId}/event` } - return makeFaultTolerantRequest(user_id, opts, callback) + makeAnalyticsBackgroundRequest(userId, opts, callback) }, updateEditingSession(userId, projectId, countryCode, callback) { - if (callback == null) { - callback = function(error) {} + if (!callback) { + // callback is optional + callback = () => {} } + const query = { userId, projectId @@ -145,16 +133,14 @@ module.exports = { method: 'PUT', url: '/editingSession', qs: query, - maxAttempts: 6 // Give up after ~ 4min + maxAttempts: 6 // dont retry for too long as session ping timestamp are + // recorded when the request is received on the analytics } - return makeFaultTolerantRequest(userId, opts, callback) + makeAnalyticsBackgroundRequest(userId, opts, callback) }, - getLastOccurrence(user_id, event, callback) { - if (callback == null) { - callback = function(error) {} - } + getLastOccurrence(userId, event, callback) { const opts = { body: { event @@ -162,22 +148,16 @@ module.exports = { json: true, method: 'POST', timeout: 1000, - url: `/user/${user_id}/event/last_occurrence` + url: `/user/${userId}/event/last_occurrence` } - return makeRequest(opts, function(err, response, body) { + makeAnalyticsRequest(userId, opts, function(err, response, body) { if (err != null) { console.log(response, opts) - logger.err({ user_id, err }, 'error getting last occurance of event') - return callback(err) + logger.err({ userId, err }, 'error getting last occurance of event') + callback(err) } else { - return callback(null, body) + callback(null, body) } }) } } - -function __guard__(value, transform) { - return typeof value !== 'undefined' && value !== null - ? transform(value) - : undefined -} diff --git a/services/web/app/src/Features/Analytics/AnalyticsProxy.js b/services/web/app/src/Features/Analytics/AnalyticsProxy.js index 9510025a16..bc6b0c3f0b 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsProxy.js +++ b/services/web/app/src/Features/Analytics/AnalyticsProxy.js @@ -1,15 +1,3 @@ -/* eslint-disable - max-len, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS103: Rewrite code to no longer use __guard__ - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const settings = require('settings-sharelatex') const Errors = require('../Errors/Errors') const httpProxy = require('express-http-proxy') @@ -17,25 +5,7 @@ const URL = require('url') module.exports = { call(basePath) { - const analyticsUrl = __guard__( - __guard__( - settings != null ? settings.apis : undefined, - x1 => x1.analytics - ), - x => x.url - ) - if (analyticsUrl != null) { - return httpProxy(analyticsUrl, { - proxyReqPathResolver(req) { - const requestPath = URL.parse(req.url).path - return `${basePath}${requestPath}` - }, - proxyReqOptDecorator(proxyReqOpts, srcReq) { - proxyReqOpts.headers = {} // unset all headers - return proxyReqOpts - } - }) - } else { + if (!settings.apis.analytics) { return (req, res, next) => next( new Errors.ServiceNotConfiguredError( @@ -43,11 +13,16 @@ module.exports = { ) ) } + + return httpProxy(settings.apis.analytics.url, { + proxyReqPathResolver(req) { + const requestPath = URL.parse(req.url).path + return `${basePath}${requestPath}` + }, + proxyReqOptDecorator(proxyReqOpts, srcReq) { + proxyReqOpts.headers = {} // unset all headers + return proxyReqOpts + } + }) } } - -function __guard__(value, transform) { - return typeof value !== 'undefined' && value !== null - ? transform(value) - : undefined -} diff --git a/services/web/app/src/Features/Analytics/AnalyticsRouter.js b/services/web/app/src/Features/Analytics/AnalyticsRouter.js index 6ffcf51704..d24e95e1b2 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsRouter.js +++ b/services/web/app/src/Features/Analytics/AnalyticsRouter.js @@ -1,10 +1,3 @@ -// TODO: This file was created by bulk-decaffeinate. -// Sanity-check the conversion and remove this comment. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const AuthenticationController = require('./../Authentication/AuthenticationController') const AnalyticsController = require('./AnalyticsController') const AnalyticsProxy = require('./AnalyticsProxy') @@ -36,7 +29,7 @@ module.exports = { AnalyticsProxy.call('/recentV1TemplateIdsActivity') ) - return publicApiRouter.use( + publicApiRouter.use( '/analytics/uniExternalCollaboration', AuthenticationController.httpAuth, AnalyticsProxy.call('/uniExternalCollaboration') diff --git a/services/web/app/src/infrastructure/FaultTolerantRequest.js b/services/web/app/src/infrastructure/FaultTolerantRequest.js new file mode 100644 index 0000000000..ed5ed398d3 --- /dev/null +++ b/services/web/app/src/infrastructure/FaultTolerantRequest.js @@ -0,0 +1,85 @@ +const logger = require('logger-sharelatex') +const request = require('requestretry') + +const isProduction = + (process.env['NODE_ENV'] || '').toLowerCase() === 'production' +const isTest = process.env['MOCHA_GREP'] !== undefined + +const BACKOFF_MAX_TRIES = 3 +const BACKOFF_BASE = 500 +const BACKOFF_MULTIPLIER = 1.5 +const BACKOFF_RANDOM_FACTOR = 0.5 + +// +// Use an exponential backoff to retry requests +// +// This is based on what the Google HTTP client does: +// https://developers.google.com/api-client-library/java/google-http-java-client/reference/1.20.0/com/google/api/client/util/ExponentialBackOff +// +let FaultTolerantRequest +module.exports = FaultTolerantRequest = { + request: function(options, callback) { + options = Object.assign( + { + maxAttempts: BACKOFF_MAX_TRIES, + backoffBase: BACKOFF_BASE, + backoffMultiplier: BACKOFF_MULTIPLIER, + backoffRandomFactor: BACKOFF_RANDOM_FACTOR + }, + options + ) + + options.delayStrategy = FaultTolerantRequest.exponentialDelayStrategy( + options.backoffBase, + options.backoffMultiplier, + options.backoffRandomFactor + ) + + request(options, callback) + }, + + backgroundRequest: function(options, callback) { + FaultTolerantRequest.request(options, function(err) { + if (err) { + return logger.err( + { err, url: options.url, query: options.qs, body: options.body }, + 'Background request failed' + ) + } + }) + + callback() // Do not wait for all the attempts + }, + + exponentialDelayStrategy: function( + backoffBase, + backoffMultiplier, + backoffRandomFactor + ) { + let backoff = backoffBase + + return function() { + const delay = exponentialDelay(backoff, backoffRandomFactor) + backoff *= backoffMultiplier + return delay + } + } +} + +function exponentialDelay(backoff, backoffRandomFactor) { + // set delay to `backoff` initially + let delay = backoff + + // adds randomness + delay *= 1 - backoffRandomFactor + 2 * Math.random() * backoffRandomFactor + + // round value as it's already in milliseconds + delay = Math.round(delay) + + // log retries in production + if (isProduction && !isTest) { + logger.warn(`Background request failed. Will try again in ${delay}ms`) + } + + return delay +} diff --git a/services/web/test/unit/src/Analytics/AnalyticsControllerTests.js b/services/web/test/unit/src/Analytics/AnalyticsControllerTests.js index dec9816bd3..3d905b8900 100644 --- a/services/web/test/unit/src/Analytics/AnalyticsControllerTests.js +++ b/services/web/test/unit/src/Analytics/AnalyticsControllerTests.js @@ -1,25 +1,10 @@ -/* eslint-disable - max-len, - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const should = require('chai').should() const SandboxedModule = require('sandboxed-module') -const assert = require('assert') const path = require('path') const modulePath = path.join( __dirname, '../../../../app/src/Features/Analytics/AnalyticsController' ) const sinon = require('sinon') -const { expect } = require('chai') describe('AnalyticsController', function() { beforeEach(function() { @@ -49,7 +34,7 @@ describe('AnalyticsController', function() { } }) - return (this.res = { send() {} }) + this.res = { send() {} } }) describe('updateEditingSession', function() { @@ -59,32 +44,32 @@ describe('AnalyticsController', function() { projectId: 'a project id' } } - return (this.GeoIpLookup.getDetails = sinon + this.GeoIpLookup.getDetails = sinon .stub() - .callsArgWith(1, null, { country_code: 'XY' })) + .callsArgWith(1, null, { country_code: 'XY' }) }) - return it('delegates to the AnalyticsManager', function(done) { + it('delegates to the AnalyticsManager', function(done) { this.AuthenticationController.getLoggedInUserId.returns('1234') this.controller.updateEditingSession(this.req, this.res) this.AnalyticsManager.updateEditingSession .calledWith('1234', 'a project id', 'XY') .should.equal(true) - return done() + done() }) }) describe('recordEvent', function() { beforeEach(function() { - return (this.req = { + this.req = { params: { event: 'i_did_something' }, body: 'stuff', sessionID: 'sessionIDHere', session: {} - }) + } }) it('should use the user_id', function(done) { @@ -93,21 +78,21 @@ describe('AnalyticsController', function() { this.AnalyticsManager.recordEvent .calledWith('1234', this.req.params['event'], this.req.body) .should.equal(true) - return done() + done() }) - return it('should use the session id', function(done) { + it('should use the session id', function(done) { this.controller.recordEvent(this.req, this.res) this.AnalyticsManager.recordEvent .calledWith(this.req.sessionID, this.req.params['event'], this.req.body) .should.equal(true) - return done() + done() }) }) - return describe('licences', function() { + describe('licences', function() { beforeEach(function() { - return (this.req = { + this.req = { query: { resource_id: 1, start_date: '1514764800', @@ -116,10 +101,10 @@ describe('AnalyticsController', function() { }, sessionID: 'sessionIDHere', session: {} - }) + } }) - return it('should trigger institutions api to fetch licences graph data', function(done) { + it('should trigger institutions api to fetch licences graph data', function(done) { this.controller.licences(this.req, this.res) this.InstitutionsAPI.getInstitutionLicences .calledWith( @@ -129,7 +114,7 @@ describe('AnalyticsController', function() { this.req.query['lag'] ) .should.equal(true) - return done() + done() }) }) }) diff --git a/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js b/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js new file mode 100644 index 0000000000..72d768856d --- /dev/null +++ b/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js @@ -0,0 +1,115 @@ +const SandboxedModule = require('sandboxed-module') +const path = require('path') +const modulePath = path.join( + __dirname, + '../../../../app/src/Features/Analytics/AnalyticsManager' +) +const sinon = require('sinon') +const { expect } = require('chai') +const Errors = require('../../../../app/src/Features/Errors/Errors') + +describe('AnalyticsManager', function() { + beforeEach(function() { + this.fakeUserId = '123abc' + this.settings = { + overleaf: true, + apis: { analytics: { url: 'analytics.test' } } + } + this.backgroundRequest = sinon.stub().yields() + this.request = sinon.stub().yields() + this.AnalyticsManager = SandboxedModule.require(modulePath, { + requires: { + 'settings-sharelatex': this.settings, + '../../infrastructure/FaultTolerantRequest': { + backgroundRequest: this.backgroundRequest + }, + '../Errors/Errors': Errors, + request: this.request, + 'logger-sharelatex': { + log() {} + } + } + }) + }) + + describe('checkAnalyticsRequest', function() { + it('ignores smoke test user', function(done) { + this.settings.smokeTest = { userId: this.fakeUserId } + this.AnalyticsManager.identifyUser(this.fakeUserId, '', error => { + expect(error).to.not.exist + sinon.assert.notCalled(this.request) + done() + }) + }) + + it('return error if analytics service is not configured', function(done) { + this.settings.apis.analytics = null + this.AnalyticsManager.identifyUser(this.fakeUserId, '', error => { + expect(error).to.be.instanceof(Errors.ServiceNotConfiguredError) + sinon.assert.notCalled(this.request) + done() + }) + }) + }) + + describe('makes correct request to analytics', function() { + it('identifyUser', function(done) { + const oldUserId = '456def' + this.AnalyticsManager.identifyUser(this.fakeUserId, oldUserId, error => { + expect(error).to.not.exist + sinon.assert.calledWithMatch(this.backgroundRequest, { + body: { old_user_id: oldUserId }, + url: 'analytics.test/user/123abc/identify' + }) + done() + }) + }) + + it('recordEvent', function(done) { + const event = 'fake-event' + this.AnalyticsManager.recordEvent(this.fakeUserId, event, null, error => { + expect(error).to.not.exist + sinon.assert.calledWithMatch(this.backgroundRequest, { + body: { event }, + qs: { fromV2: 1 }, + url: 'analytics.test/user/123abc/event', + timeout: 30000, + maxAttempts: 9, + backoffBase: 3000, + backoffMultiplier: 3 + }) + done() + }) + }) + + it('updateEditingSession', function(done) { + const projectId = '789ghi' + const countryCode = 'fr' + this.AnalyticsManager.updateEditingSession( + this.fakeUserId, + projectId, + countryCode, + error => { + expect(error).to.not.exist + sinon.assert.calledWithMatch(this.backgroundRequest, { + qs: { userId: this.fakeUserId, projectId, countryCode, fromV2: 1 }, + url: 'analytics.test/editingSession' + }) + done() + } + ) + }) + + it('getLastOccurrence', function(done) { + const event = 'fake-event' + this.AnalyticsManager.getLastOccurrence(this.fakeUserId, event, error => { + expect(error).to.not.exist + sinon.assert.calledWithMatch(this.request, { + body: { event }, + url: 'analytics.test/user/123abc/event/last_occurrence' + }) + done() + }) + }) + }) +}) diff --git a/services/web/test/unit/src/infrastructure/FaultTolerantRequestTests.js b/services/web/test/unit/src/infrastructure/FaultTolerantRequestTests.js new file mode 100644 index 0000000000..a2d9b7818a --- /dev/null +++ b/services/web/test/unit/src/infrastructure/FaultTolerantRequestTests.js @@ -0,0 +1,118 @@ +const SandboxedModule = require('sandboxed-module') +const path = require('path') +const modulePath = path.join( + __dirname, + '../../../../app/src/infrastructure/FaultTolerantRequest' +) +const sinon = require('sinon') +const { expect } = require('chai') + +describe('FaultTolerantRequest', function() { + beforeEach(function() { + this.request = sinon.stub().yields() + this.logger = { + err: sinon.stub() + } + this.FaultTolerantRequest = SandboxedModule.require(modulePath, { + requires: { + requestretry: this.request, + 'logger-sharelatex': this.logger + } + }) + }) + + describe('exponentialBackoffStrategy', function() { + it('returns delays within expected range with default options', function() { + this.FaultTolerantRequest.request({}, () => {}) + const delayStrategy = this.request.lastCall.args[0].delayStrategy + expect(delayStrategy()).to.be.closeTo(500, 250) // attempt 1 + expect(delayStrategy()).to.be.closeTo(750, 375) // attempt 2 + expect(delayStrategy()).to.be.closeTo(1125, 563) // attempt 3 + expect(delayStrategy()).to.be.closeTo(1688, 844) // attempt 4 + expect(delayStrategy()).to.be.closeTo(2531, 1266) // attempt 5 + expect(delayStrategy()).to.be.closeTo(3797, 1899) // attempt 6 + expect(delayStrategy()).to.be.closeTo(5695, 2848) // attempt 7 + expect(delayStrategy()).to.be.closeTo(8543, 4272) // attempt 8 + expect(delayStrategy()).to.be.closeTo(12814, 6408) // attempt 9 + expect(delayStrategy()).to.be.closeTo(19222, 9610) // attempt 10 + }) + + it('returns delays within expected range with custom options', function() { + const delayStrategy = this.FaultTolerantRequest.exponentialDelayStrategy( + 3000, + 3, + 0.5 + ) + expect(delayStrategy()).to.be.closeTo(3000, 1500) // attempt 1 + expect(delayStrategy()).to.be.closeTo(9000, 4500) // attempt 2 + expect(delayStrategy()).to.be.closeTo(27000, 13500) // attempt 3 + expect(delayStrategy()).to.be.closeTo(81000, 40500) // attempt 4 + expect(delayStrategy()).to.be.closeTo(243000, 121500) // attempt 5 + expect(delayStrategy()).to.be.closeTo(729000, 364500) // attempt 6 + expect(delayStrategy()).to.be.closeTo(2187000, 1093500) // attempt 7 + expect(delayStrategy()).to.be.closeTo(6561000, 3280500) // attempt 8 + expect(delayStrategy()).to.be.closeTo(19683000, 9841500) // attempt 9 + expect(delayStrategy()).to.be.closeTo(59049000, 29524500) // attempt 10 + }) + }) + + describe('request', function() { + it('sets retry options', function(done) { + this.FaultTolerantRequest.request({}, error => { + expect(error).to.not.exist + sinon.assert.calledOnce(this.request) + + const { delayStrategy, maxAttempts } = this.request.lastCall.args[0] + expect(delayStrategy).to.be.a('function') + expect(maxAttempts).to.be.a('number') + done() + }) + }) + + it("don't overwrite retry options", function(done) { + const customMaxAttempts = Math.random() + const customBase = Math.random() + const customMultiplier = Math.random() + const customRandomFactor = Math.random() + this.FaultTolerantRequest.request( + { + maxAttempts: customMaxAttempts, + backoffBase: customBase, + backoffMultiplier: customMultiplier, + backoffRandomFactor: customRandomFactor + }, + error => { + expect(error).to.not.exist + + const { + maxAttempts, + backoffBase, + backoffMultiplier, + backoffRandomFactor + } = this.request.lastCall.args[0] + expect(maxAttempts).to.equal(customMaxAttempts) + expect(backoffBase).to.equal(customBase) + expect(backoffMultiplier).to.equal(customMultiplier) + expect(backoffRandomFactor).to.equal(customRandomFactor) + done() + } + ) + }) + }) + + describe('backgroundRequest', function() { + it('logs error in the background', function(done) { + this.request.yields(new Error('Nope')) + this.logger.err = (options, message) => { + expect(options.url).to.equal('test.url') + done() + } + this.FaultTolerantRequest.backgroundRequest( + { url: 'test.url' }, + error => { + expect(error).to.not.exist + } + ) + }) + }) +})