feat: Added flushes/close functionality to logging handlers #917
Conversation
product-auto-label
bot
added
size: l
gkevinzheng
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| if self.transport is None: | ||
| self.transport = self._transport_cls( | ||
| self.client, self.name, resource=self.resource | ||
| ) |
There was a problem hiding this comment.
Wouldn't this mean the handler has been closed? Why not just raise an error, instead of re-creating the transport?
There was a problem hiding this comment.
I based this off of some logging handlers I saw in the Python standard library, which will reopen a closed connection if that connection is closed.
There was a problem hiding this comment.
Personally I think this adds extra complexity, so I'd prefer to keep them permanently closed if we can get away with that
But if you think this behavior would help us integrate with the standard library, it makes sense to keep it.
| def close(self): | ||
| """Closes the log handler and cleans up all Transport objects used.""" | ||
| self.transport.close() | ||
| self.transport = None |
There was a problem hiding this comment.
I don't like the idea of using transport == None as a way to check if the handler is closed. That makes the types quite a bit more complicated.
Can't we use a new flag for this?
| _CLOSE_THREAD_SHUTDOWN_ERROR_MSG = ( | ||
| "CloudLoggingHandler shutting down, cannot send logs entries to Cloud Logging due to " | ||
| "inconsistent threading behavior at shutdown. To avoid this issue, flush the logging handler " | ||
| "manually or switch to StructuredLogHandler." |
There was a problem hiding this comment.
Maybe mention client.close or handler.close too?
| """Callback that attempts to send pending logs before termination.""" | ||
| if not self.is_alive: | ||
| return | ||
|
|
||
| # Print different messages to the user depending on whether or not the | ||
| # program is shutting down. This is because this function now handles both | ||
| # the atexit handler and the regular close. |
There was a problem hiding this comment.
Can we decouple this? Maybe let the caller pass in a message or something?
product-auto-label
bot
added
the
stale: old
product-auto-label
bot
added
stale: extraold
For design doc see go/python-logging-background-thread
Fixes #<issue_number_goes_here> 🦕