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

Rich logging #2897

Merged
merged 8 commits into from
Sep 29, 2021
Merged

Rich logging #2897

merged 8 commits into from
Sep 29, 2021

Conversation

MinchinWeb
Copy link
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
Copy link
Member

avaris commented Jul 2, 2021

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

@MinchinWeb
Copy link
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.

pelican/__init__.py Outdated Show resolved Hide resolved
@justinmayer justinmayer requested a review from avaris July 7, 2021 06:42
Copy link
Member

@avaris avaris left a comment

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!

pelican/__init__.py Outdated Show resolved Hide resolved
pelican/__init__.py Show resolved Hide resolved
pelican/__init__.py Outdated Show resolved Hide resolved
@MinchinWeb
Copy link
Contributor Author

MinchinWeb commented Jul 9, 2021

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
Copy link
Contributor Author

MinchinWeb commented Jul 9, 2021

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
Copy link
Member

avaris commented Jul 10, 2021

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
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
Member

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

@avaris
Copy link
Member

avaris commented Jul 15, 2021

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
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
Member

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

Copy link
Member

@avaris avaris left a comment

Choose a reason for hiding this comment

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

Thanks

@avaris
Copy link
Member

avaris commented Sep 28, 2021

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

@justinmayer
Copy link
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