-
Notifications
You must be signed in to change notification settings - Fork 61
Description
Issue
The redisvl/utils/log.py module configures logging by adding a root logger, which can interfere with other loggers in an application. For example, importing redisvl can cause app logs to appear twice.
A previous PR (#301) added a guard to avoid adding a root logger if handlers exist, but in my case I don’t want any logs from redisvl in production.
The Python documentation strongly advises that libraries don't log to the root logger or add any handlers other than NullHandler (https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library):
It is strongly advised that you do not log to the root logger in your library. Instead, use a logger with a unique and easily identifiable name, such as the name for your library’s top-level package or module. Logging to the root logger will make it difficult or impossible for the application developer to configure the logging verbosity or handlers of your library as they wish.
It is strongly advised that you do not add any handlers other than NullHandler to your library’s loggers. This is because the configuration of handlers is the prerogative of the application developer who uses your library. The application developer knows their target audience and what handlers are most appropriate for their application: if you add handlers ‘under the hood’, you might well interfere with their ability to carry out unit tests and deliver logs which suit their requirements.
This is the code that's adding a root logger:
redis-vl-python/redisvl/utils/log.py
Lines 13 to 21 in 32698e9
| # Only configure this specific logger, not the root logger | |
| # Check if the logger already has handlers to respect existing configuration | |
| if not logger.handlers: | |
| logging.basicConfig( | |
| level=logging.INFO, | |
| format="%(asctime)s %(name)s %(levelname)s %(message)s", | |
| datefmt="%H:%M:%S", | |
| stream=sys.stdout, | |
| ) |
Reproducing the issue:
import logging
app_logger = logging.getLogger("my_app")
app_logger.setLevel(logging.INFO)
app_logger.addHandler(logging.StreamHandler())
print("==== App logging works as expected ====")
app_logger.info("This is an INFO message from the app logger.")
print()
from redisvl.query.filter import Tag
print("==== Now app logging is affected ====")
app_logger.info("This message appears TWICE!")Output:
==== App logging works as expected ====
This is an INFO message from the app logger.
==== Now app logging is affected ====
This message appears TWICE!
15:59:44 my_app INFO This message appears TWICE!
Proposed changes
Follow the recommendations from the Python documentation:
- Don't attach a root logger
- Only attach a NullHandler to
redisvlloggers
If this isn't possible for some reason, please let me know how I can work around this.