From b3993c58a19a0b48f6e2f674d7da6486ed47e67c Mon Sep 17 00:00:00 2001 From: kmontemayor Date: Fri, 20 Feb 2026 18:20:40 +0000 Subject: [PATCH 1/3] Switch Logger to GCP structured JSON logging via stderr Replace google.cloud.logging.Client().setup_logging() with a StreamHandler writing JSON to stderr, which GCP's logging agent automatically parses into structured LogEntry fields. This removes the network dependency at log time, prevents dropped logs on crash, and produces queryable structured logs. Co-Authored-By: Claude Opus 4.6 --- gigl/common/formatters/__init__.py | 3 + gigl/common/formatters/gcp_json_formatter.py | 92 ++++++++++++++++ gigl/common/logger.py | 34 +++--- tests/unit/common/formatters/__init__.py | 0 .../formatters/gcp_json_formatter_test.py | 102 ++++++++++++++++++ tests/unit/common/logger_test.py | 30 ++++++ 6 files changed, 248 insertions(+), 13 deletions(-) create mode 100644 gigl/common/formatters/__init__.py create mode 100644 gigl/common/formatters/gcp_json_formatter.py create mode 100644 tests/unit/common/formatters/__init__.py create mode 100644 tests/unit/common/formatters/gcp_json_formatter_test.py create mode 100644 tests/unit/common/logger_test.py diff --git a/gigl/common/formatters/__init__.py b/gigl/common/formatters/__init__.py new file mode 100644 index 000000000..3b5e7d919 --- /dev/null +++ b/gigl/common/formatters/__init__.py @@ -0,0 +1,3 @@ +from gigl.common.formatters.gcp_json_formatter import GcpJsonFormatter + +__all__ = ["GcpJsonFormatter"] diff --git a/gigl/common/formatters/gcp_json_formatter.py b/gigl/common/formatters/gcp_json_formatter.py new file mode 100644 index 000000000..5a523aa39 --- /dev/null +++ b/gigl/common/formatters/gcp_json_formatter.py @@ -0,0 +1,92 @@ +import json +import logging +from datetime import datetime, timezone + +_PYTHON_LEVEL_TO_GCP_SEVERITY: dict[str, str] = { + "DEBUG": "DEBUG", + "INFO": "INFO", + "WARNING": "WARNING", + "ERROR": "ERROR", + "CRITICAL": "CRITICAL", +} + +_STANDARD_LOG_RECORD_ATTRIBUTES: frozenset[str] = frozenset( + logging.LogRecord( + name="", + level=0, + pathname="", + lineno=0, + msg="", + args=None, + exc_info=None, + ).__dict__.keys() +) + + +class GcpJsonFormatter(logging.Formatter): + """A ``logging.Formatter`` that outputs one JSON object per line with + `GCP-recognized structured logging fields + `_. + + Fields emitted: + + - ``severity`` -- mapped from the Python log level. + - ``message`` -- the formatted log message (with traceback appended when present). + - ``time`` -- ISO 8601 UTC timestamp. + - ``logging.googleapis.com/sourceLocation`` -- ``{file, line, function}``. + - ``logging.googleapis.com/labels`` -- any extra fields supplied via the + ``extra`` dict on the ``Logger`` adapter. Omitted when there are no extras. + + Example: + >>> import logging, io + >>> handler = logging.StreamHandler(io.StringIO()) + >>> handler.setFormatter(GcpJsonFormatter()) + >>> log = logging.getLogger("demo") + >>> log.addHandler(handler) + >>> log.setLevel(logging.INFO) + >>> log.info("hello") + >>> import json; json.loads(handler.stream.getvalue())["message"] + 'hello' + """ + + def format(self, record: logging.LogRecord) -> str: + """Format *record* as a single-line JSON string. + + Args: + record: The ``LogRecord`` to format. + + Returns: + A JSON string (no trailing newline) suitable for writing to + ``sys.stderr`` on GCP-managed environments. + """ + message = record.getMessage() + + if record.exc_info and not record.exc_text: + record.exc_text = self.formatException(record.exc_info) + if record.exc_text: + message = f"{message}\n{record.exc_text}" + if record.stack_info: + message = f"{message}\n{record.stack_info}" + + payload: dict[str, object] = { + "severity": _PYTHON_LEVEL_TO_GCP_SEVERITY.get( + record.levelname, record.levelname + ), + "message": message, + "time": datetime.fromtimestamp(record.created, tz=timezone.utc).isoformat(), + "logging.googleapis.com/sourceLocation": { + "file": record.pathname, + "line": record.lineno, + "function": record.funcName, + }, + } + + extra_fields = { + key: value + for key, value in record.__dict__.items() + if key not in _STANDARD_LOG_RECORD_ATTRIBUTES + } + if extra_fields: + payload["logging.googleapis.com/labels"] = extra_fields + + return json.dumps(payload, ensure_ascii=False, default=str) diff --git a/gigl/common/logger.py b/gigl/common/logger.py index 52102f3e9..378cb6bc7 100644 --- a/gigl/common/logger.py +++ b/gigl/common/logger.py @@ -1,22 +1,32 @@ import logging import os import pathlib +import sys from datetime import datetime from typing import Any, MutableMapping, Optional -from google.cloud import logging as google_cloud_logging +from gigl.common.formatters import GcpJsonFormatter _BASE_LOG_FILE_PATH = "/tmp/research/gbml/logs" -class Logger(logging.LoggerAdapter): +def _is_gcp_environment() -> bool: + """Return ``True`` when running on a GCP-managed platform (GKE, GAE, etc.). + + Checks the environment variables that GCP injects into container workloads. + Extracted as a standalone helper so tests can patch a single symbol. """ - GiGL's custom logger class used for local and cloud logging (VertexAI, Dataflow, etc.) + return bool(os.getenv("GAE_APPLICATION") or os.getenv("KUBERNETES_SERVICE_HOST")) + + +class Logger(logging.LoggerAdapter): + """GiGL's custom logger class used for local and cloud logging (VertexAI, Dataflow, etc.). + Args: - logger (Optional[logging.Logger]): A custom logger to use. If not provided, the default logger will be created. - name (Optional[str]): The name to be used for the logger. By default uses "root". - log_to_file (bool): If True, logs will be written to a file. If False, logs will be written to the console. - extra (Optional[dict[str, Any]]): Extra information to be added to the log message. + logger: A custom logger to use. If not provided, the default logger will be created. + name: The name to be used for the logger. By default uses "root". + log_to_file: If True, logs will be written to a file. If False, logs will be written to the console. + extra: Extra information to be added to the log message. """ def __init__( @@ -37,12 +47,10 @@ def _setup_logger( ) -> None: handler: logging.Handler if not logger.handlers: - if os.getenv("GAE_APPLICATION") or os.environ.get( - "KUBERNETES_SERVICE_HOST" - ): - # Google Cloud Logging - client = google_cloud_logging.Client() - client.setup_logging(log_level=logging.INFO) + if _is_gcp_environment(): + handler = logging.StreamHandler(stream=sys.stderr) + handler.setFormatter(GcpJsonFormatter()) + logger.addHandler(handler) else: # Logging locally. Set up logging to console or file if log_to_file: diff --git a/tests/unit/common/formatters/__init__.py b/tests/unit/common/formatters/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/unit/common/formatters/gcp_json_formatter_test.py b/tests/unit/common/formatters/gcp_json_formatter_test.py new file mode 100644 index 000000000..08359872f --- /dev/null +++ b/tests/unit/common/formatters/gcp_json_formatter_test.py @@ -0,0 +1,102 @@ +import json +import logging + +from gigl.common.formatters.gcp_json_formatter import GcpJsonFormatter +from tests.test_assets.test_case import TestCase + + +class GcpJsonFormatterTest(TestCase): + def setUp(self) -> None: + self.formatter = GcpJsonFormatter() + + def _make_record( + self, + message: str = "test message", + level: int = logging.INFO, + exc_info: object = None, + ) -> logging.LogRecord: + """Create a ``LogRecord`` with deterministic source location.""" + record = logging.LogRecord( + name="test", + level=level, + pathname="test_file.py", + lineno=42, + msg=message, + args=None, + exc_info=exc_info, # type: ignore[arg-type] + ) + record.funcName = "test_func" + return record + + def test_basic_info_message_produces_valid_json(self) -> None: + record = self._make_record() + output = self.formatter.format(record) + parsed = json.loads(output) + + self.assertEqual(parsed["severity"], "INFO") + self.assertEqual(parsed["message"], "test message") + self.assertIn("time", parsed) + self.assertIn("logging.googleapis.com/sourceLocation", parsed) + + def test_severity_mapping_for_all_levels(self) -> None: + levels = { + logging.DEBUG: "DEBUG", + logging.INFO: "INFO", + logging.WARNING: "WARNING", + logging.ERROR: "ERROR", + logging.CRITICAL: "CRITICAL", + } + for python_level, expected_severity in levels.items(): + with self.subTest(level=python_level): + record = self._make_record(level=python_level) + parsed = json.loads(self.formatter.format(record)) + self.assertEqual(parsed["severity"], expected_severity) + + def test_output_is_single_line(self) -> None: + record = self._make_record() + output = self.formatter.format(record) + self.assertEqual(output.count("\n"), 0) + + def test_time_field_is_iso_8601(self) -> None: + record = self._make_record() + parsed = json.loads(self.formatter.format(record)) + time_str = parsed["time"] + # ISO 8601 with timezone: contains 'T' separator and '+' offset + self.assertIn("T", time_str) + self.assertIn("+", time_str) + + def test_extra_fields_appear_under_labels(self) -> None: + record = self._make_record() + record.custom_key = "custom_value" # type: ignore[attr-defined] + parsed = json.loads(self.formatter.format(record)) + + labels = parsed["logging.googleapis.com/labels"] + self.assertEqual(labels["custom_key"], "custom_value") + + def test_no_labels_key_when_no_extras(self) -> None: + record = self._make_record() + parsed = json.loads(self.formatter.format(record)) + self.assertNotIn("logging.googleapis.com/labels", parsed) + + def test_exception_traceback_in_message(self) -> None: + try: + raise ValueError("boom") + except ValueError: + import sys + + exc_info = sys.exc_info() + + record = self._make_record(exc_info=exc_info) + parsed = json.loads(self.formatter.format(record)) + + self.assertIn("ValueError: boom", parsed["message"]) + self.assertIn("Traceback", parsed["message"]) + + def test_source_location_fields(self) -> None: + record = self._make_record() + parsed = json.loads(self.formatter.format(record)) + source = parsed["logging.googleapis.com/sourceLocation"] + + self.assertEqual(source["file"], "test_file.py") + self.assertEqual(source["line"], 42) + self.assertEqual(source["function"], "test_func") diff --git a/tests/unit/common/logger_test.py b/tests/unit/common/logger_test.py new file mode 100644 index 000000000..69afd0910 --- /dev/null +++ b/tests/unit/common/logger_test.py @@ -0,0 +1,30 @@ +import logging +from unittest.mock import patch + +from gigl.common.formatters.gcp_json_formatter import GcpJsonFormatter +from gigl.common.logger import Logger +from tests.test_assets.test_case import TestCase + + +class LoggerModeSelectionTest(TestCase): + """Verify that ``Logger`` selects the correct handler/formatter based on environment.""" + + def tearDown(self) -> None: + # Clean up any loggers created during tests to avoid handler leaks. + logging.Logger.manager.loggerDict.clear() + logging.root.handlers.clear() + + @patch("gigl.common.logger._is_gcp_environment", return_value=True) + def test_gcp_mode_uses_gcp_json_formatter(self, _mock_is_gcp: object) -> None: + logger = Logger(name="test_gcp_mode") + handlers = logger.logger.handlers + self.assertTrue(len(handlers) > 0, "Expected at least one handler") + self.assertIsInstance(handlers[0].formatter, GcpJsonFormatter) + + @patch("gigl.common.logger._is_gcp_environment", return_value=False) + def test_local_mode_uses_standard_formatter(self, _mock_is_gcp: object) -> None: + logger = Logger(name="test_local_mode") + handlers = logger.logger.handlers + self.assertTrue(len(handlers) > 0, "Expected at least one handler") + self.assertNotIsInstance(handlers[0].formatter, GcpJsonFormatter) + self.assertIsInstance(handlers[0].formatter, logging.Formatter) From 60259fadebade4b690744b179290271e02175546 Mon Sep 17 00:00:00 2001 From: kmontemayor Date: Fri, 20 Feb 2026 18:20:40 +0000 Subject: [PATCH 2/3] Switch Logger to GCP structured JSON logging via stderr Replace google.cloud.logging.Client().setup_logging() with a StreamHandler writing JSON to stderr, which GCP's logging agent automatically parses into structured LogEntry fields. This removes the network dependency at log time, prevents dropped logs on crash, and produces queryable structured logs. Co-Authored-By: Claude Opus 4.6 --- gigl/common/formatters/__init__.py | 3 + gigl/common/formatters/gcp_json_formatter.py | 80 ++++++++++++++ gigl/common/logger.py | 40 ++++--- tests/unit/common/formatters/__init__.py | 0 .../formatters/gcp_json_formatter_test.py | 102 ++++++++++++++++++ tests/unit/common/logger_test.py | 30 ++++++ 6 files changed, 239 insertions(+), 16 deletions(-) create mode 100644 gigl/common/formatters/__init__.py create mode 100644 gigl/common/formatters/gcp_json_formatter.py create mode 100644 tests/unit/common/formatters/__init__.py create mode 100644 tests/unit/common/formatters/gcp_json_formatter_test.py create mode 100644 tests/unit/common/logger_test.py diff --git a/gigl/common/formatters/__init__.py b/gigl/common/formatters/__init__.py new file mode 100644 index 000000000..e34bea005 --- /dev/null +++ b/gigl/common/formatters/__init__.py @@ -0,0 +1,3 @@ +from gigl.common.formatters.gcp_json_formatter import GCP_LABELS_RECORD_ATTR, GcpJsonFormatter + +__all__ = ["GCP_LABELS_RECORD_ATTR", "GcpJsonFormatter"] diff --git a/gigl/common/formatters/gcp_json_formatter.py b/gigl/common/formatters/gcp_json_formatter.py new file mode 100644 index 000000000..aed152066 --- /dev/null +++ b/gigl/common/formatters/gcp_json_formatter.py @@ -0,0 +1,80 @@ +import json +import logging +from datetime import datetime, timezone + +_PYTHON_LEVEL_TO_GCP_SEVERITY: dict[str, str] = { + "DEBUG": "DEBUG", + "INFO": "INFO", + "WARNING": "WARNING", + "ERROR": "ERROR", + "CRITICAL": "CRITICAL", +} + +# Key used by Logger.process() to pass user-supplied extras to the formatter +# without mixing them into the LogRecord's built-in attributes. +GCP_LABELS_RECORD_ATTR: str = "_gcp_labels" + + +class GcpJsonFormatter(logging.Formatter): + """A ``logging.Formatter`` that outputs one JSON object per line with + `GCP-recognized structured logging fields + `_. + + Fields emitted: + + - ``severity`` -- mapped from the Python log level. + - ``message`` -- the formatted log message (with traceback appended when present). + - ``time`` -- ISO 8601 UTC timestamp. + - ``logging.googleapis.com/sourceLocation`` -- ``{file, line, function}``. + - ``logging.googleapis.com/labels`` -- any extra fields supplied via the + ``extra`` dict on the ``Logger`` adapter. Omitted when there are no extras. + + Example: + >>> import logging, io + >>> handler = logging.StreamHandler(io.StringIO()) + >>> handler.setFormatter(GcpJsonFormatter()) + >>> log = logging.getLogger("demo") + >>> log.addHandler(handler) + >>> log.setLevel(logging.INFO) + >>> log.info("hello") + >>> import json; json.loads(handler.stream.getvalue())["message"] + 'hello' + """ + + def format(self, record: logging.LogRecord) -> str: + """Format *record* as a single-line JSON string. + + Args: + record: The ``LogRecord`` to format. + + Returns: + A JSON string (no trailing newline) suitable for writing to + ``sys.stderr`` on GCP-managed environments. + """ + message = record.getMessage() + + if record.exc_info and not record.exc_text: + record.exc_text = self.formatException(record.exc_info) + if record.exc_text: + message = f"{message}\n{record.exc_text}" + if record.stack_info: + message = f"{message}\n{record.stack_info}" + + payload: dict[str, object] = { + "severity": _PYTHON_LEVEL_TO_GCP_SEVERITY.get( + record.levelname, record.levelname + ), + "message": message, + "time": datetime.fromtimestamp(record.created, tz=timezone.utc).isoformat(), + "logging.googleapis.com/sourceLocation": { + "file": record.pathname, + "line": record.lineno, + "function": record.funcName, + }, + } + + labels: dict[str, object] = getattr(record, GCP_LABELS_RECORD_ATTR, {}) + if labels: + payload["logging.googleapis.com/labels"] = labels + + return json.dumps(payload, ensure_ascii=False, default=str) diff --git a/gigl/common/logger.py b/gigl/common/logger.py index 52102f3e9..ebd3c01c1 100644 --- a/gigl/common/logger.py +++ b/gigl/common/logger.py @@ -1,22 +1,32 @@ import logging import os import pathlib +import sys from datetime import datetime from typing import Any, MutableMapping, Optional -from google.cloud import logging as google_cloud_logging +from gigl.common.formatters import GCP_LABELS_RECORD_ATTR, GcpJsonFormatter _BASE_LOG_FILE_PATH = "/tmp/research/gbml/logs" -class Logger(logging.LoggerAdapter): +def _is_gcp_environment() -> bool: + """Return ``True`` when running on a GCP-managed platform (GKE, GAE, etc.). + + Checks the environment variables that GCP injects into container workloads. + Extracted as a standalone helper so tests can patch a single symbol. """ - GiGL's custom logger class used for local and cloud logging (VertexAI, Dataflow, etc.) + return bool(os.getenv("GAE_APPLICATION") or os.getenv("KUBERNETES_SERVICE_HOST")) + + +class Logger(logging.LoggerAdapter): + """GiGL's custom logger class used for local and cloud logging (VertexAI, Dataflow, etc.). + Args: - logger (Optional[logging.Logger]): A custom logger to use. If not provided, the default logger will be created. - name (Optional[str]): The name to be used for the logger. By default uses "root". - log_to_file (bool): If True, logs will be written to a file. If False, logs will be written to the console. - extra (Optional[dict[str, Any]]): Extra information to be added to the log message. + logger: A custom logger to use. If not provided, the default logger will be created. + name: The name to be used for the logger. By default uses "root". + log_to_file: If True, logs will be written to a file. If False, logs will be written to the console. + extra: Extra information to be added to the log message. """ def __init__( @@ -37,12 +47,10 @@ def _setup_logger( ) -> None: handler: logging.Handler if not logger.handlers: - if os.getenv("GAE_APPLICATION") or os.environ.get( - "KUBERNETES_SERVICE_HOST" - ): - # Google Cloud Logging - client = google_cloud_logging.Client() - client.setup_logging(log_level=logging.INFO) + if _is_gcp_environment(): + handler = logging.StreamHandler(stream=sys.stderr) + handler.setFormatter(GcpJsonFormatter()) + logger.addHandler(handler) else: # Logging locally. Set up logging to console or file if log_to_file: @@ -64,10 +72,10 @@ def _setup_logger( logger.setLevel(logging.INFO) def process(self, msg: str, kwargs: MutableMapping[str, Any]) -> Any: + merged: dict[str, Any] = dict(self.extra) if "extra" in kwargs: - kwargs["extra"].update(self.extra) - else: - kwargs["extra"] = self.extra + merged.update(kwargs["extra"]) + kwargs["extra"] = {**merged, GCP_LABELS_RECORD_ATTR: merged} return msg, kwargs def __getattr__(self, name: str): diff --git a/tests/unit/common/formatters/__init__.py b/tests/unit/common/formatters/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/unit/common/formatters/gcp_json_formatter_test.py b/tests/unit/common/formatters/gcp_json_formatter_test.py new file mode 100644 index 000000000..be264558b --- /dev/null +++ b/tests/unit/common/formatters/gcp_json_formatter_test.py @@ -0,0 +1,102 @@ +import json +import logging + +from gigl.common.formatters.gcp_json_formatter import GCP_LABELS_RECORD_ATTR, GcpJsonFormatter +from tests.test_assets.test_case import TestCase + + +class GcpJsonFormatterTest(TestCase): + def setUp(self) -> None: + self.formatter = GcpJsonFormatter() + + def _make_record( + self, + message: str = "test message", + level: int = logging.INFO, + exc_info: object = None, + ) -> logging.LogRecord: + """Create a ``LogRecord`` with deterministic source location.""" + record = logging.LogRecord( + name="test", + level=level, + pathname="test_file.py", + lineno=42, + msg=message, + args=None, + exc_info=exc_info, # type: ignore[arg-type] + ) + record.funcName = "test_func" + return record + + def test_basic_info_message_produces_valid_json(self) -> None: + record = self._make_record() + output = self.formatter.format(record) + parsed = json.loads(output) + + self.assertEqual(parsed["severity"], "INFO") + self.assertEqual(parsed["message"], "test message") + self.assertIn("time", parsed) + self.assertIn("logging.googleapis.com/sourceLocation", parsed) + + def test_severity_mapping_for_all_levels(self) -> None: + levels = { + logging.DEBUG: "DEBUG", + logging.INFO: "INFO", + logging.WARNING: "WARNING", + logging.ERROR: "ERROR", + logging.CRITICAL: "CRITICAL", + } + for python_level, expected_severity in levels.items(): + with self.subTest(level=python_level): + record = self._make_record(level=python_level) + parsed = json.loads(self.formatter.format(record)) + self.assertEqual(parsed["severity"], expected_severity) + + def test_output_is_single_line(self) -> None: + record = self._make_record() + output = self.formatter.format(record) + self.assertEqual(output.count("\n"), 0) + + def test_time_field_is_iso_8601(self) -> None: + record = self._make_record() + parsed = json.loads(self.formatter.format(record)) + time_str = parsed["time"] + # ISO 8601 with timezone: contains 'T' separator and '+' offset + self.assertIn("T", time_str) + self.assertIn("+", time_str) + + def test_extra_fields_appear_under_labels(self) -> None: + record = self._make_record() + setattr(record, GCP_LABELS_RECORD_ATTR, {"custom_key": "custom_value"}) + parsed = json.loads(self.formatter.format(record)) + + labels = parsed["logging.googleapis.com/labels"] + self.assertEqual(labels["custom_key"], "custom_value") + + def test_no_labels_key_when_no_extras(self) -> None: + record = self._make_record() + parsed = json.loads(self.formatter.format(record)) + self.assertNotIn("logging.googleapis.com/labels", parsed) + + def test_exception_traceback_in_message(self) -> None: + try: + raise ValueError("boom") + except ValueError: + import sys + + exc_info = sys.exc_info() + + record = self._make_record(exc_info=exc_info) + parsed = json.loads(self.formatter.format(record)) + + self.assertIn("ValueError: boom", parsed["message"]) + self.assertIn("Traceback", parsed["message"]) + + def test_source_location_fields(self) -> None: + record = self._make_record() + parsed = json.loads(self.formatter.format(record)) + source = parsed["logging.googleapis.com/sourceLocation"] + + self.assertEqual(source["file"], "test_file.py") + self.assertEqual(source["line"], 42) + self.assertEqual(source["function"], "test_func") diff --git a/tests/unit/common/logger_test.py b/tests/unit/common/logger_test.py new file mode 100644 index 000000000..69afd0910 --- /dev/null +++ b/tests/unit/common/logger_test.py @@ -0,0 +1,30 @@ +import logging +from unittest.mock import patch + +from gigl.common.formatters.gcp_json_formatter import GcpJsonFormatter +from gigl.common.logger import Logger +from tests.test_assets.test_case import TestCase + + +class LoggerModeSelectionTest(TestCase): + """Verify that ``Logger`` selects the correct handler/formatter based on environment.""" + + def tearDown(self) -> None: + # Clean up any loggers created during tests to avoid handler leaks. + logging.Logger.manager.loggerDict.clear() + logging.root.handlers.clear() + + @patch("gigl.common.logger._is_gcp_environment", return_value=True) + def test_gcp_mode_uses_gcp_json_formatter(self, _mock_is_gcp: object) -> None: + logger = Logger(name="test_gcp_mode") + handlers = logger.logger.handlers + self.assertTrue(len(handlers) > 0, "Expected at least one handler") + self.assertIsInstance(handlers[0].formatter, GcpJsonFormatter) + + @patch("gigl.common.logger._is_gcp_environment", return_value=False) + def test_local_mode_uses_standard_formatter(self, _mock_is_gcp: object) -> None: + logger = Logger(name="test_local_mode") + handlers = logger.logger.handlers + self.assertTrue(len(handlers) > 0, "Expected at least one handler") + self.assertNotIsInstance(handlers[0].formatter, GcpJsonFormatter) + self.assertIsInstance(handlers[0].formatter, logging.Formatter) From 9df00da9efbf6e4fd79ff6985b3fff82ecc4dd17 Mon Sep 17 00:00:00 2001 From: kmontemayor Date: Fri, 20 Feb 2026 19:17:27 +0000 Subject: [PATCH 3/3] revert --- gigl/common/logger.py | 12 ++---------- tests/unit/common/logger_test.py | 27 +-------------------------- 2 files changed, 3 insertions(+), 36 deletions(-) diff --git a/gigl/common/logger.py b/gigl/common/logger.py index 1562330fa..5e46a1fe9 100644 --- a/gigl/common/logger.py +++ b/gigl/common/logger.py @@ -75,15 +75,6 @@ def format(self, record: logging.LogRecord) -> str: return json.dumps(payload, ensure_ascii=False, default=str) -def _is_gcp_environment() -> bool: - """Return ``True`` when running on a GCP-managed platform (GKE, GAE, etc.). - - Checks the environment variables that GCP injects into container workloads. - Extracted as a standalone helper so tests can patch a single symbol. - """ - return bool(os.getenv("GAE_APPLICATION") or os.getenv("KUBERNETES_SERVICE_HOST")) - - class Logger(logging.LoggerAdapter): """GiGL's custom logger class used for local and cloud logging (VertexAI, Dataflow, etc.). @@ -112,7 +103,8 @@ def _setup_logger( ) -> None: handler: logging.Handler if not logger.handlers: - if _is_gcp_environment(): + # Check if running on GCP. + if os.getenv("GAE_APPLICATION") or os.getenv("KUBERNETES_SERVICE_HOST"): handler = logging.StreamHandler(stream=sys.stderr) handler.setFormatter(_GcpJsonFormatter()) logger.addHandler(handler) diff --git a/tests/unit/common/logger_test.py b/tests/unit/common/logger_test.py index 5f9936646..56f201726 100644 --- a/tests/unit/common/logger_test.py +++ b/tests/unit/common/logger_test.py @@ -1,9 +1,8 @@ import json import logging import sys -from unittest.mock import patch -from gigl.common.logger import Logger, _GCP_LABELS_RECORD_ATTR, _GcpJsonFormatter +from gigl.common.logger import _GCP_LABELS_RECORD_ATTR, _GcpJsonFormatter from tests.test_assets.test_case import TestCase @@ -100,27 +99,3 @@ def test_source_location_fields(self) -> None: self.assertEqual(source["file"], "test_file.py") self.assertEqual(source["line"], 42) self.assertEqual(source["function"], "test_func") - - -class LoggerModeSelectionTest(TestCase): - """Verify that ``Logger`` selects the correct handler/formatter based on environment.""" - - def tearDown(self) -> None: - # Clean up any loggers created during tests to avoid handler leaks. - logging.Logger.manager.loggerDict.clear() - logging.root.handlers.clear() - - @patch("gigl.common.logger._is_gcp_environment", return_value=True) - def test_gcp_mode_uses_gcp_json_formatter(self, _mock_is_gcp: object) -> None: - logger = Logger(name="test_gcp_mode") - handlers = logger.logger.handlers - self.assertTrue(len(handlers) > 0, "Expected at least one handler") - self.assertIsInstance(handlers[0].formatter, _GcpJsonFormatter) - - @patch("gigl.common.logger._is_gcp_environment", return_value=False) - def test_local_mode_uses_standard_formatter(self, _mock_is_gcp: object) -> None: - logger = Logger(name="test_local_mode") - handlers = logger.logger.handlers - self.assertTrue(len(handlers) > 0, "Expected at least one handler") - self.assertNotIsInstance(handlers[0].formatter, _GcpJsonFormatter) - self.assertIsInstance(handlers[0].formatter, logging.Formatter)