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

Replace print by the Python logging framework #2067

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vdbergh
Copy link
Contributor

@vdbergh vdbergh commented Jun 12, 2024

We use the logger "fishtest" as configured in production.ini.

For messages to the event log we use the child logger "fishtest.event", also configured in production.ini.

Log messages are formatted as follows:

INFO [fishtest] [waitress-0:rundb.py:failed_task:1446] <...message...>

Also: move scheduler to a separate file scheduler.py.

@vdbergh vdbergh changed the title Replace print by the Python logging framework Replace print by the Python logging framework (RFC) Jun 12, 2024
@vdbergh
Copy link
Contributor Author

vdbergh commented Jun 12, 2024

This is the code for the custom handler...

class EventHandler(logging.Handler):
    def __init__(self):
        super().__init__(level=logging.ERROR)
        logging.disable(level=logging.CRITICAL)
	rundb = RunDb(is_primary_instance=False)
        logging.disable(level=logging.NOTSET)
        self.actiondb = rundb.actiondb

    def emit(self, record):
        message=self.format(record)
	self.actiondb.log_message(
            username="fishtest.system",
            message=message,
        )

@vdbergh
Copy link
Contributor Author

vdbergh commented Jun 12, 2024

The logging section of production.ini

[loggers]
keys = root, fishtest

[handlers]
keys = console, events

[formatters]
keys = generic

[logger_root]
level = DEBUG
handlers = console

[logger_fishtest]
level = DEBUG
handlers = events
qualname = fishtest

[handler_console]
class = StreamHandler
args = (sys.stderr,)
level = NOTSET
formatter = generic

[handler_events]
class = fishtest.rundb.EventHandler
formatter = generic

[formatter_generic]
format = %(levelname)-5.5s [%(name)s][%(threadName)s] %(message)s

@ppigazzini
Copy link
Collaborator

With DEBUG the systemd log is flooded

[logger_fishtest]
level = INFO
handlers = events
qualname = fishtest

I wonder if this is a simpler way to do the intended action...

class EventHandler(logging.Handler):
    def __init__(self):
        super().__init__(level=logging.ERROR)
        rundb = RunDb(is_primary_instance=False)
        self.actiondb = rundb.actiondb

    def emit(self, record):
        if record.levelno >= self.level:  # Check if message level is ERROR or higher
            message = self.format(record)
            self.actiondb.log_message(
                username="fishtest.system",
                message=message,
            )

@vdbergh
Copy link
Contributor Author

vdbergh commented Jun 12, 2024

I am not sure I understand. The handler is already created with level equal to level.ERROR. I don’t understand the need for a further test in emit().

I am AFK but if you want to raise the log levels from DEBUG to INFO then please do!

@vdbergh
Copy link
Contributor Author

vdbergh commented Jun 13, 2024

I changed the log levels from DEBUG to INFO and fixed a typo.

@vdbergh
Copy link
Contributor Author

vdbergh commented Jun 13, 2024

I now reverted the root level to ERROR and set the fishtest level to DEBUG so that we can use logger.debug(message) inside Fishtest (for example to instrument code). According to the specs, this is how it should be.

@vdbergh
Copy link
Contributor Author

vdbergh commented Jun 13, 2024

Squashed.

@vdbergh vdbergh force-pushed the log_new branch 2 times, most recently from 2bb0a68 to e9bb803 Compare June 13, 2024 14:00
@vdbergh vdbergh changed the title Replace print by the Python logging framework (RFC) Replace print by the Python logging framework Jun 13, 2024
@vdbergh vdbergh force-pushed the log_new branch 3 times, most recently from f042b76 to e35cce7 Compare June 14, 2024 05:52
@vdbergh vdbergh force-pushed the log_new branch 3 times, most recently from dfd1f33 to 34abf06 Compare June 18, 2024 22:31
@vdbergh
Copy link
Contributor Author

vdbergh commented Jun 19, 2024

I switched back to using a single main logger (this pattern is also used by other packages). The main logger is the logger fishtest which is a child of the root logger which is preconfigured in production.ini. Using a different child logger of fishtest per source file did not seem to offer any benefits and moreover it complicates some things as we are also using a logger which logs to the event log. In this PR this logger is called fishtest.event. With module level logging it should be called __name__.event but then AFAICS it is no longer possible to configure it in production.ini (wildcards in keys appear not to work).

@vdbergh vdbergh force-pushed the log_new branch 2 times, most recently from f974b03 to a601fb7 Compare June 25, 2024 16:57
@vdbergh
Copy link
Contributor Author

vdbergh commented Jun 25, 2024

Rebased.

@vdbergh
Copy link
Contributor Author

vdbergh commented Jul 1, 2024

Rebased

We use the logger "fishtest" as configured in production.ini.

For messages to the event log we use the child logger
"fishtest.event", also configured in "production.ini".

Log messages are formatted as follows:

INFO [fishtest] [waitress-0:rundb.py:failed_task:1446] <...message...>

Also: move scheduler to a separate file scheduler.py.
@vdbergh
Copy link
Contributor Author

vdbergh commented Jul 7, 2024

@ppigazzini Could you maybe have a look at this PR? It seems like a nobrainer (replacing print by logger.info) and tying into the Python logger framework has many benefits for interoperability (I plan to do the same for the worker).

But the real reason I am mentioning this is the following: I would like to do some work on the scheduler. This PR moves the scheduler to a separate file scheduler.py. That makes this PR difficult to rebase when there are changes to the scheduler in master as they have to be copied manually to the new file which is very error prone.

@ppigazzini
Copy link
Collaborator

I'm working in stealth mode in this weekend: trying new nginx/systemd settings, instrumenting on the fly the code, documenting in the setup script the new setting successfully tested.

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.

2 participants