-
Notifications
You must be signed in to change notification settings - Fork 201
Fix log prefix hijack in c_cmis.py #609
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
base: master
Are you sure you want to change the base?
Conversation
If any code imports c_cmis.py (indirectly for the most part) and has a logger session open already, their existing session will be re-opened with a new log tag, CCmisApi which is not desired. To fix this we match the logging in c_cmis.py with logging in neighboring files, it doesn't do any syslogging anymore. Libraries (rather than executables) probably shouldn't be syslogging on their own, and they certainly shouldn't hijack the log tag of the including executable.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@prgeor and @mihirpat1 would you mind taking a look please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a logging issue where the c_cmis.py library file hijacks the syslog identifier of any importing executable by removing unused logging code and dead functions.
Key Changes:
- Removed unused logger import and logging infrastructure that was overriding syslog identifiers
- Removed dead code (
_update_dict_if_vdm_key_existsmethod) that became unused in a previous PR - Removed unused
BYTELENGTHconstant
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Fix logging in c_cmis.py to match neighboring files and to stop hijacking the syslog tag of any importing executables.
Motivation and Context
In python services using
sonic_py_common.loggerthe syslog identifier already set up by the executable/service can be overwritten when importing other modules.This log hijack is coming from a library:
sonic_platform_base/sonic_xcvr/api/public/c_cmis.pySearching in the syslog of a switch running SONiC we see logs with the
CCmisApithat shouldn't:If any code imports c_cmis.py (indirectly for the most part) and has a logger session open already, their existing session will be re-opened with a new log tag,
CCmisApiwhich is not desired.To fix this we remove this logging code entirely because the function where it is used became dead in #556
Libraries (rather than executables) probably shouldn't be syslogging on their own, and they certainly shouldn't hijack the log tag of the including executable.
Resolves sonic-net/sonic-buildimage#24489
How Has This Been Tested?
Before the fix:
Open an interactive python session and run this code:
Watch the syslog output:
Clearly the second log should also have had the tag
NATEAfter the fix:
Syslog entries should have been:
Additional Information (Optional)