-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
Fix the log levels mapping #597
Open
Jakuje
wants to merge
3
commits into
ansible:devel
Choose a base branch
from
Jakuje:patch-1
base: devel
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+189
−17
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fixed log level mapping to cover whole libssh range -- by :user:`Jakuje`. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# distutils: libraries = ssh | ||
# | ||
# This file is part of the pylibssh library | ||
# | ||
# This library is free software; you can redistribute it and/or | ||
# modify it under the terms of the GNU Lesser General Public | ||
# License as published by the Free Software Foundation; either | ||
# version 2.1 of the License, or (at your option) any later version. | ||
# | ||
# This library is distributed in the hope that it will be useful, | ||
# but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
# Lesser General Public License for more details. | ||
# | ||
# You should have received a copy of the GNU Lesser General Public | ||
# License along with this library; if not, see file LICENSE.rst in this | ||
# repository. | ||
# | ||
from pylibsshext.includes cimport callbacks, libssh | ||
|
||
|
||
cdef class Logging: | ||
pass |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
# | ||
# This file is part of the pylibssh library | ||
# | ||
# This library is free software; you can redistribute it and/or | ||
# modify it under the terms of the GNU Lesser General Public | ||
# License as published by the Free Software Foundation; either | ||
# version 2.1 of the License, or (at your option) any later version. | ||
# | ||
# This library is distributed in the hope that it will be useful, | ||
# but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
# Lesser General Public License for more details. | ||
# | ||
# You should have received a copy of the GNU Lesser General Public | ||
# License along with this library; if not, see file LICENSE.rst in this | ||
# repository. | ||
|
||
import logging | ||
|
||
from pylibsshext.errors cimport LibsshSessionException | ||
|
||
|
||
ANSIBLE_PYLIBSSH_TRACE = 5 | ||
|
||
LOG_MAP = { | ||
logging.NOTSET: libssh.SSH_LOG_NONE, | ||
ANSIBLE_PYLIBSSH_TRACE: libssh.SSH_LOG_TRACE, | ||
logging.DEBUG: libssh.SSH_LOG_DEBUG, | ||
logging.INFO: libssh.SSH_LOG_INFO, | ||
logging.WARNING: libssh.SSH_LOG_WARN, | ||
logging.ERROR: libssh.SSH_LOG_WARN, | ||
logging.CRITICAL: libssh.SSH_LOG_WARN | ||
} | ||
|
||
LOG_MAP_REV = { | ||
libssh.SSH_LOG_NONE: logging.NOTSET, | ||
libssh.SSH_LOG_TRACE: ANSIBLE_PYLIBSSH_TRACE, | ||
libssh.SSH_LOG_DEBUG: logging.DEBUG, | ||
libssh.SSH_LOG_INFO: logging.INFO, | ||
libssh.SSH_LOG_WARN: logging.WARNING, | ||
} | ||
|
||
logger = logging.getLogger("libssh") | ||
|
||
|
||
def add_trace_log_level(): | ||
level_num = ANSIBLE_PYLIBSSH_TRACE | ||
level_name = "TRACE" | ||
method_name = level_name.lower() | ||
logger_class = logging.getLoggerClass() | ||
|
||
if hasattr(logging, level_name): | ||
raise AttributeError('{} already defined in logging module'.format(level_name)) | ||
if hasattr(logging, method_name): | ||
raise AttributeError('{} already defined in logging module'.format(method_name)) | ||
if hasattr(logger_class, method_name): | ||
raise AttributeError('{} already defined in logger class'.format(method_name)) | ||
|
||
def logForLevel(self, message, *args, **kwargs): | ||
if self.isEnabledFor(level_num): | ||
self._log(level_num, message, args, **kwargs) | ||
|
||
def logToRoot(message, *args, **kwargs): | ||
logging.log(level_num, message, *args, **kwargs) | ||
|
||
logging.addLevelName(level_num, level_name) | ||
setattr(logging, level_name, level_num) | ||
setattr(logging, method_name, logToRoot) | ||
setattr(logger_class, method_name, logForLevel) | ||
|
||
|
||
cdef void _pylibssh_log_wrapper(int priority, | ||
const char *function, | ||
const char *buffer, | ||
void *userdata) noexcept nogil: | ||
with gil: | ||
log_level = LOG_MAP_REV[priority] | ||
logger.log(log_level, f"{buffer}") | ||
|
||
|
||
def set_log_callback(): | ||
callbacks.ssh_set_log_callback(_pylibssh_log_wrapper) | ||
|
||
|
||
def logging_init(): | ||
try: | ||
add_trace_log_level() | ||
except AttributeError: | ||
pass | ||
set_log_callback() | ||
|
||
|
||
def set_level(level): | ||
logging_init() | ||
if level in LOG_MAP.keys(): | ||
rc = libssh.ssh_set_log_level(LOG_MAP[level]) | ||
if rc != libssh.SSH_OK: | ||
raise LibsshSessionException("Failed to set log level [%d] with error [%d]" % (level, rc)) | ||
logger.setLevel(level) | ||
else: | ||
raise LibsshSessionException("Invalid log level [%d]" % level) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This doesn't sound right. Why not just add another level called
TRACE
mapped to a low numeric value like5
? (DEBUG
is10
in stdlib)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.
I am not much knowledgeable about python so sorry if I miss something.
How would callers discover and set such custom log level?
As I can see from
ansible-collections/ansible.netcommon
this is called likeWould it mean we should define a new list of log levels that could be used instead of the standard one? I can certainly do that if it would be preferred. I would consider it as quite a barrier to cross for the users updating from older version to learn there is one more non-standard log level and how to set it.
This was an attempt to provide full mapping of standard log levels to libssh log levels for the price of lower granularity on the verbose end (the difference between DEBUG and TRACE is not that significant, but the later is much more useful).
If you want full granularity, maybe placing the libssh
SSH_LOG_DEBUG
between theDEBUG
andINFO
with value lets say15
might be better to simplify the barrier to set up the most verbose debug level?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.
I'm leaning towards custom log level, but a precise one.
There's a warning recommending not to add custom log levels to libraries, though: https://docs.python.org/3/howto/logging.html#custom-levels.
Somewhere on the page, it's also suggested that https://docs.python.org/3/library/logging.html#logging.Logger.log should be used for such cases.
And https://docs.python.org/3/library/logging.html#logging.addLevelName exists for declaring additional log levels.
I wonder if we should declare some
ANSIBLE_PYLIBSSH_
-prefixed levels for custom in-between stuff. We could then also expose importable constants that the end-users of the library would be able to use w/o clushing with stdlib. This would address the concern of that stdlib docs warning.I don't think we should shift what
DEBUG
means. AndSSH_LOG_DEBUG
shouldn't be reassigned to "something between debug and info". Instead, a newANSIBLE_PYLIBSSH_TRACE
could exist that would be represented by a number below10
.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.
I don't know. I am not that much of a python programmer. The first linked paragraph explicitly discourages the use of custom log levels in libraries, but I did not dive into that deep enough.
My reasoning for shifting the stdlib DEBUG to libssh TRACE is that the DEBUG is the most verbose logging in stdlib and therefore it should map to the most verbose log level in libssh. If it does not, setting the "most verbose" log level becomes more complicated for users/developers using the pylibssh and debugging further issues that require more verbose log will become harder for us, libssh developers.
So if you have some idea/strong opinion how this should work, please, feel free to propose the changes.
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.
What I outlined above seems to be the way to go, API-wise. If we start re-assigning the log levels and create mismatches, that would cause more confusion, in my opinion. This library mostly supposed to be a way to re-expose the binding, not re-define the API though the own facade. New log levels would have to be exposed through an accessible import module. I believe that something like
from pylibsshext.logging import ANSIBLE_PYLIBSSH_TRACE
is ergonomic enough and aligns with the pythonic principles/expectations.