Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for Cloud Run jobs (cloud_run_job) to the default logging handler, returning a StructuredLogHandler when detected, and includes a corresponding unit test. The feedback suggests combining the duplicate elif blocks for Cloud Run revisions and jobs into a single membership check, and updating the mocked environment variables in the unit test to use string values instead of integers to accurately mimic os.environ.
| elif monitored_resource.type == _RUN_RESOURCE_TYPE: | ||
| return StructuredLogHandler(**kw, project_id=self.project) | ||
| elif monitored_resource.type == _JOB_RESOURCE_TYPE: | ||
| return StructuredLogHandler(**kw, project_id=self.project) |
There was a problem hiding this comment.
Instead of having separate elif blocks that return the exact same StructuredLogHandler instance, we can combine them into a single elif block using membership testing (in). This reduces duplication and improves maintainability.
| elif monitored_resource.type == _RUN_RESOURCE_TYPE: | |
| return StructuredLogHandler(**kw, project_id=self.project) | |
| elif monitored_resource.type == _JOB_RESOURCE_TYPE: | |
| return StructuredLogHandler(**kw, project_id=self.project) | |
| elif monitored_resource.type in (_RUN_RESOURCE_TYPE, _JOB_RESOURCE_TYPE): | |
| return StructuredLogHandler(**kw, project_id=self.project) |
118a445 to
e2001f1
Compare
By default, when we ask Google Cloud to configure logging for us in a Cloud Run Job, it gives us a `CloudLoggingHandler`. This starts a background thread and ships log messages to google via an API. A `StructuredLogHandler` just prints log messges to stdout. This is all we really need. The added complexity of managing another thread is making it harder to debug some issues with asyncio resource management in `cloud-sql-python-connector`. Some might argue that this is a bug in the Google Cloud Python SDK, as they say in their docs that `StructuredLogHandler` is the default handler for Cloud Run. "Cloud Run" is like 1,000 different things though so who's to say what that really means, and if it includes Cloud Run Jobs. In case it does, Here's a [PR](googleapis/google-cloud-python#18707) to change the Google Cloud Python SDK behavior.
Use StructuredLogHandler instead of CloudLoggingHandler by default in
Cloud Run Jobs. There's no reason that I can think that we can't just
log to stdout in Cloud Run Jobs by default like we do in Cloud Run
Services and Cloud Run Functions. Starting a background thread for log
shipping adds complexity and can cause errors, especiallly when
interactiing with other libs like `aiohttp`, leading to errors like
this:
```
File "/usr/local/lib/python3.13/site-packages/aiohttp/connector.py", line 379, in __del__
self._loop.call_exception_handler(context)
File "/usr/local/lib/python3.13/asyncio/base_events.py", line 1925, in call_exception_handler
logger.error('Exception in default exception handler',
File "/usr/local/lib/python3.13/logging/__init__.py", line 1554, in error
self._log(ERROR, msg, args, **kwargs)
File "/usr/local/lib/python3.13/logging/__init__.py", line 1670, in _log
self.handle(record)
File "/usr/local/lib/python3.13/logging/__init__.py", line 1686, in handle
self.callHandlers(record)
File "/usr/local/lib/python3.13/logging/__init__.py", line 1746, in callHandlers
hdlr.handle(record)
File "/usr/local/lib/python3.13/logging/__init__.py", line 1027, in handle
self.emit(record)
File "/usr/local/lib/python3.13/site-packages/google/cloud/logging_v2/handlers/handlers.py", line 218, in emit
self.transport = self._transport_cls(
File "/usr/local/lib/python3.13/site-packages/google/cloud/logging_v2/handlers/transports/background_thread.py", line 324, in __init__
self.worker.start()
File "/usr/local/lib/python3.13/site-packages/google/cloud/logging_v2/handlers/transports/background_thread.py", line 173, in start
self._thread.start()
File "/usr/local/lib/python3.13/threading.py", line 976, in start
_start_joinable_thread(self._bootstrap, handle=self._handle,
PythonFinalizationError: can't create new thread at interpreter shutdown
```
Of course just because *I* can't think of a reason why
`CloudLoggingHandler` should be the default doesn't mean there isn't
one, and this is a breaking change that could cause problems for folks
depending on the existing behavior.
Fixes googleapis#15407
BREAKING CHANGE: This changes the default log handler for all Cloud Run
Jobs
e2001f1 to
16dbc1d
Compare
BREAKING CHANGE: Use StructuredLogHandler instead of CloudLoggingHandler by default in Cloud Run Jobs. There's no reason that I can think that we can't just log to stdout in Cloud Run Jobs by default like we do in Cloud Run Services and Cloud Run Functions. Starting a background thread for log shipping adds complexity and can cause errors, especiallly when interactiing with other libs like
aiohttp, leading to errors like this:Of course just because I can't think of a reason why
CloudLoggingHandlershould be the default doesn't mean there isn't one, and this is a breaking change that could cause problems for folks depending on the existing behavior.Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #15407 🦕