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
|
I am not fully sure this will solve #850 and #855. I see 2 possible scenarios:
Have you considered flushing the queue at termination? Something like the following (conceptually):
I wonder if the atexit hook should remain. I believe the main thread will be in a state where is_alive may return true, but underneath it would still error out because it can’t create new threads (this is a guess). What do you think about letting users explicitly calling the teardown/close? |
gkevinzheng
force-pushed
the
cloud-logging-handler-flush
branch
from
db3be06 to
b46779e
Compare
|
Here is the summary of changes. You are about to add 4 region tags.
You are about to delete 2 region tags.
This comment is generated by snippet-bot.
|
For design doc see go/python-logging-background-thread
Fixes #<issue_number_goes_here> 🦕