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

Use rich logging for Pelican server #2927

Merged
merged 2 commits into from Oct 5, 2021

Conversation

MinchinWeb
Copy link
Contributor

Further to #2897, this enables rich logging for `pelican.server.

image

Surprisingly (at least to me), all that was needed was to import console from pelican.log.

Note this is working for python -m pelican.server, but doesn't seem to work on pelican -l. The latter seems to be drawing its logging from elsewhere.

P.S. If this could be marked "hacktoberfest-accepted" (so it counts towards my Hacktoberfest Pull Requests), that would be appreciated!

@avaris
Copy link
Member

avaris commented Oct 2, 2021

  • Linter is not happy (unused import)
  • pelican -l bits are here
  • This is a very limited support for server. All the messages the server outputs are rather "poor". It can be "enriched" by overriding log_message method in the subclass and using logging to display the output.

@MinchinWeb
Copy link
Contributor Author

Thanks for the tips @avaris ; I knew initially it was somewhat limited. I've fixed the three items you pointed to!

image

Previously the logging provided both the local address and the time. Time is still there (although without the date) but I've dropped the local address as it is provided on starting and should never change; plus is keeps the log messages more on point.

Are we good to merge?

@MinchinWeb MinchinWeb force-pushed the rich-server branch 2 times, most recently from 16167b9 to 17888bf Compare October 2, 2021 14:00
pelican/server.py Outdated Show resolved Hide resolved
@MinchinWeb
Copy link
Contributor Author

I've also set the minimum logging level to INFO, so that the server requests are actually shown.

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

@justinmayer
Copy link
Member

@MinchinWeb: I don't have time today to check this in detail, but can you confirm that the output is not hard-wrapped? c.f., https://rich.readthedocs.io/en/latest/console.html#soft-wrapping

I ask because I think I noticed the other day that the new Rich-powered logging is now hard-wrapped, and if it is indeed now hard-wrapped, I think we should change that behavior as well.

@justinmayer
Copy link
Member

Disregard my last comment. The culprit was an outdated tasks.py file that did not have PTY set correctly.

Many thanks to @MinchinWeb for continuing to make Pelican's output richer and to @avaris for the review. 👏

@justinmayer
Copy link
Member

@MinchinWeb: One more thing... Could you squash the first four commits into one commit, leaving the "set minimum logging level to INFO" commit as a separate commit? That way, there should be two discrete commits in this PR once your squash is completed. (Normally I would do this manually outside the PR, but I want to ensure you get your Hacktoberfest credit.) Thank you!

@justinmayer
Copy link
Member

@MinchinWeb: Any chance you could squash four of the five commits here, as noted above? I'd like to publish a follow-up release today, and it'd be nice to include this while ensuring you get Hacktoberfest credit.

@justinmayer
Copy link
Member

Thanks again, @MinchinWeb!

@justinmayer justinmayer merged commit b5426fb into getpelican:master Oct 5, 2021
@MinchinWeb MinchinWeb deleted the rich-server branch October 5, 2021 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants