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 logger setup #4657

Merged
merged 21 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .changelog/4657.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
changes:
- description: Fixing logger crashes when imported as library.
type: fix
pr_number: 4657
5 changes: 3 additions & 2 deletions demisto_sdk/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from demisto_sdk.commands.common.logger import logging_setup
if __name__ in ["__main__", "demisto_sdk"]:
from demisto_sdk.commands.common.logger import logging_setup

logging_setup(initial=True, calling_function="__init__")
logging_setup(initial=True, calling_function="__init__")
67 changes: 49 additions & 18 deletions demisto_sdk/commands/common/logger.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import logging # noqa: TID251 # Required for propagation handling.
import os
import platform
import sys
Expand Down Expand Up @@ -30,11 +31,17 @@
DEFAULT_FILE_SIZE = 1 * (1024**2) # 1 MB
DEFAULT_FILE_COUNT = 10

global logger
LOGURU_DIAGNOSE = "LOGURU_DIAGNOSE"

logger = loguru.logger # all SDK modules should import from this file, not from loguru
logger.disable(None) # enabled at setup_logging()


class PropagateHandler(logging.Handler):
def emit(self, record: logging.LogRecord) -> None:
logging.getLogger(record.name).handle(record)


def _setup_neo4j_logger():
import logging # noqa: TID251 # special case, to control the neo4j logging

Expand Down Expand Up @@ -93,39 +100,63 @@ def logging_setup(
file_threshold: str = DEFAULT_FILE_THRESHOLD,
path: Optional[Union[Path, str]] = None,
initial: bool = False,
propagate: bool = False,
):
"""
The initial set up is required since we have code (e.g. get_content_path) that runs in __main__ before the typer/click commands set up the logger.
In the initial set up there is NO file logging (only console)
In the initial set up there is NO file logging (only console).

Parameters:
- calling_function (str): The name of the function invoking the logger setup, included in all logs.
- console_threshold (str): Log level for console output.
- file_threshold (str): Log level for file output.
- path (Union[Path, str], optional): Path for file logs. If None, defaults to the calculated log directory.
- initial (bool): Indicates if the setup is for the initial configuration (console-only if True).
- propagate (bool): If True, propagates logs to Python's logging system.

"""
global logger
_setup_neo4j_logger()

logger.remove(None) # Removes all pre-existing handlers
logger.remove() # Removes all pre-existing handlers
Copy link
Contributor

Choose a reason for hiding this comment

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

you sure passing None vs nothing still achieves what we wanted?

Copy link
Contributor Author

@barryyosi-panw barryyosi-panw Nov 12, 2024

Choose a reason for hiding this comment

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

Default value is None then yes, and it is less confusing that way imo. https://loguru.readthedocs.io/en/stable/api/logger.html#loguru._logger.Logger.remove


diagnose = string_to_bool(os.getenv("LOGURU_DIAGNOSE", False))
colorize = not string_to_bool(os.getenv(DEMISTO_SDK_LOG_NO_COLORS), False)
diagnose = string_to_bool(os.getenv(LOGURU_DIAGNOSE, "False"))
colorize = not string_to_bool(os.getenv(DEMISTO_SDK_LOG_NO_COLORS, "False"))

logger = logger.opt(
colors=colorize
) # allows using color tags in logs (e.g. logger.info("<blue>foo</blue>"))
_add_console_logger(
colorize=colorize, threshold=console_threshold, diagnose=diagnose
)

if not initial:
_add_file_logger(
log_path=calculate_log_dir(path) / LOG_FILE_NAME,
threshold=file_threshold,
diagnose=diagnose,
if propagate:
_propagate_logger(console_threshold)
else:
logger = logger.opt(
colors=colorize
) # allows using color tags in logs (e.g. logger.info("<blue>foo</blue>"))
_add_console_logger(
colorize=colorize, threshold=console_threshold, diagnose=diagnose
)
if not initial:
_add_file_logger(
log_path=calculate_log_dir(path) / LOG_FILE_NAME,
threshold=file_threshold,
diagnose=diagnose,
)
os.environ[DEMISTO_SDK_LOGGING_SET] = "true"

logger.debug(
f"logger setup: {calling_function=},{console_threshold=},{file_threshold=},{path=},{initial=}"
)


def _propagate_logger(
threshold: Optional[str],
):
"""
Adds a PropagateHandler to Loguru's logger to forward logs to Python's logging system.
"""
logger.add(
PropagateHandler(),
format=CONSOLE_FORMAT,
level=(threshold or DEFAULT_CONSOLE_THRESHOLD),
)


def _add_file_logger(log_path: Path, threshold: Optional[str], diagnose: bool):
logger.add(
log_path,
Expand Down