From babff20cab7ede36b518bd7b2cb13f64f6f00b82 Mon Sep 17 00:00:00 2001 From: Gal Neugroschl Date: Fri, 16 Jun 2017 17:57:51 -0700 Subject: [PATCH] Reduce rollbar noise connects soxhub/qa#1301 Adds callback option to configure which errors get sent to rollbar --- .eslintignore | 4 + .eslintrc | 53 ++++++++++++++ lib/index.js | 33 +++++---- package-lock.json | 2 +- package.json | 2 +- test/index.js | 183 ++++++++++++++++++++++++++++++++++++++++------ 6 files changed, 237 insertions(+), 40 deletions(-) create mode 100644 .eslintignore create mode 100644 .eslintrc diff --git a/.eslintignore b/.eslintignore new file mode 100644 index 0000000..a28f43c --- /dev/null +++ b/.eslintignore @@ -0,0 +1,4 @@ +node_modules +db +config +bin \ No newline at end of file diff --git a/.eslintrc b/.eslintrc new file mode 100644 index 0000000..a633b60 --- /dev/null +++ b/.eslintrc @@ -0,0 +1,53 @@ +{ + "rules": { + "indent": [ + 2, + "tab" + ], + "linebreak-style": [ + 2, + "unix" + ], + "semi": [ + 2, + "always" + ], + "no-unused-vars": [ + 2, + {"args": "after-used", + "argsIgnorePattern": "^_"} + ], + "one-var": [ + 2, + "never" + ], + "no-multiple-empty-lines": [ + 1, + {"max": 1} + ], + "padded-blocks": [ + 1, + "never" + ], + "no-trailing-spaces": [ + 1, + { "skipBlankLines": true } + ], + "no-spaced-func": [ + 2 + ], + "strict": ["error"] + + }, + "parserOptions": { + "ecmaVersion": 8 + }, + "env": { + "es6": true, + "node": true + }, + "globals": { + "requireRoot": true + }, + "extends": "eslint:recommended" +} diff --git a/lib/index.js b/lib/index.js index 8f1bf52..a5ab515 100644 --- a/lib/index.js +++ b/lib/index.js @@ -34,11 +34,19 @@ * *******************************************************************/ // Load modules -var Rollbar = require('rollbar'); -var util = require('util'); +const Rollbar = require('rollbar'); +const util = require('util'); exports.register = function (server, options, next) { - var preError = options.preError || function preError(err) { return err; }; + // The sanitize function is called with the exception that will be sent + // to rollbar. Its cleaned output will be sent to rollbar instead of + // the original exception. + var sanitize = options.sanitize || function identity(err) { return err; }; + + // The filter function will be called before sending any 4XX response + // to rollbar. If a truthy value is returned, the error will be + // forwarded to rollbar; when false, the error is not sent to rollbar. + var filter = options.filter || function never_filter() { return true; }; var rollbarOpts = options; rollbarOpts.environment = rollbarOpts.environment || process.env.NODE_ENV; @@ -47,19 +55,15 @@ exports.register = function (server, options, next) { var rollbarAccessToken = rollbarOpts.accessToken; rollbarOpts.enabled = !!rollbarAccessToken; - var rollbar = new Rollbar({ - accessToken: rollbarOpts.accessToken, - environment: rollbarOpts.environment, - handleUncaughtExceptions: rollbarOpts.handleUncaughtExceptions, - enabled: rollbarOpts.enabled - }); - + var rollbar = new Rollbar(rollbarOpts); server.ext('onPreResponse', function onPreResponse(request, next) { - var response = preError(request.response); - if (response && response.isBoom) { - rollbar.error(response, request, response.output, logError); - } + let response = request.response; + + response.isBoom && + filter(response) && + rollbar.error(sanitize(response), request, response.output, logError); + next.continue(); }); @@ -75,6 +79,7 @@ exports.register = function (server, options, next) { server.expose('rollbar', rollbar); + next(); }; exports.register.attributes = { diff --git a/package-lock.json b/package-lock.json index 59f177b..8465b82 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "rollbar-hapi", - "version": "0.0.9", + "version": "0.0.10", "lockfileVersion": 1, "dependencies": { "accept": { diff --git a/package.json b/package.json index 4d8f7cc..364967d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "rollbar-hapi", - "version": "0.0.9", + "version": "0.0.10", "description": "A Hapi plugin for rollbar painless integration", "author": "Evangelos Pappas ", "engine": "node >= 0.10.x", diff --git a/test/index.js b/test/index.js index 29010a4..3c653ad 100644 --- a/test/index.js +++ b/test/index.js @@ -1,3 +1,5 @@ +'use strict'; + const Lab = require('lab'); const lab = exports.lab = Lab.script(); const Hapi = require('hapi'); @@ -8,7 +10,7 @@ var expect = require('chai').expect; lab.experiment('plugin exposes', function() { lab.test('should expose rollbar', function(done) { var server = makeServerWithPlugin(); - expect(server.plugins['rollbar-hapi'].rollbar).to.not.be.empty; + expect(server.plugins['rollbar-hapi'].rollbar).to.not.be.empty; done(); }); @@ -19,12 +21,10 @@ lab.experiment('plugin exposes', function() { expect(rollbar.error).to.be.a('function'); done(); }); - }); lab.experiment('plugin relays server errors to rollbar', function() { lab.test('should relay http state errors', function(done) { - var server = makeServerWithPlugin(); // Add a basic route and register a cookie definition @@ -43,8 +43,8 @@ lab.experiment('plugin relays server errors to rollbar', function() { encoding: 'base64json' }); - // This request with an improperly encoded `session` cookie value - // will trigger a state error and emit an internal-error event + // This request with an improperly encoded `session` cookie value + // will trigger a state error and emit an internal-error event const request = { method: 'GET', url: '/good', @@ -54,29 +54,26 @@ lab.experiment('plugin relays server errors to rollbar', function() { } }; - // Wrap `rollbar.warning` with a spy let rollbar = server.plugins['rollbar-hapi'].rollbar; - let spy = sinon.stub(rollbar,'warning') + let spy = sinon.stub(rollbar,'warning'); // The emitting of the `tail` event is the last step in the hapi request cycle // (https://hapijs.com/api#request-lifecycle) and the ideal place to check the // state of any activity that occurs after the response is sent to the client // but before the request cycle completes. - server.on('tail', function(request) { + server.on('tail', function() { sinon.assert.calledOnce(spy); var firstArg = spy.firstCall.args[0]; expect(firstArg).to.be.a('string'); spy.restore(); done(); }); - - server.inject(request); + server.inject(request); }); lab.test('should relay internal errors', function(done) { - var server = makeServerWithPlugin(); let rollbar = server.plugins['rollbar-hapi'].rollbar; @@ -97,7 +94,7 @@ lab.experiment('plugin relays server errors to rollbar', function() { payload: {} }; - server.on('tail', function(request) { + server.on('tail', function() { sinon.assert.calledTwice(stub); let secondCall = stub.secondCall; var firstArg = secondCall.args[0]; @@ -107,14 +104,12 @@ lab.experiment('plugin relays server errors to rollbar', function() { }); server.inject(request); - }); lab.test('should relay 4xx bad request errors', function(done) { - var server = makeServerWithPlugin(); - var rollbar = server.plugins['rollbar-hapi'].rollbar + var rollbar = server.plugins['rollbar-hapi'].rollbar; server.route({ method: 'GET', @@ -132,7 +127,7 @@ lab.experiment('plugin relays server errors to rollbar', function() { payload: {} }; - server.on('tail', function(request) { + server.on('tail', function() { sinon.assert.calledOnce(stub); var firstArg = stub.firstCall.args[0]; expect(firstArg.isBoom).to.be.true; @@ -141,26 +136,166 @@ lab.experiment('plugin relays server errors to rollbar', function() { }); server.inject(request); - }); + }); + + lab.test('should sanitize messages', function(done) { + let server = makeServerWithPlugin({ + sanitize: function() { + return "foobar"; + } + }); + + var rollbar = server.plugins['rollbar-hapi'].rollbar; + + server.route({ + method: 'GET', + path: '/badRequest', + handler: function (request, reply) { + return reply(Boom.badRequest('Unsupported parameter')); + } + }); + + var stub = sinon.stub(rollbar,'error'); + + const request = { + method: 'GET', + url: '/badRequest', + payload: {} + }; + server.on('tail', function() { + sinon.assert.calledOnce(stub); + var firstArg = stub.firstCall.args[0]; + expect(firstArg).to.equal('foobar'); + stub.restore(); + done(); + }); + + server.inject(request); + }); }); -var makeServerWithPlugin = function () { +lab.experiment('plugin relays server errors that are not filtered to rollbar', function() { + lab.test('Does not relay responses that are filtered out', function(done) { + let server = makeServerWithPlugin({ + filter: function(err) { + if (err.output.statusCode !== 404) { + return true; + } else { + return false; + } + } + }); + + server.route({ + method: 'GET', + path: '/notfound', + handler: function (request, reply) { + return reply(Boom.notFound('Not Found')); + } + }); + + let rollbar = server.plugins['rollbar-hapi'].rollbar; + let stub = sinon.stub(rollbar,'error'); + + const request = { + method: 'GET', + url: '/notFound', + payload: {} + }; + + server.on('tail', function() { + sinon.assert.notCalled(stub); + stub.restore(); + done(); + }); + + server.inject(request); + }); + + lab.test('Relays responses that pass through filter', function(done) { + let server = makeServerWithPlugin({ + filter: function(err) { + if (err.output.statusCode !== 401) { + return true; + } else { + return false; + } + } + }); + + server.route({ + method: 'GET', + path: '/notfound', + handler: function (request, reply) { + return reply(Boom.notFound('Not Found')); + } + }); + + let rollbar = server.plugins['rollbar-hapi'].rollbar; + let stub = sinon.stub(rollbar,'error'); + + const request = { + method: 'GET', + url: '/notFound', + payload: {} + }; + + server.on('tail', function() { + sinon.assert.calledOnce(stub); + stub.restore(); + done(); + }); + + server.inject(request); + }); + + lab.test('Relays all responses when there is no filter option', function(done) { + let server = makeServerWithPlugin(); + + server.route({ + method: 'GET', + path: '/notfound', + handler: function (request, reply) { + return reply(Boom.notFound('Not Found')); + } + }); + + let rollbar = server.plugins['rollbar-hapi'].rollbar; + let stub = sinon.stub(rollbar,'error'); + + const request = { + method: 'GET', + url: '/notFound', + payload: {} + }; + + server.on('tail', function() { + sinon.assert.calledOnce(stub); + stub.restore(); + done(); + }); + + server.inject(request); + }); + +}); + +var makeServerWithPlugin = function (moreOptions = {}) { var server = new Hapi.Server(); server.connection({ port: 7000 }); + let options = Object.assign({ + accessToken: '90bdff07d44a4984aea0d0684bb6c142' + }, moreOptions); + server.register({ register: require('../'), - options: { - accessToken: '90bdff07d44a4984aea0d0684bb6c142' - } + options }, function (err) { if (err) throw err; }); return server; }; - - -