Skip to content

Use rich logging for Pelican server#2927

Merged
justinmayer merged 2 commits into
getpelican:masterfrom
minchinweb:rich-server
Oct 5, 2021
Merged

Use rich logging for Pelican server#2927
justinmayer merged 2 commits into
getpelican:masterfrom
minchinweb:rich-server

Conversation

@minchinweb

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

avaris commented Oct 2, 2021

Copy link
Copy Markdown
Member
  • 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
Copy Markdown
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
Comment thread pelican/server.py Outdated
@minchinweb

Copy link
Copy Markdown
Contributor Author

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

@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

@justinmayer

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

3 participants