fix: Additional logging adjustments

Author: abeiderman

README.md

@@ -163,17 +163,21 @@ import logging
 logging.getLogger("configdirector").setLevel(logging.DEBUG)
 ```
 
-With no logging configured at all, Python still surfaces warnings and errors on `stderr`, so an
-invalid SDK key never passes silently.
+The SDK sets no level of its own, so this logger follows the usual `logging` rules. With nothing
+configured at all, Python still surfaces warnings and errors on `stderr`, so an invalid SDK key
+never passes silently.
 
-Pass any object implementing `ConfigDirectorLogger` to override it — including a different
-stdlib logger, or the SDK's own console logger if you would rather not configure `logging`:
+If you would rather not configure `logging`, `log_level` sets a level on that logger for you:
 
 ```python
-from configdirector import create_console_logger
+client = ConfigDirectorClient("YOUR-SERVER-SDK-KEY", log_level="DEBUG")
+```
+
+Pass any object implementing `ConfigDirectorLogger` to override the logger entirely — a
+different stdlib logger, or your own adapter:
 
+```python
 client = ConfigDirectorClient("YOUR-SERVER-SDK-KEY", logger=logging.getLogger("my_app.flags"))
-client = ConfigDirectorClient("YOUR-SERVER-SDK-KEY", logger=create_console_logger("debug"))
 ```
 
 ### Shutdown

samples/flask/README.md

@@ -96,11 +96,11 @@ client = ConfigDirectorClient(..., logger=sdk_logger)
 flask_sample.configdirector DEBUG No config state found for 'integer-config', returning default value 10
 ```
 
-Any object with `debug`/`info`/`warning`/`error` methods works — a stdlib `Logger` satisfies
-that, and so does `create_console_logger("debug")` if you would rather not configure the
-`logging` module at all. Omit `logger=` entirely and the SDK falls back to the standard library
-logger named `configdirector`. Set `CONFIGDIRECTOR_LOG_LEVEL=DEBUG` to watch every evaluation as
-it happens.
+Any object with `debug`/`info`/`warning`/`error` methods works, and a stdlib `Logger` satisfies
+that. Omit `logger=` entirely and the SDK falls back to the standard library logger named
+`configdirector`, leaving the level to your application — or pass `log_level=` if you would
+rather not configure the `logging` module at all. Set `CONFIGDIRECTOR_LOG_LEVEL=DEBUG` to watch
+every evaluation as it happens.
 
 **Shutdown is clean.** `atexit` closes the client, dropping connections and flushing pending
 telemetry. A production deployment would also hook its server's worker-exit signal.

samples/flask/configdirector_client.py

@@ -50,9 +50,9 @@
 # named "configdirector"; passing one in puts its output under this application's own logging
 # namespace instead, where existing handlers, filters, and level config already apply.
 #
-# Any object with debug/info/warning/error methods works — a stdlib Logger satisfies that, and
-# so does `configdirector.create_console_logger("debug")` if you would rather not configure the
-# logging module at all.
+# Any object with debug/info/warning/error methods works, and a stdlib Logger satisfies that.
+# Passing `log_level=` instead of `logger=` is the shortcut when you would rather not configure
+# the logging module at all.
 sdk_logger = logging.getLogger("flask_sample.configdirector")
 sdk_logger.setLevel(os.environ.get("CONFIGDIRECTOR_LOG_LEVEL", "INFO"))
 

src/configdirector/_eventsource/client.py

@@ -160,9 +160,17 @@ def _run(self) -> None:
                 self._set_state(ReadyState.CONNECTING)
                 if self._stop.wait(self._reconnect_delay(state)):
                     return
-        except BaseException as error:  # the worker must not die without saying why
-            self._logger.error("[EventSource] The connection loop stopped unexpectedly: %r", error)
+        except BaseException as error:
+            # The loop is over either way, and a state left at OPEN would have callers believing
+            # there is still a reader on the stream.
             self._ready_state = ReadyState.CLOSED
