add logger support and better support for custom exception handlers #136
Conversation
andreaseger
force-pushed
the
add-logger-support
branch
from
88a3e0d to
d7ed617
Compare
| # TODO: logger | ||
| $stderr.puts "Caught exception when recovering exchange #{x.name}" | ||
| logger.info("Caught exception when recovering exchange #{x.name}") | ||
| logger.error(e) |
There was a problem hiding this comment.
So I do 2 calls to the logger in all cases where I have a message + a exception object so that the logger implementation can decide itself how it wants to serialize the exception.
| "Connection reset by peer" | ||
| ]) | ||
|
|
||
| def is_socket_closed_or_connection_reset(error) |
There was a problem hiding this comment.
I had to re-implement this here as it's private in the java implementation and therefore not accessible in subclasses
https://github.com/rabbitmq/rabbitmq-java-client/blob/master/src/main/java/com/rabbitmq/client/impl/ForgivingExceptionHandler.java#L126
lib/march_hare/session.rb
Outdated
| @@ -608,6 +613,32 @@ def tls_certificate_password_from(opts) | |||
| self.class.tls_certificate_password_from(opts) | |||
| end | |||
|
|
|||
| # @private | |||
| def init_default_exception_handler(logger) | |||
| StrictExceptionHandler.new(logger) | |||
There was a problem hiding this comment.
AFAICS this is also the default in the java lib
https://github.com/rabbitmq/rabbitmq-java-client/blob/master/src/main/java/com/rabbitmq/client/impl/DefaultExceptionHandler.java
| "Connection reset by peer" | ||
| ]) | ||
|
|
||
| def is_socket_closed_or_connection_reset(error) |
There was a problem hiding this comment.
I understand the intent here but lumping these together makes little sense. "TCP connection reset" means a TCP connection was refused by the peer. Socket closure is a different scenario.
Naming predicate methods as is_predicate in Ruby is very uncommon. Use predicate? instead.
I'd rename this to warning? or similar.
There was a problem hiding this comment.
Yeah good idea regarding the naming I just took the exact behavior & naming from the private java method. See https://github.com/rabbitmq/rabbitmq-java-client/blob/master/src/main/java/com/rabbitmq/client/impl/ForgivingExceptionHandler.java#L112-L131
There was a problem hiding this comment.
I guess we could also decide to simply not do this separation between certain exceptions and just always log it as error
lib/march_hare/session.rb
Outdated
| @@ -608,6 +613,32 @@ def tls_certificate_password_from(opts) | |||
| self.class.tls_certificate_password_from(opts) | |||
| end | |||
|
|
|||
| # @private | |||
| def init_default_exception_handler(logger) | |||
| StrictExceptionHandler.new(logger) | |||
There was a problem hiding this comment.
I was never very happy about this default in the Java client, which goes back to 1.0 or so.
There was a problem hiding this comment.
I could use the other one(ForgivingExceptionHandler) as default if that's preferred I simply didn't want to change existing behavior
There was a problem hiding this comment.
That would be closer to how Bunny works.
|
There is no need for separate PRs I don't think there is a good way to test a logger without mocks. Not sure how useful they would be. Any integration test suite run and realistic application would test logging quite extensively. |
|
Will update the PR tomorrow. Re. the failed build on travis is that one test somehow instable? I've seen it fail occasionally on my local system too. |
|
I pushed a change that should make it more reliable. |
andreaseger
force-pushed
the
add-logger-support
branch
from
d7ed617 to
9fbdd5d
Compare
|
Thank you! |
Hi
we encountered some unexpected behavior using march_hare and only became aware of them after discovering some not very detailed output to stderr/stdout. Therefore - before fixing the actual issue we encountered - I thought march_hare needs logger support.
The interface is
stolencopied from bunny as far as it made sense. I also changes all uses ofputswill calls to the logger. Additionally I added some newdebuglog messages although for these I'm not sure about their real life usefulness (also the message format/wording can probably be improved)To also be able to log more things from the rabbitmq java lib I brought the exception handlers to the ruby world and made them log their messages to the configured logger instance instead of the previously used slf4j interface. Aside from this logging change they have the identical behavior of the java classes.
I'm not sure if or how this logger support should be tested. I'm welcome for input in this regard. Also if you prefer I can split out the exception handler stuff into a separate PR (or remove it if you don't see this as part of the gem)
fixes #35