redis-rb / redis-client

Simple low level client for Redis 6+
MIT License
124 stars 60 forks source link

Improve error log message output #178

Closed stanhu closed 7 months ago

stanhu commented 8 months ago

Previously when an error were received by a Redis server, it was not clear whether this came from a Redis Sentinel or a Redis server in the cluster. This commit adds the hostname/port of the server to the exception message via a log context.

Closes https://github.com/redis-rb/redis-client/issues/177

casperisfine commented 8 months ago

Note that this commit also fixes an existing bug where an error in a MULTI/EXEC command would not properly be processed.

Could you open this in it's own PR please?

casperisfine commented 7 months ago

Looks good now but:

stanhu commented 7 months ago

As I said I need to review what the proper way to enrich Exception#message is, I'm pretty sure it's not #to_s.

I believe in Ruby 3.2 detailed_message is the way to do this (https://bugs.ruby-lang.org/issues/18564, https://rubyreferences.github.io/rubychanges/3.2.html#exceptions), but I think previous versions still need a solution. I kept to_s as the lowest common denominator for now.

byroot commented 7 months ago

Right, https://bugs.ruby-lang.org/issues/18438 is the one I vaguely remembered.

But detailed_message is aimed at printing errors in the console, so e.g. development environment. In this case, including the server URL make sense even in production.

So maybe it's to_s we want, but I really suspect it's #message (or just both).

byroot commented 7 months ago

Ideally we'd just augment the message in #initialize, but we can't really do that here :/

casperisfine commented 7 months ago

Yeah, it's definitely #message that should be redefined:

class MyError < StandardError
  def message
    super.upcase
  end
end

raise MyError, "test"
$ ruby /tmp/error.rb
/tmp/error.rb:7:in `<main>': TEST (MyError)
stanhu commented 7 months ago

@casperisfine Is there anything else I can do to get this merged? We're seeing some timeouts associated with RedisClient::CannotConnectError, but we can't tell what server is affected.

byroot commented 7 months ago

Yeah sorry this fell through the cracks a bit. I'll a reminder to get back at it very soon.

casperisfine commented 7 months ago

0.21.0 is out with these changes.