Improve circuit error messages #176
Conversation
|
|
| @@ -5,16 +5,16 @@ class ServiceFailureError < Circuitbox::Error | |||
| attr_reader :service, :original | |||
|
|
|||
| def initialize(service, exception) | |||
| super() | |||
| super("Service #{service.inspect} was unavailable: #{exception}") | |||
There was a problem hiding this comment.
This exception class overrides the to_s method which means this message would never be used/returned.
This can be validated with the following code snippet
class BaseError < StandardError; end
class CustomError < BaseError
def initialize
super("message from initialize")
end
def to_s
"message from to_s"
end
end
begin
raise CustomError
rescue CustomError => e
puts e.message
endThere was a problem hiding this comment.
Good catch, I'll adjust the to_s instead
| @@ -5,7 +5,7 @@ class OpenCircuitError < Circuitbox::Error | |||
| attr_reader :service | |||
|
|
|||
| def initialize(service) | |||
| super() | |||
| super("Service #{service.inspect} has an open circuit") | |||
There was a problem hiding this comment.
Since the service is converted into a string in CircuitBreaker is there a specific reason inspect is being called?
I think it makes sense to have some standard exception message for this error. Previously one would need to rescue the exception, read the value of service, and construct their own message. This gets rid of the need to do that but with the downside of always allocating the message regardless of it being read. The message should be lazily created by overriding the to_s method like what ServiceFailureError does.
There was a problem hiding this comment.
inspect is to make the substituted value separate from the surrounding text for readability. Imagine you are using local hostnames for your services, for example you have a machine called gollum and you can reach it with its' bare hostname. If you template the name into a message "as is" it can be confusing because your message will look like Service gollum has an open circuit. If the service name consists of multiple words separated by spaces the value visually dissolves in the rest of the message. Since there are no strictly defined semantics for quoting / Markdown in exception messages I opt for the next best thing and use inspect for this "auto-quote". I think in some other instances a combo of backtick+single quote is used too in some Ruby builtin messages, we can also uses that?
test/service_failure_error_test.rb
Outdated
| @@ -13,7 +13,7 @@ def setup | |||
|
|
|||
| def test_includes_the_message_of_the_wrapped_exception | |||
| ex = Circuitbox::ServiceFailureError.new('test', error) | |||
| assert_equal "Circuitbox::ServiceFailureError wrapped: #{error}", ex.to_s | |||
| assert_equal "Circuitbox::ServiceFailureError (original: #{error})", ex.to_s | |||
There was a problem hiding this comment.
This test needs to be updated to handle the new response.
There was a problem hiding this comment.
I'll take care of it, thanks!
When these happen it is useful to at least know which service has raised the error. This can be ever more relevant if a circuit breaker is initialised dynamically - for example, per host getting called.