From d7c14e59a233250cb2ff698c7e5eeb748491610f Mon Sep 17 00:00:00 2001 From: Andrew Ledvina Date: Mon, 12 Jun 2017 14:52:35 -0700 Subject: [PATCH 1/4] add back verbose logging to console.error when we are about to log an exception --- src/browser/rollbar.js | 1 + src/queue.js | 17 +++++++- src/rollbar.js | 2 +- src/server/rollbar.js | 1 + test/queue.test.js | 96 ++++++++++++++++++++++++++++++++++-------- 5 files changed, 98 insertions(+), 19 deletions(-) diff --git a/src/browser/rollbar.js b/src/browser/rollbar.js index bad1242b0..f30f5190f 100644 --- a/src/browser/rollbar.js +++ b/src/browser/rollbar.js @@ -291,6 +291,7 @@ var defaultOptions = { reportLevel: __DEFAULT_REPORT_LEVEL__, uncaughtErrorLevel: __DEFAULT_UNCAUGHT_ERROR_LEVEL, endpoint: __DEFAULT_ENDPOINT__, + verbose: false, enabled: true }; diff --git a/src/queue.js b/src/queue.js index 96081e30a..9a70bd40d 100644 --- a/src/queue.js +++ b/src/queue.js @@ -10,11 +10,13 @@ var _ = require('./utility'); * rateLimiter.shouldSend(item) -> bool * @param api - An object which conforms to the interface * api.postItem(payload, function(err, response)) + * @param logger - An object used to log verbose messages if desired * @param options - see Queue.prototype.configure */ -function Queue(rateLimiter, api, options) { +function Queue(rateLimiter, api, logger, options) { this.rateLimiter = rateLimiter; this.api = api; + this.logger = logger; this.options = options; this.predicates = []; this.pendingRequests = []; @@ -73,6 +75,7 @@ Queue.prototype.addItem = function(item, callback) { callback(); return; } + this._maybeLog(item); this.pendingRequests.push(item); try { this._makeApiRequest(item, function(err, resp) { @@ -212,4 +215,16 @@ Queue.prototype._dequeuePendingRequest = function(item) { } }; +Queue.prototype._maybeLog = function(item) { + if (this.logger && this.options.verbose) { + var message = _.get(item, 'data.body.trace.exception.message'); + if (!message) { + message = _.get(item, 'data.body.trace_chain.0.exception.message'); + } + if (message) { + this.logger.error(message); + } + } +}; + module.exports = Queue; diff --git a/src/rollbar.js b/src/rollbar.js index 1b30ede3c..6d487cc40 100644 --- a/src/rollbar.js +++ b/src/rollbar.js @@ -13,7 +13,7 @@ var _ = require('./utility'); function Rollbar(options, api, logger) { this.options = _.extend(true, {}, options); this.logger = logger; - this.queue = new Queue(Rollbar.rateLimiter, api, this.options); + this.queue = new Queue(Rollbar.rateLimiter, api, logger, this.options); this.notifier = new Notifier(this.queue, this.options); } diff --git a/src/server/rollbar.js b/src/server/rollbar.js index e8deef093..18b28db55 100644 --- a/src/server/rollbar.js +++ b/src/server/rollbar.js @@ -484,6 +484,7 @@ Rollbar.defaultOptions = { scrubFields: packageJson.defaults.server.scrubFields, addRequestData: null, reportLevel: packageJson.defaults.reportLevel, + verbose: false, enabled: true }; diff --git a/test/queue.test.js b/test/queue.test.js index 555145fba..7a213f7cb 100644 --- a/test/queue.test.js +++ b/test/queue.test.js @@ -41,12 +41,34 @@ function TestApiGenerator() { return TestApi; } +function TestLoggerGenerator() { + var TestLogger = function() { + this.calls = { + log: [], + error: [], + info: [] + }; + }; + TestLogger.prototype.log = function() { + this.calls.log.push(arguments); + }; + TestLogger.prototype.error = function() { + this.calls.error.push(arguments); + }; + TestLogger.prototype.info = function() { + this.calls.info.push(arguments); + }; + return TestLogger; +}; + + describe('Queue()', function() { it('should have all of the expected methods', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); expect(queue).to.have.property('configure'); expect(queue).to.have.property('addPredicate'); expect(queue).to.have.property('addItem'); @@ -60,8 +82,9 @@ describe('configure', function() { it('should update the options', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {a: 1, b: 42}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); expect(queue.options.a).to.eql(1); expect(queue.options.b).to.eql(42); @@ -82,8 +105,9 @@ describe('addItem', function() { it('should work with no callback', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -102,8 +126,9 @@ describe('addItem', function() { it('should work with a garbage callback', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -122,8 +147,9 @@ describe('addItem', function() { it('should work with no predicates', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -141,11 +167,36 @@ describe('addItem', function() { done(err); }); }); + it('should call the logger if an error is about to be logged', function(done) { + var rateLimiter = new (TestRateLimiterGenerator())(); + var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); + var options = {verbose: true}; + var queue = new Queue(rateLimiter, api, logger, options); + + var item = {data: {body: {trace: {exception: {message: 'hello'}}}}}; + var serverResponse = {success: true}; + + rateLimiter.handler = function(i) { + expect(i).to.eql(item); + return {error: null, shouldSend: true, payload: null}; + }; + api.handler = function(i, cb) { + expect(i).to.eql(item); + cb(null, serverResponse); + }; + queue.addItem(item, function(err, resp) { + expect(resp).to.eql(serverResponse); + expect(logger.calls.error[0][0]).to.eql('hello'); + done(err); + }); + }); it('should stop if a predicate returns false', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -169,8 +220,9 @@ describe('addItem', function() { it('should stop if a predicate returns an error', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -196,8 +248,9 @@ describe('addItem', function() { it('should stop if any predicate returns an error', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -227,8 +280,9 @@ describe('addItem', function() { it('should stop and callback if a wait callback is set', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -254,8 +308,9 @@ describe('addItem', function() { it('should work if wait is called with a non-function', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -275,8 +330,9 @@ describe('addItem', function() { it('should work if all predicates return true', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -304,8 +360,9 @@ describe('addItem', function() { it('should callback if the api throws an exception', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var exception = 'boom!'; @@ -331,8 +388,9 @@ describe('addItem', function() { it('should callback with the api error if not retriable', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {retryInterval: 1}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var apiError = {code: 'NOPE', message: 'borked'}; @@ -352,8 +410,9 @@ describe('addItem', function() { it('should callback with the api error if no retryInterval is set', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var apiError = {code: 'ENOTFOUND', message: 'No internet connection'}; @@ -373,8 +432,9 @@ describe('addItem', function() { it('should retry if we get a retriable error', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {retryInterval: 1}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -405,8 +465,9 @@ describe('addItem', function() { it('should callback if the rate limiter says not to send and has an error', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var rateLimitError = 'bork'; @@ -432,8 +493,9 @@ describe('addItem', function() { it('should callback if the rate limiter says not to send and has a payload', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); var options = {}; - var queue = new Queue(rateLimiter, api, options); + var queue = new Queue(rateLimiter, api, logger, options); var item = {mykey: 'myvalue'}; var rateLimitPayload = {something: 'went wrong'}; From 989ea5a1b21dd9d8f8f8713031eb3dcddc621f7c Mon Sep 17 00:00:00 2001 From: Andrew Ledvina Date: Thu, 15 Jun 2017 11:30:32 -0700 Subject: [PATCH 2/4] also log messages if the item does not have an exception --- src/queue.js | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/queue.js b/src/queue.js index 9a70bd40d..9d22e1660 100644 --- a/src/queue.js +++ b/src/queue.js @@ -216,14 +216,20 @@ Queue.prototype._dequeuePendingRequest = function(item) { }; Queue.prototype._maybeLog = function(item) { - if (this.logger && this.options.verbose) { - var message = _.get(item, 'data.body.trace.exception.message'); - if (!message) { - message = _.get(item, 'data.body.trace_chain.0.exception.message'); - } - if (message) { - this.logger.error(message); - } + if (!this.logger || !this.options.verbose) { + return; + } + + var message = _.get(item, 'data.body.trace.exception.message'); + message = message || _.get(item, 'data.body.trace_chain.0.exception.message'); + if (message) { + this.logger.error(message); + return; + } + + message = _.get(item, 'data.body.message.body'); + if (messagge) { + this.logger.log(message); } }; From 9c43b4e8cd97dd0cb97cd2dd6dbed50099c18831 Mon Sep 17 00:00:00 2001 From: Andrew Ledvina Date: Thu, 15 Jun 2017 11:37:59 -0700 Subject: [PATCH 3/4] test for message and for verbose:false, cleanup whitespace --- src/queue.js | 2 +- test/queue.test.js | 84 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 67 insertions(+), 19 deletions(-) diff --git a/src/queue.js b/src/queue.js index 9d22e1660..c38b3f99b 100644 --- a/src/queue.js +++ b/src/queue.js @@ -228,7 +228,7 @@ Queue.prototype._maybeLog = function(item) { } message = _.get(item, 'data.body.message.body'); - if (messagge) { + if (message) { this.logger.log(message); } }; diff --git a/test/queue.test.js b/test/queue.test.js index 7a213f7cb..0121e9f32 100644 --- a/test/queue.test.js +++ b/test/queue.test.js @@ -37,7 +37,7 @@ function TestApiGenerator() { }; TestApi.prototype.configure = function() {}; - + return TestApi; } @@ -85,7 +85,7 @@ describe('configure', function() { var logger = new (TestLoggerGenerator())(); var options = {a: 1, b: 42}; var queue = new Queue(rateLimiter, api, logger, options); - + expect(queue.options.a).to.eql(1); expect(queue.options.b).to.eql(42); @@ -108,7 +108,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -129,7 +129,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -150,7 +150,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -173,7 +173,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {verbose: true}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {data: {body: {trace: {exception: {message: 'hello'}}}}}; var serverResponse = {success: true}; @@ -191,13 +191,61 @@ describe('addItem', function() { done(err); }); }); + it('should call the logger if a message is about to be logged', function(done) { + var rateLimiter = new (TestRateLimiterGenerator())(); + var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); + var options = {verbose: true}; + var queue = new Queue(rateLimiter, api, logger, options); + + var item = {data: {body: {message: {body: 'hello'}}}}; + var serverResponse = {success: true}; + + rateLimiter.handler = function(i) { + expect(i).to.eql(item); + return {error: null, shouldSend: true, payload: null}; + }; + api.handler = function(i, cb) { + expect(i).to.eql(item); + cb(null, serverResponse); + }; + queue.addItem(item, function(err, resp) { + expect(resp).to.eql(serverResponse); + expect(logger.calls.log[0][0]).to.eql('hello'); + done(err); + }); + }); + it('should not call the logger if verbose is false', function(done) { + var rateLimiter = new (TestRateLimiterGenerator())(); + var api = new (TestApiGenerator())(); + var logger = new (TestLoggerGenerator())(); + var options = {verbose: false}; + var queue = new Queue(rateLimiter, api, logger, options); + + var item = {data: {body: {message: {body: 'hello'}}}}; + var serverResponse = {success: true}; + + rateLimiter.handler = function(i) { + expect(i).to.eql(item); + return {error: null, shouldSend: true, payload: null}; + }; + api.handler = function(i, cb) { + expect(i).to.eql(item); + cb(null, serverResponse); + }; + queue.addItem(item, function(err, resp) { + expect(resp).to.eql(serverResponse); + expect(logger.calls.log.length).to.eql(0); + done(err); + }); + }); it('should stop if a predicate returns false', function(done) { var rateLimiter = new (TestRateLimiterGenerator())(); var api = new (TestApiGenerator())(); var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -223,7 +271,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -251,7 +299,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -283,7 +331,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -311,7 +359,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -333,7 +381,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; @@ -363,7 +411,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var exception = 'boom!'; @@ -391,7 +439,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {retryInterval: 1}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var apiError = {code: 'NOPE', message: 'borked'}; @@ -413,7 +461,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var apiError = {code: 'ENOTFOUND', message: 'No internet connection'}; @@ -435,7 +483,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {retryInterval: 1}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var serverResponse = {success: true}; var apiError = {code: 'ENOTFOUND', message: 'No internet connection'}; @@ -468,7 +516,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var rateLimitError = 'bork'; @@ -496,7 +544,7 @@ describe('addItem', function() { var logger = new (TestLoggerGenerator())(); var options = {}; var queue = new Queue(rateLimiter, api, logger, options); - + var item = {mykey: 'myvalue'}; var rateLimitPayload = {something: 'went wrong'}; var serverResponse = {message: 'good times'}; From dc4f8e3688941b49179f2a16ea8658e66ce22b5b Mon Sep 17 00:00:00 2001 From: Andrew Ledvina Date: Thu, 15 Jun 2017 12:22:57 -0700 Subject: [PATCH 4/4] fake reduce cyclomatic complexity --- src/queue.js | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/queue.js b/src/queue.js index c38b3f99b..4ef73e8c6 100644 --- a/src/queue.js +++ b/src/queue.js @@ -39,7 +39,7 @@ Queue.prototype.configure = function(options) { /* * addPredicate - adds a predicate to the end of the list of predicates for this queue - * + * * @param predicate - function(item, options) -> (bool|{err: Error}) * Returning true means that this predicate passes and the item is okay to go on the queue * Returning false means do not add the item to the queue, but it is not an error @@ -216,20 +216,17 @@ Queue.prototype._dequeuePendingRequest = function(item) { }; Queue.prototype._maybeLog = function(item) { - if (!this.logger || !this.options.verbose) { - return; - } - - var message = _.get(item, 'data.body.trace.exception.message'); - message = message || _.get(item, 'data.body.trace_chain.0.exception.message'); - if (message) { - this.logger.error(message); - return; - } - - message = _.get(item, 'data.body.message.body'); - if (message) { - this.logger.log(message); + if (this.logger && this.options.verbose) { + var message = _.get(item, 'data.body.trace.exception.message'); + message = message || _.get(item, 'data.body.trace_chain.0.exception.message'); + if (message) { + this.logger.error(message); + return; + } + message = _.get(item, 'data.body.message.body'); + if (message) { + this.logger.log(message); + } } };