Skip to content

Improve error log message output #178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

stanhu
Copy link
Contributor

@stanhu stanhu commented Feb 16, 2024

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 #177

@stanhu stanhu force-pushed the sh-improve-log-messages branch from 5ef5641 to fe0c3a3 Compare February 16, 2024 15:01
@casperisfine
Copy link
Collaborator

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?

@stanhu stanhu force-pushed the sh-improve-log-messages branch 3 times, most recently from b353025 to cd92dee Compare February 16, 2024 22:32
@stanhu stanhu requested a review from casperisfine February 19, 2024 15:16
@casperisfine
Copy link
Collaborator

Looks good now but:

  • We probably should do this for most if not all errors, not just CommandError.
  • 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.

@stanhu
Copy link
Contributor Author

stanhu commented Feb 19, 2024

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
Copy link
Member

byroot commented Feb 19, 2024

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
Copy link
Member

byroot commented Feb 19, 2024

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

@stanhu stanhu force-pushed the sh-improve-log-messages branch from cd92dee to 835698a Compare February 20, 2024 05:33
@casperisfine
Copy link
Collaborator

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 stanhu force-pushed the sh-improve-log-messages branch from 835698a to fa56ab3 Compare February 20, 2024 16:52
@stanhu
Copy link
Contributor Author

stanhu commented Mar 3, 2024

@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
Copy link
Member

byroot commented Mar 3, 2024

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

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 redis-rb#177
@casperisfine casperisfine force-pushed the sh-improve-log-messages branch 2 times, most recently from 310b4aa to c6f936c Compare March 4, 2024 12:45
- Don't include the db if it's `0`
- Show unix sockets with `unix://` scheme
@casperisfine casperisfine force-pushed the sh-improve-log-messages branch from c6f936c to 5b0e6f7 Compare March 4, 2024 13:04
@casperisfine casperisfine merged commit 7879c84 into redis-rb:master Mar 4, 2024
@casperisfine
Copy link
Collaborator

0.21.0 is out with these changes.

stanhu added a commit to stanhu/redis-client that referenced this pull request Mar 21, 2024
redis-rb#178 introduced
a regression that caused a ConnectionError to be thrown to the
caller if the Sentinel master or replica could not be resolved.

When a ConnectionError is thrown, the error message handler
would attempt to retrieve `config.server_url`, but this in turn
causes another Sentinel resolution to be attempted.

We avoid this by adding a `resolved?` method that will indicate
whether the config can be used. The error handler won't attempt
to provide more details if the config has yet to be resolved.

Closes redis-rb#182
stanhu added a commit to stanhu/redis-client that referenced this pull request Mar 21, 2024
redis-rb#178 introduced
a regression that caused a ConnectionError to be thrown to the
caller if the Sentinel master or replica could not be resolved.

When a ConnectionError is thrown, the error message handler
would attempt to retrieve `config.server_url`, but this in turn
causes another Sentinel resolution to be attempted.

We avoid this by adding a `resolved?` method that will indicate
whether the config can be used. The error handler won't attempt
to provide more details if the config has yet to be resolved.

Closes redis-rb#182
stanhu added a commit to stanhu/redis-client that referenced this pull request Mar 21, 2024
redis-rb#178 introduced
a regression that caused a `ConnectionError` to be thrown to the
caller if the Sentinel master or replica could not be resolved.

When a `ConnectionError` is thrown, the error message handler would
attempt to retrieve `config.server_url`, but this in turn causes
another Sentinel resolution to be attempted.

We avoid this by adding a `resolved?` method that will indicate
whether the config can be used. The error handler won't attempt
to provide more details if the config has yet to be resolved.

Closes redis-rb#182
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it easier to discern between errors from Redis Sentinel vs. Redis nodes
3 participants