Skip to content
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

Merged
merged 10 commits into from Nov 8, 2018
Merged

Improve logging #2781

merged 10 commits into from Nov 8, 2018

Conversation

matthewrmshin
Copy link
Contributor

@matthewrmshin matthewrmshin commented Oct 3, 2018

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:

  • Just log/suite/log*.
  • Improved calls to logger. E.g.:
    • Exceptions are logged using the logger.exception method.
    • Log with format string + positional arguments instead of pre-formatted string.
  • Can, in theory, expose log form cherrypy. (Not done.)
  • One log handler class (mainly to handle rotation with time-stamped files) and one log formatter class. (The latter should be unnecessary when we move to Python 3.)
  • The --verbose and --debug options are now used to control the level of the logger.
  • Log header reprinted on top of continuation log files - useful information when inspecting the logs for a long running suite. E.g.:
    • Suite URL and PID.
    • Restart number and log file number.
    • Initial/final cycle point, etc.

(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.

@matthewrmshin matthewrmshin added this to the next release milestone Oct 3, 2018
@matthewrmshin matthewrmshin self-assigned this Oct 3, 2018
@cylc cylc deleted a comment Oct 3, 2018
@cylc cylc deleted a comment Oct 3, 2018
@cylc cylc deleted a comment Oct 3, 2018
@cylc cylc deleted a comment Oct 8, 2018
@cylc cylc deleted a comment Oct 8, 2018
@cylc cylc deleted a comment Oct 16, 2018
@cylc cylc deleted a comment Oct 16, 2018
@cylc cylc deleted a comment from matthewrmshin Oct 16, 2018
@hjoliver
Copy link
Member

(I'm intending to review this after #2609, which I'm part way through, since this is based on top of that).

@cylc cylc deleted a comment Oct 24, 2018
@cylc cylc deleted a comment Oct 24, 2018
@cylc cylc deleted a comment from kinow Oct 24, 2018
@cylc cylc deleted a comment from kinow Oct 24, 2018
@cylc cylc deleted a comment from matthewrmshin Oct 24, 2018
@hjoliver
Copy link
Member

@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?

@matthewrmshin
Copy link
Contributor Author

@hjoliver done.

@matthewrmshin
Copy link
Contributor Author

(Actually, just found a logical conflict with the new cylc review.)

@matthewrmshin
Copy link
Contributor Author

(Should now have got it.)

@hjoliver
Copy link
Member

@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.

@kinow
Copy link
Member

kinow commented Oct 29, 2018

Sure thing @hjoliver , just finishing some work in another branch, then get some strong coffee and start reviewing this one 👍

Copy link
Member

@oliver-sanders oliver-sanders left a 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.

@matthewrmshin
Copy link
Contributor Author

#1958 was still worth it though. It was the first attempt to address #386. It also gave us a better foundation for the logging mechanism. The real wake up call was the discussion in #2505, however (about 16 months after #1958).

@matthewrmshin
Copy link
Contributor Author

(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.
@matthewrmshin
Copy link
Contributor Author

matthewrmshin commented Nov 7, 2018

Branch re-based + new commit:

  • Site test with latest changes.
  • Replace a few missed print and sys.*.write statements with logging statements - revealed during site tests with Auto stop-restart #2809.

@cylc cylc deleted a comment Nov 7, 2018
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Nice improvement; LGTM.

@hjoliver hjoliver merged commit 6b40dd3 into cylc:master Nov 8, 2018
@matthewrmshin matthewrmshin deleted the improve-logging branch November 9, 2018 09:43
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this pull request Nov 9, 2018
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this pull request Nov 9, 2018
matthewrmshin added a commit to matthewrmshin/rose that referenced this pull request Nov 9, 2018
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this pull request Nov 14, 2018
oliver-sanders added a commit to metomi/rose that referenced this pull request Nov 16, 2018
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this pull request Nov 21, 2018
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this pull request Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants