-
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
WRLGS-11: Utils to timestamp stderr logs #167
WRLGS-11: Utils to timestamp stderr logs #167
Conversation
Hello bourgoismickael,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
34fef9b
to
e20f930
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command:
Alternatively, the |
/create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
/approve |
nyc coverage seems to slow down the process on first run The slow seems to vary between branches (due to nyc version)
All tags from 7.4.10 and 7.10.1.0 to 7.10.4.10 are all the same commit 23dfe7c
e20f930
to
4984e5b
Compare
/reset |
function catchAndTimestampUncaughtException( | ||
dateFct = defaultTimestamp, exitCode = 1, | ||
) { | ||
process.on('uncaughtException', (err, origin) => { | ||
printErrorWithTimestamp(err, origin, dateFct()); | ||
if (exitCode !== null) { | ||
process.nextTick(() => process.exit(exitCode)); | ||
} | ||
}); | ||
} |
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.
Would it make sense not to stop the process if we pass a flag? Example: https://github.com/scality/vault2/blob/6a0c2cf3de174f5f339f1379227fbe87b0e99ce7/lib/vaultd.js#L30
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.
I think it should be the default behavior to stop the process.
Most processes don't have any listener on this.
I plan to use simply:
require('werelogs').stderrUtils.catchAndTimestampStderr();
Some processes have, like cloudserver when it does not exit all the time and could end in undefined behavior.
/force_reset |
function printErrorWithTimestamp( | ||
err, origin, date = defaultTimestamp(), | ||
) { | ||
return process.stderr.write(`${date}: ${origin}:\n${err.stack}\n`); |
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.
If we plan to crash the process, why not logging with fatal
level here?
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.
Those are stderr logs, there is no Logger
to format those logs as json.
I copied what's done there: https://github.com/scality/cloudserver/pull/5406/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R7
And we decided for now we don't want to change the format completely, we want functions to encapsulate the logic and later if we need to we could change the format in those functions and just bump in all components
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.
We try not to change the behavior too much for now, we just need to include the timestamp in stderr for the CS
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: approve, create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: $ git fetch
$ git checkout -B w/8.0/improvement/WRLGS-11-stderr-timestamp origin/development/8.0
$ git merge origin/improvement/WRLGS-11-stderr-timestamp
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/8.0/improvement/WRLGS-11-stderr-timestamp The following options are set: approve, create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: $ git fetch
$ git checkout -B w/8.1/improvement/WRLGS-11-stderr-timestamp origin/development/8.1
$ git merge origin/w/8.0/improvement/WRLGS-11-stderr-timestamp
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/8.1/improvement/WRLGS-11-stderr-timestamp The following options are set: approve, create_integration_branches |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue WRLGS-11. Goodbye bourgoismickael. The following options are set: approve, create_integration_branches |
Define functions that adds a timestamp to stderr logs:
uncaughtException
andwarning
events.These functions should be used in every component, so if we want to refine the format of stderr logs, we can change only one function in one place
Based on assessment S3C-8936_logs_inventory
The versions of werelogs used by S3C components between
S3C 7.10.8
andS3C 9.4
are:hotfix/8.1.0
7.4.10
and7.10.1.0
to7.10.4.10
: they are all the same commit23dfe7c
Branches
hotfix/7.4.10
=hotfix/7.10.1
and above =development/7.10
development/8.0
with unused version8.0.0
in latest S3C assessedImportant
PR for
hotfix/8.1.0
⏭️ #168 ⏮️Note
Logs will look like this for uncaught errors
And like this for warnings
I'll remove this warnings in components also