Skip to content
This repository was archived by the owner on Apr 21, 2022. It is now read-only.

Adds callback parameter to the log and other functions. #73

Merged
vilyapilya merged 8 commits into from
Oct 24, 2019
Merged

Adds callback parameter to the log and other functions. #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
835c136
add a callback to logging functionality
lvilya Sep 12, 2019
023b188
lints
lvilya Sep 12, 2019
59255b4
address some comments
lvilya Oct 23, 2019
523553e
nested describe block.
lvilya Oct 24, 2019
760d93c
lints
lvilya Oct 24, 2019
669b426
spelling
lvilya Oct 24, 2019
0e6327b
bhvsdf
lvilya Oct 24, 2019
13e64d9
space between the brackets
lvilya Oct 24, 2019
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
29 changes: 20 additions & 9 deletions lib/logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ function Logger(key, options) {
loggers.push(this);
}

Logger.prototype.log = function(statement, opts) {
Logger.prototype.log = function(statement, opts, callback) {
this._err = false;
if (typeof statement === 'object') {
statement = JSON.parse(JSON.stringify(statement));
Expand All @@ -173,7 +173,9 @@ Logger.prototype.log = function(statement, opts) {
};

if (opts) {
if (typeof opts === 'string') {
if (typeof opts === 'function') {
Copy link
Copy Markdown

@dchai76 dchai76 Oct 15, 2019

Choose a reason for hiding this comment

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

We seem to do this a lot and it seems to me like a terrible idea - it's confusing to me that we can let people send in a callback function as the opts param. Wouldn't it be preferable to require them to send in an empty/null opts and be explicit about what callback is than this?

Copy link
Copy Markdown

@dchai76 dchai76 Oct 17, 2019

Choose a reason for hiding this comment

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

Didn't answer my question, but I really dislike this - let's keep each parameter as it's meant to be instead of pseudo-overloading.

Copy link
Copy Markdown
Contributor Author

@vilyapilya vilyapilya Oct 23, 2019

Choose a reason for hiding this comment

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

sorry I did not see your comments.

Copy link
Copy Markdown
Contributor Author

@vilyapilya vilyapilya Oct 23, 2019

Choose a reason for hiding this comment

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

@dchai76 I think it's arguable. On one hand the implementation of the code library becomes harder to read and maintain. On the other hand, usage of our code libraries becomes easier (you don't have to write your code likes this logger.log("my lolg", {}, (err) => {if err stdout)}); every time when you need to pass the cb to the log function. I think it's better to take the burden of readability on us than pass it on users.

Copy link
Copy Markdown
Contributor Author

@vilyapilya vilyapilya Oct 23, 2019

Choose a reason for hiding this comment

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

and besides, we can also pass a level to that. And it can grow into log("logline", "", {}, cb). Which I think is not a really good practice.

Copy link
Copy Markdown

@dchai76 dchai76 Oct 23, 2019

Choose a reason for hiding this comment

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

To be clear, I think this is bad for callers, not for our own maintainability - magic overloading can mask errors on their part. Did they mean to forget to include opts? Or was it intentional? The type of magic where we say one parameter can be treated in different ways is surprising, and not in a good way.

Copy link
Copy Markdown
Contributor

@weizou19 weizou19 Oct 24, 2019

Choose a reason for hiding this comment

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

I personally don't like this pattern, but I do see this used in lots of places :(
I guess the best way to escape is the make it an async function that returns a promise, so up to the caller whether to chain it or not

Copy link
Copy Markdown

@dchai76 dchai76 Oct 24, 2019

Choose a reason for hiding this comment

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

@weizou19 pointed out to me that Node itself does this (magic overloading of provided callbacks) (https://github.com/nodejs/node/blob/master/lib/fs.js#L405-L414) so as much as I dislike it I'm going to say I'm wrong on this.

callback = opts;
} else if (typeof opts === 'string') {
if (opts.length > configs.MAX_INPUT_LENGTH) {
debug('Level had more than ' + configs.MAX_INPUT_LENGTH + ' chars, was truncated');
opts = opts.substring(0, configs.MAX_INPUT_LENGTH);
Expand Down Expand Up @@ -226,15 +228,19 @@ Logger.prototype.log = function(statement, opts) {
return this._err;
}

this._bufferLog(message);
this._bufferLog(message, callback);
};

Logger.prototype._bufferLog = function(message) {
Logger.prototype._bufferLog = function(message, callback) {
if (!message || !message.line) {
debug('Ignoring empty message');
return;
}

if (!callback || typeof callback !== 'function') {
callback = (err) => { debug(err); };
}

if (this._max_length && message.line.length > configs.MAX_LINE_LENGTH) {
message.line = message.line.substring(0, configs.MAX_LINE_LENGTH) + ' (cut off, too long...)';
debug('Line was longer than ' + configs.MAX_LINE_LENGTH + ' chars and was truncated.');
Expand Down Expand Up @@ -263,14 +269,14 @@ Logger.prototype._bufferLog = function(message) {
debug('Backing off.');
this._isLoggingBackedOff = false;
this._flusher = setTimeout(() => {
this._flush((err) => { debug(err); });
this._flush(callback);
}, this._retryTimeout);
}

if (!this._flusher) {
debug('No scheduled flush. Scheduling for %d ms from now.', configs.FLUSH_INTERVAL);
this._flusher = setTimeout(() => {
this._flush((err) => { debug(err); });
this._flush(callback);
}, this._flushInterval);
}
};
Expand Down Expand Up @@ -358,11 +364,16 @@ Logger.prototype._flush = function(callback) {
* Populate short-hand for each supported Log Level
*/
configs.LOG_LEVELS.forEach(function(level) {
var l = level.toLowerCase();
Logger.prototype[l] = function(statement, opts) {
const levelFunctionName = level.toLowerCase();
Logger.prototype[levelFunctionName] = function(statement, opts, callback) {
if (opts && typeof opts === 'function') {
callback = opts;
opts = {};
}
Comment thread
dchai76 marked this conversation as resolved.
opts = opts || {};
opts.level = level;
this.log(statement, opts);

this.log(statement, opts, callback);
};
});

Expand Down
54 changes: 54 additions & 0 deletions test/logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ describe('Test all Levels', function() {
let allLevelsServer;
let sentLines = [];
let sentLevels = [];

let callbackResult;
const testCallback = (er, res) => { callbackResult = res; };
beforeEach(function(done) {
allLevelsServer = http.createServer(function(req, res) {
req.on('data', function(data) {
Expand Down Expand Up @@ -58,6 +61,45 @@ describe('Test all Levels', function() {
sentLines = [];
sentLevels = [];
body = '';
callbackResult = '';
});
describe('passing callback', function() {
it('Executes callback when provided - debug', function(done) {
allLevelsLogger.debug('Sent a log', testCallback);
setTimeout(function() {
assert(callbackResult.httpStatus === 200);
assert(sentLines[0] === 'Sent a log');
assert(sentLevels[0] === 'DEBUG');
done();
}, configs.FLUSH_INTERVAL + 200);
});
it('Executes callback when provided - trace', function(done) {
allLevelsLogger.trace('Sent a log1', testCallback);
setTimeout(function() {
assert(callbackResult.httpStatus === 200);
assert(sentLines[0] === 'Sent a log1');
assert(sentLevels[0] === 'TRACE');
done();
}, configs.FLUSH_INTERVAL + 200);
});
it('Executes callback when provided - info', function(done) {
allLevelsLogger.info('Sent a log2', testCallback);
setTimeout(function() {
assert(callbackResult.httpStatus === 200);
assert(sentLines[0] === 'Sent a log2');
assert(sentLevels[0] === 'INFO');
done();
}, configs.FLUSH_INTERVAL + 200);
});
it('Executes callback when provided - warn', function(done) {
allLevelsLogger.warn('Sent a log3', testCallback);
setTimeout(function() {
assert(callbackResult.httpStatus === 200);
assert(sentLines[0] === 'Sent a log3');
assert(sentLevels[0] === 'WARN');
done();
}, configs.FLUSH_INTERVAL + 200);
});
});
it('Debug Function', function(done) {
allLevelsLogger.debug('Sent a log');
Expand Down Expand Up @@ -107,6 +149,7 @@ describe('Test all Levels', function() {
done();
}, configs.FLUSH_INTERVAL + 200);
});

});

describe('Testing for Correctness', function() {
Expand Down Expand Up @@ -599,4 +642,15 @@ describe('HTTP Exception Handling', function() {
done();
}, configs.FLUSH_INTERVAL + 200);
});
it('if log has a callback, it should be called with an error', function(done) {
const opts = testHelper.createOptions({port: port});
const flushAllTest = Logger.createLogger(testHelper.apikey, opts);
let callbackError;
flushAllTest.log('Test line', (e) => {callbackError = e;});

setTimeout(function() {
assert(callbackError === 'An error occured while making the request. Response status code: 302 Found');
done();
}, configs.FLUSH_INTERVAL + 200);
});
});