-
Notifications
You must be signed in to change notification settings - Fork 132
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.
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
Improve Logger class #397
Merged
Merged
Improve Logger class #397
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
f2c9997
Add defaults to log methods
iwiznia 8e163a9
Some refactor on log classes to play nicer with server logging
iwiznia 5ddf00f
Do not flush logs with 1 line
iwiznia 05e51e1
Fix tests
iwiznia 64a2ff6
Fix tests
iwiznia ca4597f
Add missing space
iwiznia 453a6ce
Add onlyFlushWithOThers
iwiznia 5d69f59
Improve tests, remove onlyFlushWithOthers from request to server
iwiznia 1861959
Clear log lines before calling callback
iwiznia 144b817
Merge branch 'master' into ionatan_defaultparams_log
iwiznia f31dd1f
Pass the logger instace to the server logging callback
iwiznia 8aba7d3
Fix tests
iwiznia 5fb22bd
Fix Log
iwiznia File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
import _ from 'underscore'; | ||
|
||
const MAX_LOG_LINES_BEFORE_FLUSH = 50; | ||
export default class Logger { | ||
constructor({serverLoggingCallback, isDebug, clientLoggingCallback}) { | ||
this.TEMP_LOG_LINES_TO_KEEP = 50; | ||
// An array of log lines that limits itself to a certain number of entries (deleting the oldest) | ||
this.logLines = []; | ||
this.serverLoggingCallback = serverLoggingCallback; | ||
|
@@ -21,57 +21,49 @@ export default class Logger { | |
|
||
/** | ||
* Ask the server to write the log message | ||
* | ||
* @param {String} message The message to write | ||
* @param {Number} recentMessages A number of recent messages to append as context | ||
* @param {Object|String} parameters The parameters to send along with the message | ||
*/ | ||
logToServer(message, recentMessages, parameters = {}) { | ||
// Optionally append recent log lines as context | ||
let msg = message; | ||
if (recentMessages > 0) { | ||
msg += ' | Context: '; | ||
msg += JSON.stringify(this.get(recentMessages)); | ||
} | ||
|
||
// Output the message to the console too. | ||
if (this.isDebug) { | ||
this.client(`${msg} - ${JSON.stringify(parameters)}`); | ||
logToServer() { | ||
// We do not want to call the server with an empty list or if all the lines has onlyFlushWithOthers=false | ||
if (!_.any(this.logLines, l => !l.onlyFlushWithOthers)) { | ||
marcaaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return; | ||
} | ||
|
||
// We don't care about log setting web cookies so let's define it as false | ||
const params = {parameters, message, api_setCookie: false}; | ||
this.serverLoggingCallback(params); | ||
const promise = this.serverLoggingCallback({api_setCookie: false, logPacket: JSON.stringify(this.logLines)}); | ||
this.logLines = []; | ||
if (!promise) { | ||
marcaaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return; | ||
} | ||
promise.then((response) => { | ||
if (response.requestID) { | ||
this.info('Previous log requestID', false, {requestID: response.requestID}, true); | ||
} | ||
}); | ||
} | ||
|
||
/** | ||
* Add a message to the list | ||
* @param {String} message | ||
* @param {Object|String} parameters The parameters associated with the message | ||
* @param {Boolean} forceFlushToServer Should we force flushing all logs to server? | ||
* @param {Boolean} onlyFlushWithOthers A request will never be sent to the server if all loglines have this set to true | ||
*/ | ||
add(message, parameters) { | ||
add(message, parameters, forceFlushToServer, onlyFlushWithOthers = false) { | ||
const length = this.logLines.push({ | ||
message, | ||
parameters, | ||
onlyFlushWithOthers, | ||
timestamp: (new Date()) | ||
}); | ||
|
||
// If we're over the limit, remove one line. | ||
if (length > this.TEMP_LOG_LINES_TO_KEEP) { | ||
this.logLines.shift(); | ||
if (this.isDebug) { | ||
this.client(`${message} - ${JSON.stringify(parameters)}`); | ||
} | ||
} | ||
|
||
/** | ||
* Get the last messageCount lines | ||
* @param {Number} messageCount Number of messages to get (optional, | ||
* defaults to 1) | ||
* @return {array} an array of messages (oldest first) | ||
*/ | ||
get(messageCount = 1) { | ||
// Don't get more than we have | ||
const count = Math.min(this.TEMP_LOG_LINES_TO_KEEP, messageCount); | ||
return this.logLines.slice(this.logLines.length - count); | ||
// If we're over the limit, flush the logs | ||
if (length > MAX_LOG_LINES_BEFORE_FLUSH || forceFlushToServer) { | ||
marcaaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.logToServer() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB missing semi colon |
||
} | ||
} | ||
|
||
/** | ||
|
@@ -81,60 +73,51 @@ export default class Logger { | |
* @param {String} message The message to log. | ||
* @param {Boolean} sendNow if true, the message will be sent right away. | ||
* @param {Object|String} parameters The parameters to send along with the message | ||
* @param {Boolean} onlyFlushWithOthers A request will never be sent to the server if all loglines have this set to true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this param for 2 reasons:
|
||
*/ | ||
info(message, sendNow, parameters) { | ||
if (sendNow) { | ||
const msg = `[info] ${message}`; | ||
this.logToServer(msg, 0, parameters); | ||
} else { | ||
this.add(message, parameters); | ||
} | ||
info(message, sendNow = false, parameters= '', onlyFlushWithOthers = false) { | ||
const msg = `[info] ${message}`; | ||
this.add(msg, parameters, sendNow, onlyFlushWithOthers); | ||
} | ||
|
||
/** | ||
* Logs an alert. | ||
* | ||
* @param {String} message The message to alert. | ||
* @param {Number} recentMessages A number of recent messages to append as context | ||
* @param {Object|String} parameters The parameters to send along with the message | ||
* @param {Boolean} includeStackTrace Must be disabled for testing | ||
*/ | ||
alert(message, recentMessages, parameters = {}, includeStackTrace = true) { | ||
alert(message, parameters = {}, includeStackTrace = true) { | ||
marcaaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const msg = `[alrt] ${message}`; | ||
const params = parameters; | ||
|
||
if (includeStackTrace) { | ||
params.stack = JSON.stringify(new Error().stack); | ||
} | ||
|
||
this.logToServer(msg, recentMessages, params); | ||
this.add(msg, params); | ||
this.add(msg, params, true); | ||
} | ||
|
||
/** | ||
* Logs a warn. | ||
* | ||
* @param {String} message The message to warn. | ||
* @param {Number} recentMessages A number of recent messages to append as context | ||
* @param {Object|String} parameters The parameters to send along with the message | ||
*/ | ||
warn(message, recentMessages, parameters) { | ||
warn(message, parameters = '') { | ||
const msg = `[warn] ${message}`; | ||
this.logToServer(msg, recentMessages, parameters); | ||
this.add(msg, parameters); | ||
this.add(msg, parameters, true); | ||
} | ||
|
||
/** | ||
* Logs a hmmm. | ||
* | ||
* @param {String} message The message to hmmm. | ||
* @param {Number} recentMessages A number of recent messages to append as context | ||
* @param {Object|String} parameters The parameters to send along with the message | ||
*/ | ||
hmmm(message, recentMessages, parameters) { | ||
hmmm(message, parameters = '') { | ||
const msg = `[hmmm] ${message}`; | ||
this.logToServer(msg, recentMessages, parameters); | ||
this.add(msg, parameters); | ||
this.add(msg, parameters, false); | ||
} | ||
|
||
/** | ||
|
@@ -143,8 +126,6 @@ export default class Logger { | |
* @param {String} message The message to log. | ||
*/ | ||
client(message) { | ||
this.add(message); | ||
|
||
if (!this.clientLoggingCallback) { | ||
return; | ||
} | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are now a bit shitty, since the logPacket contains a timestamp, I don't know how to make this test a bit better (like check its including the actual log lines).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jest functions can have their arguments inspected so that might be one way?
e.g.
mockServerLoggingCallback.mock.calls
should return an array of arrays representing each calls arguments. So to get the first argument of the first call you'd doThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then we can do like
expect(args).toEqual({something: 'blah'})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice, did not know that, will try it