Skip to content
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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

rplevka
Copy link
Member

@rplevka rplevka commented Apr 25, 2023

Replaces deprecated logzero by a ntaive logging lib
inroduces logger hierarchy per module (handler, formatter inheritance, ...)

This complements the changes introduced in robottelo by SatelliteQE/robottelo#11332

TBD:

  • figure out initialization when broker is run from CLI (__main__)
  • figure out a way of adding broker's external lib loggers (awxkit,...) to the hierarchy

@rplevka rplevka marked this pull request as draft April 25, 2023 07:42
Copy link
Member

@JacobCallahan JacobCallahan left a 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.

Comment on lines -1 to +3
from broker.broker import Broker
#from broker.broker import Broker

VMBroker = Broker
#VMBroker = Broker
Copy link
Member

Choose a reason for hiding this comment

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

Why comment these out?

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

@rplevka rplevka May 4, 2023

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.

Copy link
Member

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.

Comment on lines -138 to +137
path="logs/broker.log",
path=None,
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants