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

Fix file logging level issue #289

Merged
merged 2 commits into from
Apr 19, 2024

Conversation

tpapaioa
Copy link
Collaborator

@tpapaioa tpapaioa commented Apr 17, 2024

Fixes #223

  1. This PR fixes the issue, so that with the following log levels set in ~/.broker/broker_settings.yaml:
logging:
  console_level: info
  file_level: trace

The CLI logs only info-level messages to the console, but logs trace-level messages to broker.log:

$ broker execute --workflow list-templates --artifacts last
[INFO 240417 11:02:34] Using provider AnsibleTower for execution
[INFO 240417 11:02:34] Using token authentication
[INFO 240417 11:02:35] No inventory specified, Ansible Tower will use a default.
[INFO 240417 11:02:36] Waiting for job: 
[...]

$ cat ~/.broker/logs/broker.log
[...]
[D 240417 11:02:34 __init__:75] Registered help option inventory for provider inventory
[T 240417 11:02:34 commands:26] Calling func=<function execute at 0x7f1785d48180>(*args=() **kwargs={'workflow': 'list-templates', 'artifacts': 'last', 'background': False, 'nick': None, 'output_format': 'log', 'args_file': None, 'job_tem
plate': None, 'tower_inventory': None}
[D 240417 11:02:34 broker:94] Broker instantiated with kwargs={'workflow': 'list-templates', 'artifacts': 'last'}
[I 240417 11:02:34 broker:191] Using provider AnsibleTower for execution
[...]
[T 240417 11:03:47 commands:28] Finished func=<function execute at 0x7f1785d48180>(*args=() **kwargs={'workflow': 'list-templates', 'artifacts': 'last', 'background': False, 'nick': None, 'output_format': 'log', 'args_file': None, 'job_template': None, 'tower_inventory': None}) retval=None
  1. Also updated the logic so that the file level is never set higher than the console level, i.e., the log file will always log what is logged to the console, if not more.

Example behavior:

  • Console level set to info and file level set to debug:
$ vim ~/.broker/broker_settings.yaml 
[...]
logging:
  console_level: info
  file_level: debug
[...]

The file level is lower than the console level, and this works as expected now, fixing issue #223

  • Console level set to debug and file level set to info:
logging:
  console_level: debug
  file_level: info

In this case, the file level is automatically adjusted to match the console level, which is lower. Info and debug messages are logged to both.

  1. Also added a fix to setup_logzero(), so that it calls patch_awx_for_verbosity() to patch the ansible api client and allow trace-level logging, no matter what the passed in value for level is. Before this fix, trace-level logging from the ansible client wasn't happening in the following cases:
  • Trace logging is set for the log file but not for console:
console_level: info
file_level: trace
  • Trace logging is enabled from the command line instead of from the settings file:
$ broker --log-level trace execute [...]

Copy link
Member

@ogajduse ogajduse left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix, Tasos.

@ogajduse ogajduse linked an issue Apr 17, 2024 that may be closed by this pull request
@JacobCallahan
Copy link
Member

What do you think of taking this a little bit of a different direction?
We could modify the
set_log_level function to check if the file log level (may need to add module-level tracking vars) would be greater than the new log level, then call set_file_logging with the new level. This way a user can ensure that their file logs are never a higher level than what they specify in the cli.

@tpapaioa
Copy link
Collaborator Author

What do you think of taking this a little bit of a different direction? We could modify the set_log_level function to check if the file log level (may need to add module-level tracking vars) would be greater than the new log level, then call set_file_logging with the new level. This way a user can ensure that their file logs are never a higher level than what they specify in the cli.

Updated to set file level to match console level if the console level is lower.

@tpapaioa
Copy link
Collaborator Author

Added one more fix related to trace-level logging.

@ogajduse
Copy link
Member

Added one more fix related to trace-level logging.

Neat! But now the docstring of patch_awx_for_verbosity is not correct since it will now run always.

def patch_awx_for_verbosity(api):
    """Patch the awxkit API to log when we're at trace level."""

Can you please update it?

@tpapaioa
Copy link
Collaborator Author

Added one more fix related to trace-level logging.

Neat! But now the docstring of patch_awx_for_verbosity is not correct since it will now run always.

def patch_awx_for_verbosity(api):
    """Patch the awxkit API to log when we're at trace level."""

Can you please update it?

Updated the docstring to hopefully be more accurate.

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.

ACK, thanks for the changes!

@JacobCallahan JacobCallahan merged commit a824052 into SatelliteQE:0.5 Apr 19, 2024
4 checks passed
@tpapaioa tpapaioa deleted the fix_logging_level branch April 19, 2024 20:02
JacobCallahan pushed a commit that referenced this pull request May 29, 2024
* Fix file logging level issue.

Fixes #223

* Patch awx logging to allow trace logging in all cases.
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.

settings.logging.file_level is not honored
3 participants