Skip to content

Rich logging#2897

Merged
justinmayer merged 8 commits into
getpelican:masterfrom
minchinweb:rich-logging
Sep 29, 2021
Merged

Rich logging#2897
justinmayer merged 8 commits into
getpelican:masterfrom
minchinweb:rich-logging

Conversation

@minchinweb

Copy link
Copy Markdown
Contributor

Further to #2869, this expands rich's usage by extending it to cover logging.

This has the side effect of somewhat simplifying the logging code we need "in house" as some of this is pushed to rich.

Current (v4.6.0) logging:

pelican-old-logging

Logging with this PR:

pelican-rich-logging

Pull Request Checklist

  • Ensured tests pass and (if applicable) updated functional test output
  • Conformed to code style guidelines by running appropriate linting tools
  • [ n/a ] Added tests for changed code
  • [ n/a ] Updated documentation for changed code

Re formatting: On my side I'm showing quite a large number of files that would be reformatted by black and isort. I'm not going to make those changes here...

@minchinweb minchinweb mentioned this pull request Jul 1, 2021
2 tasks
@avaris

avaris commented Jul 2, 2021

Copy link
Copy Markdown
Member

Shiny ✨ . I'll review once I have time to test this. By the way, does this help with #2896 ?

@minchinweb

Copy link
Copy Markdown
Contributor Author

Ah yes, this would close #2896!

The "Generating..." spinner is still there (if you're sharpeyed, you might see it in the above screen cast); the trick was to use the same console.

Comment thread pelican/__init__.py Outdated
@justinmayer justinmayer requested a review from avaris July 7, 2021 06:42

@avaris avaris left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left some comments, but overall looks really nice. Thanks!

Comment thread pelican/__init__.py Outdated
Comment thread pelican/__init__.py
Comment thread pelican/__init__.py Outdated
@minchinweb

minchinweb commented Jul 9, 2021

Copy link
Copy Markdown
Contributor Author

This also updates the formatting of the "Found Writer" debug message to match the Generators:

           DEBUG    Found generator: ArticlesGenerator (internal)                                        __init__.py:211
           DEBUG    Found generator: PagesGenerator (internal)                                           __init__.py:211
           DEBUG    Found generator: GPXGenerator (pelican.plugins.gpx_reader)                           __init__.py:211
           DEBUG    Found generator: StaticGenerator (internal)                                          __init__.py:211
...
           DEBUG    Found writer: XMLWriter (pelican.plugins.xml_writer.writer)                          __init__.py:228

@minchinweb

minchinweb commented Jul 9, 2021

Copy link
Copy Markdown
Contributor Author

I've also expanded the use the rich.console to the --print-settings CLI option, particularly when there is an error in your pelicanconf.py.

General errors (as run through the FatalLogger / LimitLogger) only display the error message but not the error class. I'm not sure how to "fix" this, but it was "broken" before I started...

@avaris

avaris commented Jul 10, 2021

Copy link
Copy Markdown
Member

General errors (as run through the FatalLogger / LimitLogger) only display the error message but not the error class. I'm not sure how to "fix" this, but it was "broken" before I started...

Which ones are those? If you mean logger.error(...) cases, then those are only logs, they are not necessarily associated with an exception, so no error class.

@minchinweb

Copy link
Copy Markdown
Contributor Author

General errors (as run through the FatalLogger / LimitLogger) only display the error message but not the error class. I'm not sure how to "fix" this, but it was "broken" before I started...

Which ones are those? If you mean logger.error(...) cases, then those are only logs, they are not necessarily associated with an exception, so no error class.

Yes, it was logger.error() cases, although I can't remember exactly which ones at the moment.

In any case, I didn't change the behaviour, it just seemed off at times.

@justinmayer

Copy link
Copy Markdown
Member

Unrelated note: Maybe we could add support for Better Exceptions, much like my friend Hynek just did for Structlog.

@avaris

avaris commented Jul 15, 2021

Copy link
Copy Markdown
Member

Somewhat related note: The only remaining non-rich output is the server.py (and a print in it's counterpart in listen function from __init__.py). To make the server use console, simply defining a log_message method in the handler and replace the original sys.stderr.write with console.print or console.log would be enough.

I was also thinking about making autoreload message a status/spinner as well but -lr (--listen --autoreload) makes things problematic. Even if you "share" console, server messages disrupt status and results in similar issue as #2896. I'm 99% certain that the culprit is multiprocessing and in fact the console instances are not really same, but solution will be tricky.

@justinmayer

Copy link
Copy Markdown
Member

@avaris: Any chance you could take another look at this and update your review with any remaining items to be addressed, if any?

@justinmayer

Copy link
Copy Markdown
Member

@avaris: What do you think we should do in terms of moving this forward?

@avaris avaris left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks

@avaris

avaris commented Sep 28, 2021

Copy link
Copy Markdown
Member

Sorry about that, I think this is good as is. We can always add further enhancements later.

@justinmayer

Copy link
Copy Markdown
Member

@avaris: No need to be sorry! You raised some good points, which hopefully we can use as guidance for further improvements in the future.

@minchinweb: Many thanks for these enhancements! Very cool, indeed. 💯

@justinmayer justinmayer merged commit 7ccaa9a into getpelican:master Sep 29, 2021
@minchinweb minchinweb deleted the rich-logging branch September 29, 2021 13:05
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.

4 participants