Skip to content
This repository has been archived by the owner on Aug 18, 2021. It is now read-only.

Commit

Permalink
Logging refactor (#255)
Browse files Browse the repository at this point in the history 
**Summary**

Improvement to application logging.

Explain the **motivation** for making this change. What existing problem does the pull request solve?

The logging for the application was a bit hard to follow when queries needed to be done to the logs.

There was a ton of unnecessary inheritance that was eliminated. Serializers were also added to help improve request and response logging

**Test plan (required)**

Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes UI.
  • Loading branch information
Froilan Irizarry authored Sep 8, 2018
1 parent d2ba761 commit d196fc0
Show file tree
Hide file tree
Showing 14 changed files with 261 additions and 160 deletions.
3 changes: 2 additions & 1 deletion .eslintrc
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
{
"parserOptions": {
"sourceType": "module",
"allowImportExportEverywhere": true
"allowImportExportEverywhere": true,
"ecmaVersion": 2018
},
"env": {
"node": true,
Expand Down
32 changes: 13 additions & 19 deletions app.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const RateLimit = require('express-rate-limit');
const Searcher = require("./services/searcher");
const ElasticsearchSearcherAdapter = require("./utils/search_adapters/elasticsearch_adapter");
const swaggerUi = require('swagger-ui-express');
const addRequestId = require('express-request-id')();

/* eslint-disable */
const pug = require("pug");
Expand Down Expand Up @@ -43,7 +44,6 @@ if( config.USE_RATE_LIMITER) {
app.use(bodyParser.json());
app.use(bodyParser.urlencoded({ extended: false }));
app.use(cookieParser());
app.use('/', express.static(path.join(__dirname, 'public')));
app.use(cors());
app.use(helmet());
app.use(helmet.hsts({
Expand All @@ -54,11 +54,18 @@ app.use(helmet.hsts({
}
}));

app.use(addRequestId);
app.use(compression());

app.use(function(req, res, next) {
logger.info({ req: req, res: res });
next();
});

app.use('/api/docs', swaggerUi.serve, swaggerUi.setup(config.SWAGGER_DOCUMENT));
app.set('views', path.join(__dirname, 'views'));
app.set('view engine', 'pug');
app.set('views', path.join(__dirname, 'views'));
app.use('/static', express.static(path.join(__dirname, 'public')));
app.set('json spaces', 2);

/* ------------------------------------------------------------------ *
Expand All @@ -80,25 +87,12 @@ app.use(function(req, res, next) {
next(err);
});

// development error handler (prints stacktrace)
if (app.get('env') === 'development') {
app.use(function(err, req, res) {
res.status(err.status || 500);
logger.error(err);
res.render('error', {
message: err.message,
error: err
});
});
}

// production error handler (prints generic error message)
app.use(function(err, req, res) {
app.use(function(err, req, res, next) {
res.status(err.status || 500);
logger.error(err);
res.render('error', {
logger.error({req: req, res: res, err: err});
res.json({
message: err.message,
error: {}
error: app.get('env') === 'development' ? err : {}
});
});

Expand Down
2 changes: 1 addition & 1 deletion config/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ function getSwaggerConf(isProd, apiUrl) {
* @param {string} env - The application environment. This will default to a development environment
* @returns {object} - object with all the configuration needed for the environment
*/
function getConfig(env) {
function getConfig(env='development') {
let config = {
prod_envs: ['prod', 'production', 'stag', 'staging']
};
Expand Down
8 changes: 8 additions & 0 deletions package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 7 additions & 6 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@
},
"scripts": {
"start": "node app.js | ./node_modules/.bin/bunyan --color",
"debug": "node --inspect app.js",
"debug": "node --inspect app.js | ./node_modules/.bin/bunyan --color",
"index": "node ./scripts/index/index.js | ./node_modules/.bin/bunyan --color",
"index-debug": "node --inspect ./scripts/index/index.js",
"index-repos": "node ./scripts/index/repo/index.js",
"index-terms": "node ./scripts/index/term/index.js",
"index-debug": "node --inspect ./scripts/index/index.js | ./node_modules/.bin/bunyan --color",
"index-repos": "node ./scripts/index/repo/index.js | ./node_modules/.bin/bunyan --color",
"index-terms": "node ./scripts/index/term/index.js | ./node_modules/.bin/bunyan --color",
"create-diffs": "node ./scripts/create_diffs/index.js --max_old_space_size=8192",
"find-gh-gov-orgs": "node ./scripts/find_gh_gov_orgs/index.js",
"find-gh-gov-repos": "node ./scripts/find_gh_gov_repos/index.js",
"find-gh-gov-orgs": "node ./scripts/find_gh_gov_orgs/index.js | ./node_modules/.bin/bunyan",
"find-gh-gov-repos": "node ./scripts/find_gh_gov_repos/index.js | ./node_modules/.bin/bunyan",
"test": "./node_modules/.bin/nyc mocha --reporter mocha-multi-reporters --reporter-options configFile=test/mocha-multi-reporter-config.json test",
"unit-test": "./node_modules/.bin/nyc mocha --reporter mocha-multi-reporters --reporter-options configFile=test/mocha-multi-reporter-config.json test/unit",
"integration-test": "./node_modules/.bin/nyc mocha --reporter mocha-multi-reporters --reporter-options configFile=test/mocha-multi-reporter-config.json test/integration",
Expand All @@ -43,6 +43,7 @@
"event-stream": "^3.3.4",
"express": "^4.16.2",
"express-rate-limit": "^2.9.0",
"express-request-id": "^1.4.0",
"git-rev": "^0.2.1",
"github": "^14.0.0",
"helmet": "^3.11.0",
Expand Down
73 changes: 43 additions & 30 deletions routes/routes.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,33 @@ function getApiRoutes(config, searcher, router) {

const logger = new Logger({ name: 'routes.index', level: config.LOGGER_LEVEL });

router.get('/repos/:id', (request, response) => getRepoById (request, response, searcher, logger));
router.get('/repos', (request, response) => queryReposAndSendResponse(
searcher, request.query, response, logger));
router.get('/terms', (request, response) => getTerms(request, response, searcher));
router.get(`/agencies`, (request, response) => {
router.get('/repos/:id', async (request, response, next) => {
try {
const result = await getRepoById(request.params.id, searcher);

if(_.isEmpty(result)) {
const error = new Error('Not Found');
error.status = 404;
next(error);
}

response.json(result);

} catch(error) {
next(error);
}
});
router.get('/repos', (request, response, next) => {
queryReposAndSendResponse(searcher, request.query, logger)
.then(result => response.json(result))
.catch(error => next(error));
});
router.get('/terms', (request, response, next) => {
getTerms(request, response, searcher)
.then(result => response.json(result))
.catch(error => next(error));
});
router.get(`/agencies`, (request, response, next) => {
getAgencies(request, searcher, config, logger)
.then(result => {
if(result) {
Expand All @@ -37,7 +59,7 @@ function getApiRoutes(config, searcher, router) {
response.sendStatus(404);
});
});
router.get(`/agencies/:agency_acronym`, (request, response) => {
router.get(`/agencies/:agency_acronym`, (request, response, next) => {
getAgency(request, searcher, config, logger)
.then(results => {
if(results) {
Expand All @@ -51,7 +73,7 @@ function getApiRoutes(config, searcher, router) {
response.sendStatus(404);
});
});
router.get(`/languages`, (request, response) => {
router.get(`/languages`, (request, response, next) => {
let options;
getLanguages(request, searcher, logger, options)
.then(results => {
Expand All @@ -66,8 +88,8 @@ function getApiRoutes(config, searcher, router) {
response.sendStatus(404);
});
});
router.get('/repo.json', (request, response) => getRepoJson(response));
router.get('/status.json', (request, response) => {
router.get('/repo.json', (request, response, next) => getRepoJson(response));
router.get('/status.json', (request, response, next) => {
getStatusData(searcher)
.then(results => {
if(results){
Expand All @@ -82,7 +104,7 @@ function getApiRoutes(config, searcher, router) {
response.sendStatus(404);
});
});
router.get(`/status`, (request, response) => {
router.get(`/status`, (request, response, next) => {
getStatusData(searcher)
.then(results => response.render('status', { title: "Code.gov API Status", statusData: results }))
.catch(error => {
Expand All @@ -91,7 +113,7 @@ function getApiRoutes(config, searcher, router) {
});

});
router.get(`/status/:agency/issues`, (request, response) => {
router.get(`/status/:agency/issues`, (request, response, next) => {
let agency = request.params.agency.toUpperCase();
getAgencyIssues(agency, searcher)
.then(issuesData => {
Expand All @@ -106,27 +128,18 @@ function getApiRoutes(config, searcher, router) {
response.sendStatus(500);
});
});
router.get(`/status/:agency/fetched`, (request, response) => {
router.get(`/status/:agency/fetched`, async (request, response, next) => {
const agency = request.params.agency.toUpperCase();

if(agency) {
getFetchedReposByAgency(agency, config)
.then(results => {
if(results) {
response.json(results);
} else {
response.sendStatus(404);
}
})
.catch(error => {
logger.error(error);
response.sendStatus(404);
});
} else {
response.sendStatus(400);
try {
const results = await getFetchedReposByAgency(agency, config);
response.json(results);
} catch(error) {
logger.trace(error);
next(error);
}
});
router.get(`/status/:agency/discovered`, (request, response) => {
router.get(`/status/:agency/discovered`, (request, response, next) => {
const agency = request.params.agency.toUpperCase();

if(agency) {
Expand All @@ -146,7 +159,7 @@ function getApiRoutes(config, searcher, router) {
response.sendStatus(400);
}
});
router.get('/version', (request, response) => {
router.get('/version', (request, response, next) => {
getVersion(response)
.then(versionInfo => response.json(versionInfo))
.catch(error => {
Expand All @@ -155,7 +168,7 @@ function getApiRoutes(config, searcher, router) {
});
});

router.get('/', (request, response) =>
router.get('/', (request, response, next) =>
getRootMessage()
.then(rootMessage => response.json(rootMessage))
);
Expand Down
71 changes: 35 additions & 36 deletions routes/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,28 +131,29 @@ function getInvalidRepoQueryParams (queryParams) {
});
}

function queryReposAndSendResponse (searcher, query, response, logger) {
function queryReposAndSendResponse (searcher, query, logger) {
let queryParams = Object.keys(query);

if(queryParams.length) {
// validate query params...
let invalidParams = getInvalidRepoQueryParams(queryParams);
if (invalidParams.length > 0) {
let error = {
"Error": "Invalid query params.",
"Invalid Params": invalidParams
};
logger.error(error);
return response.status(400).json(error);
return new Promise((resolve, reject) => {
if(queryParams.length) {
let invalidParams = getInvalidRepoQueryParams(queryParams);
if (invalidParams.length > 0) {
let error = {
"Error": "Invalid query params.",
"Invalid Params": invalidParams
};
logger.trace(error);
reject(error);
}
}
}

searcher.searchRepos(query, (error, repos) => {
if(error) {
logger.error(error);
return response.sendStatus(500);
}
response.json(repos);
searcher.searchRepos(query, (error, repos) => {
if(error) {
logger.error(error);
reject(error);
}
resolve(repos);
});
});
}

Expand All @@ -171,29 +172,27 @@ function getFileDataByAgency(agency, directoryPath) {
});
}

function getRepoById (request, response, searcher, logger) {
let id = request.params.id;
searcher.getRepoById(id, (error, repo) => {
if (error) {
logger.error(error);
return response.sendStatus(500);
}
if (!_.isEmpty(repo)) {
response.json(repo);
} else {
response.sendStatus(404);
}
function getRepoById (id, searcher) {
return new Promise((resolve, reject) => {
searcher.getRepoById(id, (error, repo) => {
if (error) {
reject(error);
}
resolve(repo);
});
});
}

function getTerms(request, response, searcher) {
let query = _.pick(request.query, ["term", "term_type", "size", "from"]);
return new Promise((resolve, reject) => {
let query = _.pick(request.query, ["term", "term_type", "size", "from"]);

searcher.searchTerms(query, (error, terms) => {
if (error) {
return response.sendStatus(500);
}
response.json(terms);
searcher.searchTerms(query, (error, terms) => {
if (error) {
reject(error);
}
resolve(terms);
});
});
}

Expand Down
Loading

0 comments on commit d196fc0

Please sign in to comment.