Ignore socket errors always #2

Closed
wants to merge 1 commit into
from

2 participants

@j-manu

Ignore socket errors even if logger is not set. If logger is set log the debug message and error message (if any)

@jeremy
Owner

We squeeze a little better perf by avoiding the logging code path. How about something like:

module Sending
  def send_to_socket(message)
    @socket.send message, 0, @host, @port
  rescue => error
    handle_socket_error error, message
  end

  def handle_socket_error(error, message)
    # Ignore errors by default
  end
end

module Logging
  def handle_socket_error(error, message)
    super
    self.class.logger.error { "Statsd: #{error.class} #{error}" }
  end
end
@j-manu

The debug log statement is not available then. I'm ok with that but for those who need logging it may not be acceptable

A standard benchmark around send_to_socket with and without this change should show the change? Didn't think there would be performance loss due to calling self.class.logger :)

@jeremy
Owner

Yeah -- would still need Logging to override send_to_socket for the debug message.

I think you're right about performance, too :) This feels like defensive coding. I profiled the code and just tried to remove stuff from the hot path, but this probably doesn't really matter.

@jeremy jeremy added a commit that closed this pull request May 30, 2012
@j-manu j-manu Ignore socket errors even if logger is not set.
Closes #2
fecfbc5
@jeremy jeremy closed this in fecfbc5 May 30, 2012
@j-manu

Made a stupid typo in the code logger is missing an "r" in the second if :(

@jeremy
Owner

@j-manu caught it :)

@j-manu

Cool. I had mistakenly thought that both logger statements were tested. I see that you have added the tests now :)

@jeremy
Owner

Felt guilty ;) Thanks again for the pull ❤️

@j-manu

entirely my fault :).

Btw thanks for maintaining this. I literally spent an hour looking at the network graph of original reinh/statsd trying to decide which fork to use and finally settled on your fork. (Quite a few forks).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment