Repository navigation
Conversation
get_logger attached a NullHandler, which counts as a handler and stops Python from sending WARNING and above to logging.lastResort. Drop it so an application that has not configured logging still sees RedisVL warnings on stderr. Fixes redis#786
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit f9584bd. Configure here.
| if not logger.handlers: | ||
| logger.addHandler(logging.NullHandler()) | ||
| return logger | ||
| return logging.getLogger(name) |
There was a problem hiding this comment.
CLI failures dump traceback to stderr
Medium Severity
Removing the NullHandler from get_logger sends unconfigured records to logging.lastResort. The rvl CLI never configures logging and already prints failures to stderr, so logger.error(..., exc_info=True) now also emits a traceback and a duplicate error line on every command failure.
Reviewed by Cursor Bugbot for commit f9584bd. Configure here.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Motivation
RedisVL warnings never reach an application that has not configured logging, including warnings that signal lost data (#786).
get_loggerattached alogging.NullHandlerwhenever a logger had no handlers. A WARNING record with no handler in the hierarchy normally goes tologging.lastResort(stderr), but aNullHandlercounts as a handler, so the record was dropped.Changes
get_loggerno longer attaches a handler; it returnslogging.getLogger(name). Warnings and errors now reach stderr when logging is unconfigured. Applications that configure logging are unaffected: RedisVL loggers still propagate, and nothing callsbasicConfigor replaces handlers.tests/unit/test_log.pychecks this in a fresh interpreter and reads stderr, sincecaploginstalls its own handler and would pass even when theNullHandlerswallows the record. Both new tests fail with theNullHandlerrestored and pass with the fix.Notes
Existing handlers, including a
NullHandlerattached earlier in the same process, are left in place. INFO and DEBUG stay quiet when logging is unconfigured, matching standard-library behavior.Release Notes
RedisVL warnings and errors now show on stderr when the application has not configured logging.
Fixes #786
Note
Low Risk
Small logging-helper change with subprocess tests; configured apps still propagate normally, with only unconfigured apps gaining stderr visibility for WARNING+.
Overview
Fixes silent RedisVL warnings when the host app has not set up logging (e.g. data-loss warnings in #786).
get_loggerstops attaching aNullHandler. That handler counted as “handled,” so WARNING/ERROR never reached Python’slogging.lastResorton stderr. The function now only returnslogging.getLogger(name)with an updated docstring explaining the behavior.New unit tests in
test_log.pyspawn a fresh interpreter and assert on stderr/stdout, because pytestcaplogwould mask the old bug. They cover: no handlers on the returned logger, unconfigured apps seeing warnings/errors on stderr (matching a plain logger), and configuredbasicConfigstill receiving RedisVL warnings on the app stream.Reviewed by Cursor Bugbot for commit f9584bd. Bugbot is set up for automated code reviews on this repo. Configure here.