Add logger middleware, refactor logging layer

TailorDev · Jan 28, 2017 · 2972f54 · 2972f54
1 parent 7236fa2
commit 2972f54
Show file tree

Hide file tree

Showing 7 changed files with 56 additions and 51 deletions.
diff --git a/app.js b/app.js
@@ -1,9 +1,8 @@
-/* eslint no-console: 0*/
+/* eslint global-require: 0 */
 const express = require('express');
 const compression = require('compression');
 const session = require('express-session');
 const bodyParser = require('body-parser');
-const chalk = require('chalk');
 const lusca = require('lusca');
 const MongoStore = require('connect-mongo')(session);
 const flash = require('express-flash');
@@ -14,9 +13,7 @@ const expressValidator = require('express-validator');
 const sass = require('node-sass-middleware');
 const moment = require('moment');
 const d3Format = require('d3-format');
-
 const gh = require('./helpers/github');
-const logger = require('./helpers/logger');
 
 /**
  * Controllers (route handlers).
@@ -36,26 +33,28 @@ const passportConfig = require('./config/passport');
  * Create Express server.
  */
 const app = express();
+const logger = require('./helpers/logger').new(app.get('env'));
 
 /**
  * Connect to MongoDB.
  */
 mongoose.Promise = global.Promise;
 mongoose.connect(process.env.MONGODB_URI || process.env.MONGOLAB_URI);
 mongoose.connection.on('error', () => {
-  console.log('%s MongoDB connection error. Please make sure MongoDB is running.', chalk.red('✗'));
+  logger.error('✗ MongoDB connection error. Please make sure MongoDB is running.');
   process.exit();
 });
 
 /**
  * Express configuration.
  */
 app.set('port', process.env.PORT || 3000);
