fix: Log logging_api.write_entries errors internally rather than crashing
#958
Conversation
product-auto-label
bot
added
size: m
| try: | ||
| client.logging_api.write_entries(entries, partial_success=True) | ||
| except Exception: | ||
| _LOGGER.error("Failed to submit log message.", exc_info=True) |
There was a problem hiding this comment.
I don't think it's a good idea to just swallow all exceptions here. All grpc networking errors would now be ignored (ex bad IAM credentials), and users will have no idea anything is wrong unless they know to subscribe to the internal _LOGGER. And existing customers may have error handling logic in place that will now be bypassed, so this could be a breaking change hidden in a bug fix
There was a problem hiding this comment.
Could we just do json_format.ParseErrors instead?
There was a problem hiding this comment.
You mean swallowing the specific exception instead of all exceptions?
Honestly, I think just raising an exception here makes sense, so the user knows their input isn't handled. I'd do a raise ValueError(...) from e though, so we can give more context instead of raising the raw internal error. And then add a Raises: section in the docstring describing this situation
| info (dict): | ||
| the log entry information. The dict must be serializable | ||
| to a Protobuf Struct (map from strings to Values, see | ||
| https://protobuf.dev/reference/protobuf/google.protobuf/#value), |
There was a problem hiding this comment.
is it possible to lay out the specific types that are serializable?
Nested dicts and lists could make it complicated. But maybe we could do something like dict[str, float|bool|str|dict|list|None]
| the log entry information. The dict must be serializable | ||
| to a Protobuf Struct (map from strings to Values, see | ||
| https://protobuf.dev/reference/protobuf/google.protobuf/#value), | ||
| otherwise the item will not be logged. |
There was a problem hiding this comment.
it might be good to add more info to the main text too, instead of "Log a dictionary message"
| # attempt to attach extra contex on which log caused error | ||
| self._append_context_to_error(e) | ||
| raise e | ||
| except Exception as e: |
There was a problem hiding this comment.
same comment as above: hiding exceptions by default seems like it could have unintentional consequences, and could be a breaking change
Two fixes for #943:
log_structLogger._do_logandBatch.commitwill log errors internally rather than reraising an Exception which could propagate to user code when callinglogging_api.write_entries