-
Notifications
You must be signed in to change notification settings - Fork 42
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
refactor logging #209
base: master
Are you sure you want to change the base?
refactor logging #209
Conversation
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.
Some major questions along with a need for screenshots of debug-level logging and potentially an exception being raised. We will still need colorized logging levels.
from broker.broker import Broker | ||
#from broker.broker import Broker | ||
|
||
VMBroker = Broker | ||
#VMBroker = Broker |
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.
Why comment these out?
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.
Good question.
I have checked out SatelliteQE/robottelo#1332 and installed broker from this PR/branch and I reverted the patch in this __init__.py
file. I put breakpoint into both logger setup functions in broker and here is the call stack.
We can see that broker's logger gets initialized during the import time. That is not ideal, we want to initialize broker logger once setup_logger
is called, not when it is imported.
from broker.logger import setup_logger as broker_log_setup
import imports from broker.broker import Broker
which imports from broker.hosts import Host
which calls logger = init_logger(__name__)
that initializes the logger.
Do you have any idea what could be a different approach to avoid setting the logger up during the import time?
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.
Let's reframe this a bit.
Why try to avoid the initial logger setup?
What is gained from finding a way around it?
What is lost from user experience by implementing workarounds?
It's not always easy to do this in a way that satisfies what @rplevka wants here while maintaining a nice API and reducing waste. One potential option is for Broker to use its own logging wrapper that lazily sets up the logging interface.
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.
Hi.
My primary motivation was to redesign broker logging in a way, so it can fully give away control over its setup to library that uses it (robottelo).
I was looking to setup different handlers (that would log to their own files) for different pytest objects - especially fixtures.
In pytest, it is possible to control this from the appropriate hook functions, however, these are being entered quite late in the process (by that time, the broker logger is self-initialized by mere module import - thus, it generates unwanted loggers and log files that are outside of robottelo control.
I don't have a strong opinion about how this is supposed to work, so I am opened to some ideas, but this design is something I came to while trying to solve a problem of a proper structured logging (to know what logs belong to what part of the session) for purposes of log parsing and pushing towards reporting platforms.
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.
Due to import sequences, this isn't something that we can do easily in Python. In my opinion, it's not a big enough concern to justify changes like this. If Broker creates a log file and potentially a few logs before Robottelo redirects its logger, that's not a big deal. Please revert changes like the ones here.
path="logs/broker.log", | ||
path=None, |
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.
Why default to none instead of Broker's default log path?
Replaces deprecated
logzero
by a ntaivelogging
libinroduces logger hierarchy per module (handler, formatter inheritance, ...)
This complements the changes introduced in robottelo by SatelliteQE/robottelo#11332
TBD:
__main__
)awxkit
,...) to the hierarchy