+            # SystemExit and KeyboardInterrupt ask to unwind; they are not stream failures.
+            # Logging one as "the connection stopped" would describe a problem the caller never
+            # had, so they are left to travel as themselves.
+            if not isinstance(error, Exception):
+                raise
+            # Anything else: the worker must not die without saying why.
+            self._logger.error("[EventSource] The connection loop stopped unexpectedly: %r", error)
 
     def _connect_once(self) -> _Failure | None:
         try:

src/configdirector/_logger.py

@@ -1,13 +1,12 @@
 import logging
 
+__all__ = ["LOGGER_NAME", "get_default_logger"]
 
-def get_default_logger(log_level: int | str | None = None) -> logging.Logger:
-    """Return the standard library logger the SDK uses when none is supplied.
+LOGGER_NAME = __package__ or "configdirector"
 
-    Configure it like any other logger::
 
-        logging.getLogger("configdirector").setLevel(logging.DEBUG)
-    """
-    logger = logging.getLogger(__name__)
-    logger.setLevel(logging.WARNING if log_level is None else log_level)
+def get_default_logger(log_level: int | str | None = None) -> logging.Logger:
+    logger = logging.getLogger(LOGGER_NAME)
+    if log_level is not None:
+        logger.setLevel(log_level)
     return logger

src/configdirector/client.py

@@ -78,8 +78,13 @@ class ConfigDirectorClient:
             recommended so that they can be referenced from targeting rules.
         connection: Connection options such as mode, timeout, and polling interval.
         logger: Any object implementing :class:`~configdirector.types.ConfigDirectorLogger`,
-            including a standard library :class:`logging.Logger`. Defaults to the logger named
-            ``"configdirector"``.
+            including a standard library :class:`logging.Logger`. Defaults to the standard
+            library logger named ``"configdirector"``, which your application configures like
+            any other logger.
+        log_level: A level to set on that default logger, as either a :mod:`logging` constant
+            or its name. A convenience for applications that do not otherwise configure
+            logging; when omitted, the SDK sets no level and the usual ``logging`` rules apply.
+            Ignored when ``logger`` is supplied.
         telemetry: Telemetry queue and flush tuning.
         hooks: Event handlers to attach before the client can emit any event.
 
@@ -633,6 +638,7 @@ def create_client(
     metadata: Metadata | None = None,
     connection: ConnectionOptions | None = None,
     logger: ConfigDirectorLogger | None = None,
+    log_level: int | str | None = None,
     telemetry: TelemetryOptions | None = None,
     hooks: ClientHooks | None = None,
 ) -> ConfigDirectorClient:
@@ -646,6 +652,7 @@ def create_client(
         metadata=metadata,
         connection=connection,
         logger=logger,
+        log_level=log_level,
         telemetry=telemetry,
         hooks=hooks,
     )

tests/eventsource/test_client.py

@@ -17,7 +17,7 @@
 )
 
 from .helpers import FailingTransport, FakeResponse, FakeTransport, sse, wait_for
-from helpers import create_stubbed_logger
+from helpers import RecordingLogger, create_stubbed_logger
 
 URL = "http://localhost/sse"
 _R = TypeVar("_R")
@@ -382,6 +382,40 @@ def explode(message: EventSourceMessage) -> None:
         client.connect()
         assert wait_for(lambda: received == ["one", "two"])
 
+    # The escaping SystemExit is the point of the test: it reaches threading.excepthook as
+    # itself instead of being logged as a connection error, and pytest reports that as a warning.
+    @pytest.mark.filterwarnings("ignore::pytest.PytestUnhandledThreadExceptionWarning")
+    def test_system_exit_from_a_handler_is_not_reported_as_a_loop_failure(
+        self, clients: list[EventSourceClient]
+    ) -> None:
+        # SystemExit is a deliberate request to unwind, not a stream error. Logging it as one
+        # would leave a caller that raised it looking at an unexplained connection failure.
+        recording = RecordingLogger()
+        delivered = threading.Event()
+
+        def exit_now(message: EventSourceMessage) -> None:
+            delivered.set()
+            raise SystemExit(1)
+
+        client = build(
+            clients,
+            logger=recording,
+            transport=FakeTransport(FakeResponse(chunks=sse("data: one\n\n"))),
+            on_message=exit_now,
+            should_reconnect=lambda state: False,
+        )
+
+        client.connect()
+        assert delivered.wait(timeout=5)
+        worker = client._thread
+        assert worker is not None
+        worker.join(timeout=5)
+        assert worker.is_alive() is False
+
+        assert recording.messages("error") == []
+        # The loop still leaves the state honest on its way out.
+        assert client.ready_state is ReadyState.CLOSED
+
     def test_a_raising_should_reconnect_falls_back_to_reconnecting(
         self, clients: list[EventSourceClient]
     ) -> None:

