Merge pull request #178 from stanhu/sh-improve-log-messages

Improve error log message output

Author: casperisfine

hiredis-client/lib/redis_client/hiredis_connection.rb

@@ -39,6 +39,8 @@ def initialize(ca_file: nil, ca_path: nil, cert: nil, key: nil, hostname: nil)
       end
     end
 
+    attr_reader :config
+
     def initialize(config, connect_timeout:, read_timeout:, write_timeout:)
       super()
       @config = config
@@ -98,14 +100,20 @@ def read(timeout = nil)
         end
       end
     rescue SystemCallError, IOError => error
-      raise ConnectionError, error.message
+      raise ConnectionError.with_config(error.message, config)
+    rescue Error => error
+      error._set_config(config)
+      raise error
     end
 
     def write(command)
       _write(command)
       flush
     rescue SystemCallError, IOError => error
-      raise ConnectionError, error.message
+      raise ConnectionError.with_config(error.message, config)
+    rescue Error => error
+      error._set_config(config)
+      raise error
     end
 
     def write_multi(commands)
@@ -114,7 +122,10 @@ def write_multi(commands)
       end
       flush
     rescue SystemCallError, IOError => error
-      raise ConnectionError, error.message
+      raise ConnectionError.with_config(error.message, config)
+    rescue Error => error
+      error._set_config(config)
+      raise error
     end
 
     private

lib/redis_client.rb

@@ -77,7 +77,29 @@ def timeout=(timeout)
     end
   end
 
-  Error = Class.new(StandardError)
+  module HasConfig
+    attr_reader :config
+
+    def _set_config(config)
+      @config = config
+    end
+
+    def message
+      return super unless config
+
+      "#{super} (#{config.server_url})"
+    end
+  end
+
+  class Error < StandardError
+    include HasConfig
+
+    def self.with_config(message, config = nil)
+      new(message).tap do |error|
+        error._set_config(config)
+      end
+    end
+  end
 
   ProtocolError = Class.new(Error)
   UnsupportedServer = Class.new(Error)
@@ -114,7 +136,7 @@ def parse(error_message)
         end
         code ||= error_message.split(' ', 2).first
         klass = ERRORS.fetch(code, self)
-        klass.new(error_message)
+        klass.new(error_message.strip)
       end
     end
   end
@@ -750,10 +772,13 @@ def connect
         end
       end
     end
-  rescue FailoverError, CannotConnectError
-    raise
+  rescue FailoverError, CannotConnectError => error
+    error._set_config(config)
+    raise error
   rescue ConnectionError => error