-app.use(require('express-request-id')()); // eslint-disable-line global-require
+app.use(require('express-request-id')());
+app.use(require('./middlewares/logger')(logger));
 
 if (app.get('env') === 'production') {
   app.enable('trust proxy');
-  app.use(require('express-sslify').HTTPS({ trustProtoHeader: true })); // eslint-disable-line global-require
+  app.use(require('express-sslify').HTTPS({ trustProtoHeader: true }));
 
   // redirect to custom domain (if any)
   // TODO: make it work in dev too
@@ -248,15 +247,15 @@ app.use((req, res) => {
 });
 
 // handle all other errors
-app.use(require('./middlewares/error')(app.get('env'), logger));
+app.use(require('./middlewares/error')(app.get('env')));
 
 /**
  * Start Express server.
  */
 app.listen(app.get('port'), () => {
-  console.log('%s App is running at http://localhost:%d in %s mode', chalk.green('✓'), app.get('port'), app.get('env'));
+  logger.info('✓ App is running at http://localhost:%d in %s mode', app.get('port'), app.get('env'));
 
-  console.log('  Press CTRL-C to stop\n');
+  logger.info('  Press CTRL-C to stop\n');
 });
 
 module.exports = app;
diff --git a/bin/assignees b/bin/assignees
@@ -30,6 +30,11 @@ const error = (message) => {
   return 1;
 };
 
+const logger = {
+  info: success,
+  error: error,
+};
+
 const ADD = 'add';
 const REMOVE = 'remove';
 
@@ -145,11 +150,7 @@ program
         maxPullRequestFilesToProcess: process.env.maxPullRequestFilesToProcess || 5,
         nbCommitsToRetrieve: process.env.nbCommitsToRetrieve || 30,
         createReviewRequest: dryrun === false,
-        logger: {
-          info: success,
-          error: error,
-        },
-      })(repositoryId, number, author);
+      })(repositoryId, number, author, logger);
 
       if (!dryrun) {
         success('reviewer(s) assigned');

diff --git a/controllers/event.js b/controllers/event.js
@@ -4,7 +4,6 @@ const findReviewers = require('../tasks/findReviewers')({
   maxPullRequestFilesToProcess: process.env.maxPullRequestFilesToProcess || 5,
   nbCommitsToRetrieve: process.env.nbCommitsToRetrieve || 30,
   createReviewRequest: true,
-  logger,
 });
 
 /**
@@ -31,7 +30,7 @@ exports.listen = async (req, res) => {
     req.body.repository.id,
     req.body.pull_request.number,
     req.body.pull_request.user.login,
-    req.id
+    req.logger
   );
 
   res.send({ status: 'ok' });

diff --git a/helpers/logger.js b/helpers/logger.js
@@ -1,24 +1,37 @@
 /* eslint no-console: 0 */
 
-const info = (message) => {
-  console.log(`[info] ${message}`);
+const prependMessage = (pre, fn) => (...args) => {
+  const message = args.shift();
+
+  fn(`${pre} ${message}`, ...args);
 };
 
-const error = (message) => {
-  console.log(`[error] ${message}`);
+// basic logger
+const logger = {
+  info: (...args) => prependMessage('[info]', console.log)(...args),
+  error: (...args) => prependMessage('[error]', console.log)(...args),
 };
 
-let logger;
-if (process.env.NODE_ENV === 'test') {
-  logger = {
-    info: () => {},
-    error: () => {},
-  };
-} else {
-  logger = {
-    info,
-    error,
+const nullLogger = {
+  info: () => {},
+  error: () => {},
+};
+
+exports.withRequestId = (logger, requestId) => {
+  if (!requestId) {
+    return logger;
+  }
+
+  return {
+    info: (message) => logger.info(`request_id=${requestId} ${message}`),
+    error: (message) => logger.error(`request_id=${requestId} ${message}`),
   };
-}
+};
 
-module.exports = logger;
+exports.new = (env) => {
+  if (env === 'test') {
+    return nullLogger;
+  }
+
+  return logger;
+};
diff --git a/middlewares/error.js b/middlewares/error.js
@@ -3,7 +3,7 @@ const util = require('util');
 
 const inspect = require('../helpers/inspect
D7AE
');
 
-module.exports = (env, logger) => {
+module.exports = (env) => {
   if (env === 'test') {
     return (err, req, res, next) => {
       res.status(err.statusCode || 500).end();
@@ -13,10 +13,6 @@ module.exports = (env, logger) => {
   return (err, req, res, next) => {
     const info = [];
 
-    if (req.id) {
-      info.push(`request_id=${req.id}`);
-    }
-
     if (req.user) {
       info.push(`user_id=${req.user._id}`);
       info.push(`user_login=${req.user.github_login}`);
@@ -32,7 +28,7 @@ module.exports = (env, logger) => {
       k => info.push(`error_${k}=${inspect(err[k])}`)
     );
 
-    logger.error(info.join(' '));
+    req.logger.error(info.join(' '));
 
     return res.format({
       json: () => {

diff --git a/middlewares/logger.js b/middlewares/logger.js
@@ -0,0 +1,8 @@
+const { withRequestId } = require('../helpers/logger');
+
+module.exports = (logger) => {
+  return (req, res, next) => {
+    req.logger = withRequestId(logger, req.id);
+    next();
+  };
+};
diff --git a/tasks/findReviewers.js b/tasks/findReviewers.js
@@ -48,26 +48,16 @@ const logReviewers = (logger, repository, number, reviewers) => {
   ].join(' '));
 };
 
-const loggerWithRequestId = (logger, requestId) => {
-  if (!requestId) {
-    return logger;
-  }
 
-  return {
-    info: (message) => logger.info(`request_id=${requestId} ${message}`),
-    error: (message) => logger.error(`request_id=${requestId} ${message}`),
-  };
-};
 
 /**
  * config = {
  *   maxPullRequestFilesToProcess: number,
  *   nbCommitsToRetrieve: number,
  *   createReviewRequest: boolean,
- *   logger: { info: Function, error: Function },
  * }
  */
-module.exports = config => async (repositoryId, number, author, requestId) => {
+module.exports = config => async (repositoryId, number, author, logger) => {
   const repository = await Repository.findOneByGitHubId(repositoryId);
 
   if (!repository) {
@@ -84,7 +74,6 @@ module.exports = config => async (repositoryId, number, author, requestId) => {
     throw createHttpError(401, 'ignored', 'user not found');
   }
 
-  const logger = loggerWithRequestId(config.logger, requestId);
   logger.info(`repository_id=${repositoryId} number=${number} author=${author}`);
 
   // the GitHub dance