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

Add basic_logger utility #22

Merged
merged 2 commits into from
Jul 7, 2022
Merged

Add basic_logger utility #22

merged 2 commits into from
Jul 7, 2022

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Jun 9, 2022

Description

This implements a thin wrapper around logging.basicConfig, addressing the show-stopper that basicConfig only configures the root logger.

I copied the basicConfig docs and then customized/enhanced them a bit.

@javierggt - what do you think? It's a bit of a hack but maybe works for us?

Interface impacts

None

Testing

Unit tests

  • No unit tests

Functional tests

In [1]: from ska_helpers.logging import basic_logger

In [2]: logger = basic_logger('me')

In [3]: logger.info('asdf')

In [5]: logger.warning('asdf')
2022-06-09 07:15:47,349 <module>: asdf

In [6]: logger = basic_logger('me', level='INFO')

In [7]: logger.info('asdf')

In [8]: logger = basic_logger('me', level='INFO', force=True)

In [9]: logger.info('asdf')
2022-06-09 07:16:25,243 <module>: asdf

In [10]: logger = basic_logger('me', level='INFO', force=True, format=None)

In [11]: logger.info('asdf')
INFO:me:asdf

Sorry, something went wrong.

@taldcroft taldcroft requested review from javierggt and jeanconn June 9, 2022 11:22
Copy link
Collaborator

@javierggt javierggt left a comment

Choose a reason for hiding this comment

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

I think it is ok. I checked that it works as advertised.

I would note that this does not help in the more complicated logging cases, but it is good as a replacement of pyyaks with what I think is a more predictable behavior.

My first thought was "what about something else changes in the global scope of the logging module while it is monkey-patched?". I checked, and there is not much else in the global scope. It is still something to keep in mind in the future.

My only quibble is that perhaps it should go into a new sub-module ska_helpers.logging. Otherwise this utils thing will eventually become a mix-match of tools and functions. It is also possible that we come up with other logging tools. For example, aca_view has a logging module with some logging utils that we could move into ska_helpers if we ever wanted to use them elsewhere (https://github.com/sot/aca_view/blob/master/aca_view/logging.py).

@taldcroft
Copy link
Member Author

It is still something to keep in mind in the future.

Yeah, I would not recommend this function for fancy logging or in a multiprocess/multithread applications. But for the simple things it should be better than pyyaks.

I'll move this to a new logging module.

@taldcroft
Copy link
Member Author

@javierggt - I implemented your suggestion and added something to the docstring about possible multi-thread/process issues.

I re-did the functional testing shown in the description and got the same results.

@taldcroft taldcroft merged commit f84c044 into master Jul 7, 2022
@taldcroft taldcroft deleted the basic-logger branch July 7, 2022 19:16
This was referenced Oct 5, 2022
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

2 participants