-    raise CannotConnectError, error.message, error.backtrace
+    connect_error = CannotConnectError.with_config(error.message, config)
+    connect_error.set_backtrace(error.backtrace)
+    raise connect_error
   rescue CommandError => error
     if error.message.match?(/ERR unknown command ['`]HELLO['`]/)
       raise UnsupportedServer,

lib/redis_client/config.rb

@@ -124,10 +124,17 @@ def ssl_context
 
       def server_url
         if path
-          "#{path}/#{db}"
+          url = "unix://#{path}"
+          if db != 0
+            url = "#{url}?db=#{db}"
+          end
         else
-          "redis#{'s' if ssl?}://#{host}:#{port}/#{db}"
+          url = "redis#{'s' if ssl?}://#{host}:#{port}"
+          if db != 0
+            url = "#{url}/#{db}"
+          end
         end
+        url
       end
 
       private

lib/redis_client/connection_mixin.rb

@@ -32,6 +32,7 @@ def call(command, timeout)
       @pending_reads -= 1
       if result.is_a?(Error)
         result._set_command(command)
+        result._set_config(config)
         raise result
       else
         result
@@ -53,10 +54,16 @@ def call_pipelined(commands, timeouts)
 
         # A multi/exec command can return an array of results.
         # An error from a multi/exec command is handled in Multi#_coerce!.
-        if result.is_a?(Error)
+        if result.is_a?(Array)
+          result.each do |res|
+            res._set_config(config) if res.is_a?(Error)
+          end
+        elsif result.is_a?(Error)
           result._set_command(commands[index])
+          result._set_config(config)
           exception ||= result
         end
+
         results[index] = result
       end
 

lib/redis_client/ruby_connection.rb

@@ -40,6 +40,8 @@ def ssl_context(ssl_params)
 
     SUPPORTS_RESOLV_TIMEOUT = Socket.method(:tcp).parameters.any? { |p| p.last == :resolv_timeout }
 
+    attr_reader :config
+
     def initialize(config, connect_timeout:, read_timeout:, write_timeout:)
       super()
       @config = config
@@ -73,7 +75,7 @@ def write(command)
       begin
         @io.write(buffer)
       rescue SystemCallError, IOError => error
-        raise ConnectionError, error.message
+        raise ConnectionError.with_config(error.message, config)
       end
     end
 
@@ -85,7 +87,7 @@ def write_multi(commands)
       begin
         @io.write(buffer)
       rescue SystemCallError, IOError => error
-        raise ConnectionError, error.message
+        raise ConnectionError.with_config(error.message, config)
       end
     end
 
@@ -96,9 +98,9 @@ def read(timeout = nil)
         @io.with_timeout(timeout) { RESP3.load(@io) }
       end
     rescue RedisClient::RESP3::UnknownType => error
-      raise RedisClient::ProtocolError, error.message
+      raise RedisClient::ProtocolError.with_config(error.message, config)
     rescue SystemCallError, IOError, OpenSSL::SSL::SSLError => error
-      raise ConnectionError, error.message
+      raise ConnectionError.with_config(error.message, config)
     end
 
     def measure_round_trip_delay
@@ -130,9 +132,9 @@ def connect
         loop do
           case status = socket.connect_nonblock(exception: false)
           when :wait_readable
-            socket.to_io.wait_readable(@connect_timeout) or raise CannotConnectError
+            socket.to_io.wait_readable(@connect_timeout) or raise CannotConnectError.with_config("", config)
           when :wait_writable
-            socket.to_io.wait_writable(@connect_timeout) or raise CannotConnectError
+            socket.to_io.wait_writable(@connect_timeout) or raise CannotConnectError.with_config("", config)
           when socket
             break
           else

test/redis_client/config_test.rb

@@ -198,13 +198,13 @@ def test_overriding
     end
 
     def test_server_url
-      assert_equal "redis://localhost:6379/0", Config.new.server_url
-      assert_equal "redis://localhost:6379/0", Config.new(username: "george", password: "hunter2").server_url
+      assert_equal "redis://localhost:6379", Config.new.server_url
+      assert_equal "redis://localhost:6379", Config.new(username: "george", password: "hunter2").server_url
       assert_equal "redis://localhost:6379/5", Config.new(db: 5).server_url
-      assert_equal "redis://example.com:8080/0", Config.new(host: "example.com", port: 8080).server_url
-      assert_equal "rediss://localhost:6379/0", Config.new(ssl: true).server_url
+      assert_equal "redis://example.com:8080", Config.new(host: "example.com", port: 8080).server_url
+      assert_equal "rediss://localhost:6379", Config.new(ssl: true).server_url
 
-      assert_equal "/var/redis/redis.sock/5", Config.new(path: "/var/redis/redis.sock", db: 5).server_url
+      assert_equal "unix:///var/redis/redis.sock?db=5", Config.new(path: "/var/redis/redis.sock", db: 5).server_url
     end
 
     def test_custom_field

test/redis_client/connection_test.rb

@@ -26,26 +26,32 @@ def test_connected?
 
     def test_connect_failure
       client = new_client(host: "example.com")
-      assert_raises RedisClient::ConnectionError do
+      error = assert_raises RedisClient::ConnectionError do
         client.call("PING")
       end
+
+      assert_match(%r{ \(rediss?://example.com:.*\)$}, error.message)
     end
 
     def test_redis_down_after_connect
       @redis.call("PING") # force connect
       Toxiproxy[/redis/].down do
-        assert_raises RedisClient::ConnectionError do
+        error = assert_raises RedisClient::ConnectionError do
           @redis.call("PING")
         end
+
+        assert_match(%r{ \(rediss?://127.0.0.1:.*\)$}, error.message)
       end
     end
 
     def test_redis_down_before_connect
       @redis.close
       Toxiproxy[/redis/].down do
-        assert_raises RedisClient::ConnectionError do
+        error = assert_raises RedisClient::ConnectionError do
           @redis.call("PING")
         end
+
+        assert_match(%r{ \(rediss?://127.0.0.1:.*\)$}, error.message)
       end
     end
 
@@ -291,9 +297,11 @@ class TCPConnectionTest < Minitest::Test
 
     def test_connecting_to_a_ssl_server
       client = new_client(**ssl_config, ssl: false)
-      assert_raises CannotConnectError do
+      error = assert_raises CannotConnectError do
         client.call("PING")
       end
+
+      assert_match(%r{ \(rediss?://.*:.*\)$}, error.message)
     end
 
     def test_protocol_error
@@ -307,9 +315,11 @@ def test_protocol_error
         session.close
       end
 
-      assert_raises RedisClient::ProtocolError do
+      error = assert_raises RedisClient::ProtocolError do
         new_client(host: "127.0.0.1", port: port).call("PING")
       end
+
+      assert_match(%r{ \(rediss?://127.0.0.1:#{port}\)$}, error.message)
     ensure
       server_thread&.kill
     end
@@ -372,10 +382,11 @@ def test_reconnect_on_readonly
 
       client = new_client(host: "127.0.0.1", port: port)
       client.call("PING")
-      assert_raises RedisClient::ReadOnlyError do
+      error = assert_raises RedisClient::ReadOnlyError do
         client.call("SET", "foo", "bar")
       end
       refute_predicate client, :connected?
+      assert_match(%r{ \(rediss?://127.0.0.1:#{port}\)$}, error.message)
     ensure
       server_thread&.kill
     end

test/redis_client_test.rb

@@ -43,9 +43,11 @@ def test_encoding
 
   def test_dns_resolution_failure
     client = RedisClient.new(host: "does-not-exist.example.com")
-    assert_raises RedisClient::ConnectionError do
+    error = assert_raises RedisClient::ConnectionError do
       client.call("PING")
     end
+
+    assert_match(%r{ \(redis://does-not-exist.example.com:.*\)$}, error.message)
   end
 
   def test_older_server
@@ -64,6 +66,7 @@ def call_pipelined(commands, *)
       client.call("PING")
     end
     assert_includes error.message, "redis-client requires Redis 6+ with HELLO command available"
+    assert_includes error.message, "(redis://"
   end
 
   def test_redis_6_server_with_missing_hello_command
@@ -82,6 +85,7 @@ def call_pipelined(commands, *)
       client.call("PING")
     end
     assert_includes error.message, "redis-client requires Redis 6+ with HELLO command available"
+    assert_includes error.message, "(redis://"
   end
 
   def test_handle_async_raise
@@ -123,7 +127,7 @@ def test_measure_round_trip_delay
   end
 
   def test_server_url
-    assert_equal "redis://#{Servers::HOST}:#{Servers::REDIS_TCP_PORT}/0", @redis.server_url
+    assert_equal "redis://#{Servers::HOST}:#{Servers::REDIS_TCP_PORT}", @redis.server_url
   end
 
   def test_timeout

test/shared/redis_client_tests.rb

@@ -271,30 +271,30 @@ def test_wrong_type
       @redis.call("SISMEMBER", "str", "member")
     end
     assert_equal ["SISMEMBER", "str", "member"], error.command
-    assert_includes error.message, "WRONGTYPE Operation against a key holding the wrong kind of value"
+    assert_match(/WRONGTYPE Operation against a key holding the wrong kind of value (.*:.*)/, error.message)
 
     error = assert_raises RedisClient::CommandError do
       @redis.pipelined do |pipeline|
         pipeline.call("SISMEMBER", "str", "member")
       end
     end
     assert_equal ["SISMEMBER", "str", "member"], error.command
-    assert_includes error.message, "WRONGTYPE Operation against a key holding the wrong kind of value"
+    assert_match(/WRONGTYPE Operation against a key holding the wrong kind of value (.*:.*)/, error.message)
 
     error = assert_raises RedisClient::CommandError do
       @redis.multi do |transaction|
         transaction.call("SISMEMBER", "str", "member")
       end
     end
     assert_equal ["SISMEMBER", "str", "member"], error.command
-    assert_includes error.message, "WRONGTYPE Operation against a key holding the wrong kind of value"
+    assert_match(/WRONGTYPE Operation against a key holding the wrong kind of value (.*:.*)/, error.message)
   end
 
   def test_command_missing
     error = assert_raises RedisClient::CommandError do
       @redis.call("DOESNOTEXIST", "foo")
     end
-    assert_match(/ERR unknown command .DOESNOTEXIST./, error.message)
+    assert_match(/ERR unknown command .DOESNOTEXIST.*\(.*:.*\)/, error.message)
   end
 
   def test_authentication