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

Pawel plesniak/logger inheritance #322

Draft
wants to merge 25 commits into
base: develop
Choose a base branch
from

Conversation

PawelPlesniak
Copy link
Contributor

@PawelPlesniak PawelPlesniak commented Dec 6, 2024

Upgrading the logging inheritance, in preparation for the ERS upgrade.

Requirements

  1. Need a hierarchical structure for the loggers to correctly inherit handlers. Define the psuedo-root controller drunc, with associated parent loggers drunc.process_manager etc. (pseudo as the true root logger is created by logging.getlogger(), but in the scope of drunc this will be the new root logger)
  2. Need to use keep the logger output for the process_manager and the controllers, but not any of the shells.
  3. Handle all of the exception handling through loggers, remove print_traceback.
  4. Remove the controller RichHandler as it is not needed
  5. Need to move away from default logging.getLogger as this can cause problems if the logging root logger is initialized
  6. Need to define a scope for choosing which handlers are used on a per-logger basis, providing the basic platform for extending this with ers
  7. Leaving console.print in place as data that we are not interested in keeping in the log files, i.e. for the shell only.

Summary of changes made

  1. Defined a function setup_root_logger that creates the drunc logger
  2. Defined a function get_logger that changes the logging name to drunc.<log_name>. This will treat drunc as the pseudo-root logger. This will easily be extended for ers when it is ready.
  3. Defined a logging hierarchy such that
    • drunc is the root logger
    • There are separate parent loggers for the process_manager, controller, and utils with the appropriate handlers
    • When new loggers are redefined, theyw ill not override the handlers
    • Child loggers will either have a RichHandler or a StreamHandler
  4. logging.basicConfig default loggers removed, this will force us to be stricter with the choice of handlers. This addresses the issue of no controller RichHandlers. console printing has been left as this is information that will remain in the termina output at all times without information being commited to any logs.
  5. Changed all instances of self.logger and self._log to self.log
  6. Added .nfs* and info.*.json to .gitignore. We don't want these files in the repo anyway
  7. Created a root logger with level logging.ERROR when spawning the unified_shell to ensure that there is always a drunc logger before any logging is set. The level is overridden in setup_root_logger if the level is logging.ERROR

Notes

  1. There is a process_manager logger instantiated with the unified_shell, this is needed as the process_manager_driver does not see the logger created by the process-manager itself (I suspect this is because it is run in a separate mp.process and does not register with the primary process)

Still requires

Validating the parent logger selection, checking if more are needed. Still reviewing the FSM sepcific ones.
Add additional parent
Batch unit test

Discussion

We can define exception handling to go through the loggers, as per here, do we want this? It would mean that we can see the errors in both the console and the logs every time. The logs displayed in the console with RichHandler are rich formatted and the ones in the logs are regularly formatted.

There is now markup in the logs, e.g. in the process_manager log with drunc.process_manager.driver: Booting session [green]local-2x3-config[/green]. The relevant log entries are color coded so they are easier to read with the RichHandler, should this be kept or reverted? It's not clear how to do this to affect only one of the handlers.

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.

1 participant