-
Notifications
You must be signed in to change notification settings - Fork 94
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 logging #2781
Improve logging #2781
Conversation
02c6a63
to
be23f92
Compare
be23f92
to
1aca704
Compare
(I'm intending to review this after #2609, which I'm part way through, since this is based on top of that). |
1aca704
to
8dc033d
Compare
@matthewrmshin - this now has conflicts. And post merge of #2609 perhaps you could rebase this branch to remove the spurious commits and make the review easier? |
6cca742
to
c508a5c
Compare
@hjoliver done. |
(Actually, just found a logical conflict with the new |
c508a5c
to
a1cc893
Compare
(Should now have got it.) |
@kinow - any chance you could review this today (since we're in a hurry to get the release out)? It is actually quite a straightforward change - mostly single liners, and some bulk code relocations. |
Sure thing @hjoliver , just finishing some work in another branch, then get some strong coffee and start reviewing this one 👍 |
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'm happy here, I only wish we had made this decision before #1958, it would have made life a lot easier!
My only outstanding comment is that we might want to put in a test to test the log file header and to ensure that no "INFO" lines get printed out before the header (which would break the deamonise logic) as this is particularly easy to break.
(We should merge #2809 before this one. The new logging logic needs to be propagated into the relocated host appointer module.) |
Basic stream logging for utility commands. Single log file for suite server program. Return control back to Python's logging module, where possible.
Print log header in continued log files. Print number of restarts and number of log files in current run. Print server URL as https://host:port/ instead of just host:port.
8ba81df
to
300fe7e
Compare
Branch re-based + new commit:
|
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.
Nice improvement; LGTM.
…cylc-2781 Fix tests broken by cylc/cylc-flow#2781 and cylc/cylc-flow#2849
This is built on top of #2609, so only the final 3 changesets are applicable, but I think it is now worth exposing for discussion.The rationale of this comes from the discussions of #2505. As a first step, we need to really simplify how we do logging, i.e. we should go back to mainly rely on the functionality of Python's logging module in the standard library where possible.
We now have:
log/suite/log*
.logger.exception
method.--verbose
and--debug
options are now used to control the level of the logger.(In the future, we should be able to take this change further to allow users to configure logging in their own way at suite or user/site level. I am not doing this yet so this change can still be used under Python 2.6 on some ancient platforms.)
Close #386.