Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add logging for error handling edge cases #742

Closed
wants to merge 4 commits into from
Closed

add logging for error handling edge cases #742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d9f8078
add logging for error handling edge cases
missinglink Dec 2, 2016
acbad4c
change all log labels to 'api'
missinglink Dec 2, 2016
5bda15e
update 500 middleware; add log statement, clean up code
missinglink Dec 2, 2016
650dd57
additional backend error logging
missinglink Dec 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion controller/place.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
var service = { mget: require('../service/mget') };
var logger = require('pelias-logger').get('api:controller:place');
var logger = require('pelias-logger').get('api');

function setup( config, backend ){

Expand Down
2 changes: 1 addition & 1 deletion controller/search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
var _ = require('lodash');

var service = { search: require('../service/search') };
var logger = require('pelias-logger').get('api:controller:search');
var logger = require('pelias-logger').get('api');
var logging = require( '../helper/logging' );

function setup( config, backend, query ){
Expand Down
16 changes: 11 additions & 5 deletions middleware/500.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
var logger = require( 'pelias-logger' ).get( 'middleware-500' );
var check = require('check-types'),
logger = require( 'pelias-logger' ).get( 'api' );

// handle application errors
function middleware(err, req, res, next) {

logger.error( 'Error: `%s`. Stack trace: `%s`.', err, err.stack );
res.header('Cache-Control','public');
var error = (err && err.message) ? err.message : err;

if( res.statusCode < 400 ){ res.status(500); }
res.json({ error: typeof error === 'string' ? error : 'internal server error' });
if( res.statusCode < 400 ){
logger.info( 'status code changed from', res.statusCode, 'to 500' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need spaces after from and before to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

console.log( "doesn't", "it", "add", "them", "for", "me?" );
doesn't it add them for me?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I learn something new about javascript every day!

res.status(500);
}

var error = ( err && err.message ) ? err.message : err;
res.header('Cache-Control','public');
res.json({ error: check.nonEmptyString( error ) ? error : 'internal server error' });
}

module.exports = middleware;
2 changes: 1 addition & 1 deletion middleware/confidenceScoreFallback.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*/

var check = require('check-types');
var logger = require('pelias-logger').get('api-confidence');
var logger = require('pelias-logger').get('api');

function setup() {
return computeScores;
Expand Down
9 changes: 8 additions & 1 deletion middleware/sendJSON.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
var check = require('check-types'),
es = require('elasticsearch'),
logger = require( 'pelias-logger' ).get( 'api' ),
exceptions = require('elasticsearch-exceptions/lib/exceptions/SupportedExceptions');

// create a list of regular expressions to match against.
Expand Down Expand Up @@ -42,7 +43,10 @@ function sendJSONResponse(req, res, next) {
if( err instanceof es.errors.RequestTimeout ){ statusCode = Math.max( statusCode, 408 ); }
else if( err instanceof es.errors.NoConnections ){ statusCode = Math.max( statusCode, 502 ); }
else if( err instanceof es.errors.ConnectionFault ){ statusCode = Math.max( statusCode, 502 ); }
else { statusCode = Math.max( statusCode, 500 ); }
else {
logger.warn( 'unknown geocoding error object:', err.constructor.name, err );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought we were going to check for existance of err.contructor here? seems a bit optimistic to assume it's there

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a think about it and the if( err instanceof Error ){ check above guarantees that err.constructor.name will always exist.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

var err = new Error()

err.constructor.name;
"Error"

delete err.constructor
true

err.constructor.name;
"Error"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

function MyError(){}
MyError.constructor = Error.Constructor;

var err = new MyError()

err.constructor.name;
"MyError"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for some reason.. even with a typo the above works :P

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right, it works for any function:

function test(){}

test.constructor.name;
"Function"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and because javascript; everything apparently

(99).constructor.name
"Number"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh javascript...

(NaN).constructor.name
"Number"

statusCode = Math.max( statusCode, 500 );
}

/*
some elasticsearch errors are only returned as strings (not instances of Error).
Expand All @@ -55,7 +59,10 @@ function sendJSONResponse(req, res, next) {
statusCode = Math.max( statusCode, 500 );
break; // break on first match
}
logger.warn( 'unknown geocoding error string:', err );
}
} else {
logger.warn( 'unknown geocoding error type:', typeof err, err );
}
});
}
Expand Down
2 changes: 1 addition & 1 deletion sanitizer/search_fallback.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ var sanitizeAll = require('../sanitizer/sanitizeAll'),
};

var sanitize = function(req, cb) { sanitizeAll(req, sanitizers, cb); };
var logger = require('pelias-logger').get('api:controller:search_fallback');
var logger = require('pelias-logger').get('api');
var logging = require( '../helper/logging' );

// middleware
Expand Down
13 changes: 11 additions & 2 deletions service/mget.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

**/

var peliasLogger = require( 'pelias-logger' ).get( 'service/mget' );
var logger = require( 'pelias-logger' ).get( 'api' );

function service( backend, query, cb ){

Expand All @@ -24,8 +24,17 @@ function service( backend, query, cb ){

// query new backend
backend().client.mget( cmd, function( err, data ){

// log total ms elasticsearch reported the query took to execute
if( data && data.took ){
logger.verbose( 'time elasticsearch reported:', data.took / 1000 );
}

// handle backend errors
if( err ){ return cb( err ); }
if( err ){
logger.error( 'backend error', err );
return cb( err );
}

// map returned documents
var docs = [];
Expand Down
15 changes: 10 additions & 5 deletions service/search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,23 @@

**/

var peliasLogger = require( 'pelias-logger' ).get( 'service/search' );
var logger = require( 'pelias-logger' ).get( 'api' );

function service( backend, cmd, cb ){

// query new backend
backend().client.search( cmd, function( err, data ){

// handle backend errors
if( err ){ return cb( err ); }

// log total ms elasticsearch reported the query took to execute
peliasLogger.verbose( 'time elasticsearch reported:', data.took / 1000 );
if( data && data.took ){
logger.verbose( 'time elasticsearch reported:', data.took / 1000 );
}

// handle backend errors
if( err ){
logger.error( 'backend error', err );
return cb( err );
}

// map returned documents
var docs = [];
Expand Down