From c6192dd24b3a50c80baf9837c6e4205f41f9e36c Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Thu, 26 May 2016 23:10:38 +0200 Subject: [PATCH 1/3] feat(logging): improve logging removing not useful data and distinguishing between user and server errors --- website/server/libs/api-v3/logger.js | 26 +++++++++++++++++-- .../server/middlewares/api-v3/errorHandler.js | 14 +++++----- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/website/server/libs/api-v3/logger.js b/website/server/libs/api-v3/logger.js index a5a46fec87..4260e72a48 100644 --- a/website/server/libs/api-v3/logger.js +++ b/website/server/libs/api-v3/logger.js @@ -2,6 +2,9 @@ import winston from 'winston'; import nconf from 'nconf'; import _ from 'lodash'; +import { + CustomError, +} from './errors'; const IS_PROD = nconf.get('IS_PROD'); const IS_TEST = nconf.get('IS_TEST'); @@ -42,8 +45,27 @@ let loggerInterface = { // pass the error stack as the first parameter to logger.error let stack = err.stack || err.message || err; - if (_.isPlainObject(errorData) && !errorData.fullError) errorData.fullError = err; - logger.error(stack, errorData, ...otherArgs); + if (_.isPlainObject(errorData) && !errorData.fullError) { + // If the error object has interesting data (not only httpCode, message and name from the CustomError class) + // add it to the logs + if (err instanceof CustomError) { + let errWithoutCommonProps = _.omit(err, ['name', 'httpCode', 'message']); + + if (Object.keys(errWithoutCommonProps).length > 0) { + errorData.fullError = errWithoutCommonProps; + } + } else { + errorData.fullError = err; + } + } + + let loggerArgs = [stack, errorData, ...otherArgs]; + // Treat 4xx errors as warnings, 5xx and uncaught errors as serious problems + if (!errorData || !errorData.statusCode || errorData.statusCode > 499) { + logger.error(...loggerArgs); + } else { + logger.warn(stack, errorData, ...otherArgs); + } } else { logger.error(...args); } diff --git a/website/server/middlewares/api-v3/errorHandler.js b/website/server/middlewares/api-v3/errorHandler.js index 11d75042e3..891f0b36af 100644 --- a/website/server/middlewares/api-v3/errorHandler.js +++ b/website/server/middlewares/api-v3/errorHandler.js @@ -12,12 +12,6 @@ import { } from 'lodash'; module.exports = function errorHandler (err, req, res, next) { // eslint-disable-line no-unused-vars - logger.error(err, { - originalUrl: req.originalUrl, - headers: omit(req.headers, ['x-api-key']), - body: req.body, - }); - // In case of a CustomError class, use it's data // Otherwise try to identify the type of error (mongoose validation, mongodb unique, ...) // If we can't identify it, respond with a generic 500 error @@ -70,6 +64,14 @@ module.exports = function errorHandler (err, req, res, next) { // eslint-disable responseErr = new InternalServerError(); } + // log the error + logger.error(err, { + originalUrl: req.originalUrl, + headers: omit(req.headers, ['x-api-key', 'cookie']), // don't send sensitive information that only adds noise + body: req.body, + statusCode: responseErr.httpCode, + }); + let jsonRes = { success: false, error: responseErr.name, From 10cf22cd4e797d482ae07a0bfd7e5afc9ed2a7a9 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Thu, 26 May 2016 23:19:29 +0200 Subject: [PATCH 2/3] better distinguish between handled and not handler errors --- website/server/libs/api-v3/logger.js | 7 ++++--- website/server/middlewares/api-v3/errorHandler.js | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/website/server/libs/api-v3/logger.js b/website/server/libs/api-v3/logger.js index 4260e72a48..dd0ca8396e 100644 --- a/website/server/libs/api-v3/logger.js +++ b/website/server/libs/api-v3/logger.js @@ -60,11 +60,12 @@ let loggerInterface = { } let loggerArgs = [stack, errorData, ...otherArgs]; - // Treat 4xx errors as warnings, 5xx and uncaught errors as serious problems - if (!errorData || !errorData.statusCode || errorData.statusCode > 499) { + + // Treat 4xx errors that are handled as warnings, 5xx and uncaught errors as serious problems + if (!errorData || !errorData.isHandledError || errorData.httpCode >= 500) { logger.error(...loggerArgs); } else { - logger.warn(stack, errorData, ...otherArgs); + logger.warn(...loggerArgs); } } else { logger.error(...args); diff --git a/website/server/middlewares/api-v3/errorHandler.js b/website/server/middlewares/api-v3/errorHandler.js index 891f0b36af..cd76277c78 100644 --- a/website/server/middlewares/api-v3/errorHandler.js +++ b/website/server/middlewares/api-v3/errorHandler.js @@ -69,7 +69,8 @@ module.exports = function errorHandler (err, req, res, next) { // eslint-disable originalUrl: req.originalUrl, headers: omit(req.headers, ['x-api-key', 'cookie']), // don't send sensitive information that only adds noise body: req.body, - statusCode: responseErr.httpCode, + httpCode: responseErr.httpCode, + isHandledError: responseErr.httpCode < 500, }); let jsonRes = { From cb4676980d188f6946a9fab4f36ac4918a70d43d Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Thu, 26 May 2016 23:35:57 +0200 Subject: [PATCH 3/3] fix tests for error handler middleware --- test/api/v3/unit/middlewares/errorHandler.test.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/api/v3/unit/middlewares/errorHandler.test.js b/test/api/v3/unit/middlewares/errorHandler.test.js index 72cad12a32..0cb1e1bb97 100644 --- a/test/api/v3/unit/middlewares/errorHandler.test.js +++ b/test/api/v3/unit/middlewares/errorHandler.test.js @@ -168,6 +168,8 @@ describe('errorHandler', () => { originalUrl: req.originalUrl, headers: req.headers, body: req.body, + httpCode: 400, + isHandledError: true, }); }); });