Skip to content

Fix to ensure that asyncio uses SaltLoggingClass #68401

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: 3007.x
Choose a base branch
from
+27 −0
Open

Fix to ensure that asyncio uses SaltLoggingClass #68401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/68400.fixed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixes issue with asyncio logger not using SaltLoggingClass and causing exceptions when "%(jid)s" is used in a log format.
7 changes: 7 additions & 0 deletions salt/_logging/impl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,13 @@ def shutdown_temp_handler():
logging.root.addHandler(get_temp_handler())


# Override asyncio logger class if asyncio is already imported
if "asyncio" in sys.modules:
asyncio_logger = logging.getLogger("asyncio")
if not isinstance(asyncio_logger, SaltLoggingClass):
asyncio_logger.__class__ = SaltLoggingClass
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really the correct/ideal way to do this? Shouldn't a the logger be created/instantiated as a SaltLoggingClass and assigned to the logger instead of mucking with the internal attributes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The logger for asyncio isn't being created in salt - it's created within asyncio and happens as soon as you import it:

>>> import logging
>>> logging.root.getChildren()
set()
>>> import asyncio
>>> logging.root.getChildren()
{<Logger asyncio (WARNING)>}
>>>

As I noted above, the other option is to try to make sure that asyncio is imported after salt._logging, but that is somewhat fragile and it also doesn't work in the test suite where asyncio gets imported by pytest (or maybe one of the pytest extensions..)

I don't love it as a solution, but I can't see a better way to do it - open to suggestions if you can.

Copy link
Contributor

@bdrx312 bdrx312 Oct 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Would something like this quick untested rough draft (with the help from llm) work?

import logging

def ensure_asyncio_uses_salt_logger(SaltLoggingClass):
    name = "asyncio"
    mgr = logging.root.manager
    # Preserve any existing logger settings
    existing = mgr.loggerDict.get(name)
    handlers = getattr(existing, "handlers", []) if isinstance(existing, logging.Logger) else []
    level = getattr(existing, "level", logging.NOTSET) if isinstance(existing, logging.Logger) else logging.NOTSET
    propagate = getattr(existing, "propagate", True) if isinstance(existing, logging.Logger) else True

    # Create SaltLoggingClass logger instance
    salt_logger = SaltLoggingClass(name)
    # Copy preserved state
    salt_logger.handlers[:] = handlers[:]    # shallow copy handlers list
    salt_logger.setLevel(level)
    salt_logger.propagate = propagate

    # Replace manager entry so logging.getLogger(name) returns salt_logger
    mgr.loggerDict[name] = salt_logger

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, although that won't entirely solve the problem. Whilst that will replace the logger for asyncio in the logging manager and any new calls to logging.getLogger("asyncio") will return the new logger, the logger used by the asyncio module has already been assigned, so it will continue to use the old logger object.

We could do something like this and then update asyncio.logger I guess. That said, I'm wondering if it makes more sense to move the code that enriches the log records (ie adding jid) out of the SaltLoggerClass and into a filter that can be applied to existing logging classes. Will have a play about with that to see how much effort that'd be.



# Now that we defined the default logging logger class, we can instantiate our logger
# DO NOT MOVE THIS
log = logging.getLogger(__name__)
Expand Down
19 changes: 19 additions & 0 deletions tests/pytests/unit/_logging/test_asyncio_logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
"""
Tests to ensure asyncio logger uses SaltLoggingClass
"""

import logging


def test_asyncio_logger_saltloggingclass():
"""
Test that the asyncio logger is an instance of SaltLoggingClass

It is imported before salt._logging so we need to ensure it is overridden
"""

asyncio_logger = logging.getLogger("asyncio")

import salt._logging.impl

assert isinstance(asyncio_logger, salt._logging.impl.SaltLoggingClass)