tests/test_construction.py

@@ -1,5 +1,7 @@
 from __future__ import annotations
 
+import inspect
+
 import pytest
 
 from configdirector import (
@@ -32,6 +34,14 @@ def test_create_client_is_equivalent_to_the_constructor() -> None:
     assert client.is_ready is False
 
 
+def test_create_client_takes_every_constructor_option() -> None:
+    # create_client documents itself as equivalent to the constructor, so an option added to one
+    # and not the other is a defect rather than a design choice.
+    assert list(inspect.signature(create_client).parameters) == list(
+        inspect.signature(ConfigDirectorClient).parameters
+    )
+
+
 def test_accepts_every_option() -> None:
     client = ConfigDirectorClient(
         SDK_KEY,

tests/test_logger.py

@@ -0,0 +1,72 @@
+from __future__ import annotations
+
+import logging
+from collections.abc import Iterator
+
+import pytest
+
+from configdirector import ConfigDirectorClient, create_client
+from configdirector._logger import get_default_logger
+
+SDK_KEY = "test-server-sdk-key"
+
+# Spelled out rather than imported, because the name being exactly this is what is under test.
+LOGGER_NAME = "configdirector"
+
+
+@pytest.fixture(autouse=True)
+def restore_the_package_logger() -> Iterator[None]:
+    """Loggers are process-wide, so a test that changes one has to hand it back."""
+    logger = logging.getLogger(LOGGER_NAME)
+    level = logger.level
+    yield
+    logger.setLevel(level)
+
+
+def test_the_default_logger_is_named_for_the_package() -> None:
+    assert get_default_logger().name == "configdirector"
+
+
+def test_no_level_is_imposed_when_none_is_given() -> None:
+    assert get_default_logger().level == logging.NOTSET
+
+
+def test_the_application_keeps_control_of_the_level() -> None:
+    # The configuration the README and the client docstring both tell users to write.
+    logging.getLogger("configdirector").setLevel(logging.DEBUG)
+
+    assert get_default_logger().isEnabledFor(logging.DEBUG) is True
+
+
+def test_warnings_reach_an_application_that_configured_nothing() -> None:
+    # An unconfigured application inherits the root logger's WARNING, which logging.lastResort
+    # then carries to stderr. That is what keeps an invalid SDK key from failing silently.
+    assert get_default_logger().isEnabledFor(logging.WARNING) is True
+
+
+@pytest.mark.parametrize(
+    ("log_level", "expected"),
+    [(logging.DEBUG, logging.DEBUG), ("INFO", logging.INFO)],
+)
+def test_applies_an_explicit_log_level(log_level: int | str, expected: int) -> None:
+    assert get_default_logger(log_level).level == expected
+
+
+def test_the_client_logs_through_the_package_logger() -> None:
+    client = ConfigDirectorClient(SDK_KEY)
+
+    assert client._logger is logging.getLogger("configdirector")
+
+
+def test_the_client_applies_an_explicit_log_level() -> None:
+    client = ConfigDirectorClient(SDK_KEY, log_level="DEBUG")
+
+    assert isinstance(client._logger, logging.Logger)
+    assert client._logger.isEnabledFor(logging.DEBUG) is True
+
+
+def test_create_client_applies_an_explicit_log_level() -> None:
+    client = create_client(SDK_KEY, log_level="DEBUG")
+
+    assert isinstance(client._logger, logging.Logger)
+    assert client._logger.isEnabledFor(logging.DEBUG) is True