From ef66ac574e8b41ef16ba74e769ca81c840dc6b8c Mon Sep 17 00:00:00 2001
From: ohmayr <omairn@google.com>
Date: Mon, 25 Nov 2024 21:54:41 +0000
Subject: [PATCH] address PR feedback

---
 google/api_core/client_logging.py | 29 +++++++++-------
 tests/unit/test_client_logging.py | 58 ++++++++++++++++++++++++++-----
 2 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/google/api_core/client_logging.py b/google/api_core/client_logging.py
index ba445916..a2485e76 100644
--- a/google/api_core/client_logging.py
+++ b/google/api_core/client_logging.py
@@ -4,16 +4,16 @@
 import os
 
 _LOGGING_INITIALIZED = False
-
-# TODO(<add-link>): Update Request / Response messages.
+_BASE_LOGGER_NAME = "google"
+# TODO(https://github.com/googleapis/python-api-core/issues/760): Update Request / Response messages.
 REQUEST_MESSAGE = "Sending request ..."
 RESPONSE_MESSAGE = "Receiving response ..."
 
-# TODO(<add-link>): Update this list to support additional logging fields
+# TODO(https://github.com/googleapis/python-api-core/issues/761): Update this list to support additional logging fields
 _recognized_logging_fields = ["httpRequest", "rpcName", "serviceName"] # Additional fields to be Logged.
 
 def logger_configured(logger):
-  return logger.hasHandlers() or logger.level != logging.NOTSET or logger.propagate == False
+  return logger.handlers != [] or logger.level != logging.NOTSET or logger.propagate == False
 
 def initialize_logging():
    global _LOGGING_INITIALIZED
@@ -26,8 +26,9 @@ def initialize_logging():
 def parse_logging_scopes(scopes):
   if not scopes:
      return []
-  # TODO(<add-link>): check if the namespace is a valid namespace.
-  # TODO(<add-link>): parse a list of namespaces. Current flow expects a single string for now.
+  # TODO(https://github.com/googleapis/python-api-core/issues/759): check if the namespace is a valid namespace.
+  # TODO(b/380481951): Support logging multiple scopes.
+  # TODO(b/380483756): Raise or log a warning for an invalid scope.
   namespaces = [scopes]
   return namespaces
 
@@ -40,23 +41,25 @@ def configure_defaults(logger):
         console_handler.setFormatter(formatter)
         logger.addHandler(console_handler)
 
-def setup_logging(scopes):
-    # disable log propagation at base logger level to the root logger only if a base logger is not already configured via code changes.
-    base_logger = logging.getLogger("google")
-    if not logger_configured(base_logger):
-        base_logger.propagate = False
+def setup_logging(scopes=[]):
     
     # only returns valid logger scopes (namespaces)
     # this list has at most one element.
-    loggers = parse_logging_scopes(scopes) 
+    logger_names = parse_logging_scopes(scopes) 
 
-    for namespace in loggers:
+    for namespace in logger_names:
       # This will either create a module level logger or get the reference of the base logger instantiated above.
       logger = logging.getLogger(namespace)
 
       # Configure default settings.
       configure_defaults(logger)
 
+    # disable log propagation at base logger level to the root logger only if a base logger is not already configured via code changes.
+    # Maybe we do this at the end?
+    base_logger = logging.getLogger(_BASE_LOGGER_NAME)
+    if not logger_configured(base_logger):
+        base_logger.propagate = False
+
 class StructuredLogFormatter(logging.Formatter):
     def format(self, record):
         log_obj = {
diff --git a/tests/unit/test_client_logging.py b/tests/unit/test_client_logging.py
index 216dae7f..1654b122 100644
--- a/tests/unit/test_client_logging.py
+++ b/tests/unit/test_client_logging.py
@@ -1,20 +1,62 @@
 import logging
 import pytest
 
+from google.api_core.client_logging import setup_logging
 # Test expected behaviour for warnings, propagation, handler + formatter.
 
+def reset_logger(scope):
+    logger = logging.getLogger(scope)
+    logger.handlers = []
+    logger.setLevel(logging.NOTSET)
+    logger.propagate = True
+    
 def test_setup_logging_w_no_scopes():
-    # TODO(in-progress): 
-    pass
+    setup_logging()
+    base_logger = logging.getLogger("google")
+    assert base_logger.handlers == []
+    assert base_logger.propagate == False
+    assert base_logger.level == logging.NOTSET
+
+    reset_logger("google")
+
 
 def test_setup_logging_w_base_scope():
-    # TODO(in-progress): 
-    pass
+    setup_logging("google")
+    base_logger = logging.getLogger("google")
+    assert isinstance(base_logger.handlers[0], logging.StreamHandler)
+    assert base_logger.propagate == False
+    assert base_logger.level == logging.DEBUG
+
+    reset_logger("google")
 
 def test_setup_logging_w_module_scope():
-    # TODO(in-progress): 
-    pass
+    setup_logging("google.foo")
+    
+    base_logger = logging.getLogger("google")
+    assert base_logger.handlers == []
+    assert base_logger.propagate == False
+    assert base_logger.level == logging.NOTSET
+
+    module_logger = logging.getLogger("google.foo")
+    assert isinstance(module_logger.handlers[0], logging.StreamHandler)
+    assert module_logger.propagate == False
+    assert module_logger.level == logging.DEBUG
+
+
+    reset_logger("google")
 
 def test_setup_logging_w_incorrect_scope():
-    # TODO(in-progress): 
-    pass
+    setup_logging("foo")
+    
+    base_logger = logging.getLogger("google")
+    assert base_logger.handlers == []
+    assert base_logger.propagate == False
+    assert base_logger.level == logging.NOTSET
+
+    # TODO: update test once we add logic to ignore an incorrect scope.
+    logger = logging.getLogger("foo")
+    assert isinstance(logger.handlers[0], logging.StreamHandler)
+    assert logger.propagate == False
+    assert logger.level == logging.DEBUG
+
+    reset_logger("google")