Fix the log levels mapping #597
Conversation
The `INFO` log level is usually not enough to get useful information from the libssh regarding the low-level issues that we would like to be able to resolve. This can be made even more fine grained, but I do not think less verbose logs would be much useful anyway. Related is the issue in pylibssh, which maps the DEBUG verbosity to something that is not debug in libssh itself: ansible/pylibssh#597
| @@ -46,11 +46,11 @@ OPTS_DIR_MAP = { | |||
|
|
|||
| LOG_MAP = { | |||
| logging.NOTSET: libssh.SSH_LOG_NONE, | |||
| logging.DEBUG: libssh.SSH_LOG_DEBUG, | |||
There was a problem hiding this comment.
This doesn't sound right. Why not just add another level called TRACE mapped to a low numeric value like 5? (DEBUG is 10 in stdlib)
There was a problem hiding this comment.
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 like
self.ssh.set_log_level(logging.INFO)
Would 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 the DEBUG and INFO with value lets say 15 might be better to simplify the barrier to set up the most verbose debug level?
There was a problem hiding this comment.
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. And SSH_LOG_DEBUG shouldn't be reassigned to "something between debug and info". Instead, a new ANSIBLE_PYLIBSSH_TRACE could exist that would be represented by a number below 10.
There was a problem hiding this comment.
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.
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.
|
It'd be useful to have tests in such PRs. I'm unable to verify what this patch affects without them. |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
psf-chronographer
bot
added
the
bot:chronographer:provided
| @@ -0,0 +1 @@ | |||
| Fix log level mapping to cover whole libssh range -- by :user:`Jakuje` | |||
There was a problem hiding this comment.
This would need to have some context for the end-users, past tense (or non-imperative present simple with the reference to previous release) and a trailing period for as long as the byline is within the sentence.
tests/unit/session_test.py
Outdated
| def test_session_log_level(): | ||
| """Test setting the log level.""" | ||
| ssh_session = Session() | ||
| assert ssh_session |
There was a problem hiding this comment.
This doesn't seem necessary. I'd instead expect some log-related checks. Perhaps, using the caplog fixture that pytest provides..
The libssh provides the most verbose logging with SSH_LOG_TRACE, which was not mapped to any of the standard values so the users are unable to get full debug logs. These are critical for libssh developers to be able to investigate issues. Signed-off-by: Jakub Jelen <[email protected]>
Jakuje
force-pushed
the
patch-1
branch
2 times, most recently
from
68dd501
to
a55aea1
Compare
|
@webknjaz the current version should be working version with tests of what, I hope, you were trying to describe. I think it will need some polishing so I would be happy for your inputs. |
Signed-off-by: Jakub Jelen <[email protected]>
Signed-off-by: Jakub Jelen <[email protected]>
|
SUMMARY
The libssh provides the most verbose logging with SSH_LOG_TRACE, which was mapped to
logging.CRITICAL, causing the users being unable to get full debug logs. These are critical for libssh developers to be able to investigate issues.ISSUE TYPE