Open stefanchrobot opened 4 years ago
@stefanchrobot Implemented a fix for this. You have any thoughts on the change? https://github.com/dwbutler/logstash-logger/pull/165
@marcoadkins Thanks for taking the time to fix this! The change looks good, but I'm wondering if it would make sense to add a catch-all clause here: https://github.com/dwbutler/logstash-logger/pull/165/files#diff-247c8156e87173ba822a64e0e41ec50cR8
rescue StandardError
log_error(e)
'failed to format log event'
end
This would protect from any future errors that might pop out. I think it would make sense to add it in the Base
formatter to avoid repeating it in all the places.
@stefanchrobot Thats a good idea. I could add that in. Can't add it to the base formatter because they all override the format_event
method so it wouldn't propagate.
@stefanchrobot I see what you were saying now. The recommended change has been made.
The
LogStashLogger
can raise an exception when logging. This makes it unsuitable for error logging, as that would require error handling code for error handling code.Note that this issue is not about the particular error (see https://github.com/dwbutler/logstash-logger/issues/158 for that), rather about the whole approach - logging must not raise exceptions.
Reproduction:
Expected result: Swallow any error. Optionally log a warning.
Actual result: Exception.