-
Notifications
You must be signed in to change notification settings - Fork 2
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
FT: Logs should not be able to overwrite reserved fields #69
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -205,6 +205,19 @@ class RequestLogger { | |
this.elapsedTime = null; | ||
} | ||
|
||
/** | ||
* This function sets the name field for the request logger. | ||
* | ||
* @note As this is a protected field, it will not be copied/overwritten | ||
* while logging objects. | ||
* @param {string} name - name to be set for the request logger | ||
* @returns {string} - name field of the request logger | ||
*/ | ||
setName(name) { | ||
this.fields.name = name; | ||
return name; | ||
} | ||
|
||
/** | ||
* This function returns a copy of the local uid list, in order for | ||
* requests to a sub-component to be sent with the parent request's UID | ||
|
@@ -434,26 +447,26 @@ class RequestLogger { | |
}); | ||
return; | ||
} | ||
const fields = objectCopy({}, this.fields, logFields || {}); | ||
objectCopy(this.fields, logFields || {}); | ||
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. So we keep augmenting this.fields, alterating it, and thus corrupting future log entries ? |
||
const endFlag = isEnd || false; | ||
|
||
/* | ||
* using Date.now() as it's faster than new Date(). logstash component | ||
* uses this field to generate ISO 8601 timestamp | ||
*/ | ||
if (fields.time === undefined) { | ||
fields.time = Date.now(); | ||
if (this.fields.time === undefined) { | ||
this.fields.time = Date.now(); | ||
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. So, we're only setting one "date" for the whole Logger ? It sounds like a mistake to me. The previous condition helped avoid not setting the time, but we do not want to set it only once per logger. |
||
} | ||
|
||
fields.req_id = serializeUids(this.uids); | ||
this.fields.req_id = serializeUids(this.uids); | ||
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. So you want to update "this" for every log entry ? I'm not sure that's what you actually aim for ? (And I wouldn't see the point) |
||
if (endFlag) { | ||
this.elapsedTime = process.hrtime(this.startTime); | ||
fields.elapsed_ms = this.elapsedTime[0] * 1000 | ||
this.fields.elapsed_ms = this.elapsedTime[0] * 1000 | ||
+ this.elapsedTime[1] / 1000000; | ||
} | ||
const logEntry = { | ||
level, | ||
fields, | ||
fields: this.fields, | ||
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. Since we're setting a reference to this.fields, and we're updating this.fields for each log entry, this means that this object will be constantly modified. |
||
msg, | ||
}; | ||
this.entries.push(logEntry); | ||
|
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.
shouldn't return
this
instead ofname
?