Skip to content
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

Add logging #99

Merged
merged 16 commits into from
Apr 29, 2014
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,5 @@ stdout
tags
tags.*
*.gem
.ruby-version
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

.ruby-gemset
28 changes: 28 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,34 @@ Or you can define the entire key by overriding `redis_retry_key`.
end
end

### Debugging

By default, resque-retry doesn't log anything, however if you are having trouble understanding why your jobs
are, or are not, retrying, you can turn logging on.

1. Ensure that `VERBOSE` or `VVERBOSE` are set in your environment. This will enable log formatters that generate log output.
2. Ensure that `RESQUE_RETRY_LOGGING` is set to "true" in your environment. This will enable log messages within resque-retry to be sent to Resque's logger
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this, further details as to the why below.


This will show what resque-retry is doing, and when, as well as your job args and the exception that initiated the retry process.

Example output:

```
** [09:09:38 2014-04-24] 86961: got: (Job{fire_item_event} | FireItemEventJob | [-2000, "foobar"])
** [09:09:38 2014-04-24] 86961: Running before_fork hooks with [(Job{fire_item_event} | FireItemEventJob | [-2000, "foobar"])]
** [09:09:38 2014-04-24] 87285: Running after_fork hooks with [(Job{fire_item_event} | FireItemEventJob | [-2000, "foobar"])]
** [09:09:38 2014-04-24] 87285: [-2000, "foobar"]: before_perform_retry
** [09:09:38 2014-04-24] 87285: [-2000, "foobar"]: Attempt 11 set in Redis
** [09:09:39 2014-04-24] 87285: [-2000, "foobar"] [ActiveRecord::RecordNotFound/No Foobar with id -2000]: on_failure_retry
** [09:09:39 2014-04-24] 87285: [-2000, "foobar"] [ActiveRecord::RecordNotFound/No Foobar with id -2000]: Retrying based on exception
** [09:09:39 2014-04-24] 87285: [-2000, "foobar"] [ActiveRecord::RecordNotFound/No Foobar with id -2000]: NOT Retrying based on criteria
** [09:09:39 2014-04-24] 87285: [-2000, "foobar"] [ActiveRecord::RecordNotFound/No Foobar with id -2000]: retry criteria valid, trying again
** [09:09:39 2014-04-24] 87285: [-2000, "foobar"] [ActiveRecord::RecordNotFound/No Foobar with id -2000]: trying again
** [09:09:39 2014-04-24] 87285: [-2000, "foobar"] [ActiveRecord::RecordNotFound/No Foobar with id -2000]: retry delay: 5 for queue FireItemEventJob
** [09:09:39 2014-04-24] 87285: (Job{fire_item_event} | FireItemEventJob | [-2000, "foobar"]) failed: #<ActiveRecord::RecordNotFound: No Foobar with id -2000>
```


Contributing/Pull Requests
--------------------------

Expand Down
54 changes: 47 additions & 7 deletions lib/resque/plugins/retry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -230,17 +230,27 @@ def retry_exceptions
# @api public
def retry_criteria_valid?(exception, *args)
# if the retry limit was reached, dont bother checking anything else.
return false if retry_limit_reached?
if retry_limit_reached?
log 'retry limit reached',args,exception
return false
end

# We always want to retry if the exception matches.
should_retry = retry_exception?(exception)
retry_based_on_exception = retry_exception?(exception)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this new-line

log "#{retry_based_on_exception ? 'Retrying' : 'NOT Retrying'} based on exception",args,exception

# call user retry criteria check blocks.
retry_criteria_checks.each do |criteria_check|
should_retry ||= !!instance_exec(exception, *args, &criteria_check)
retry_based_on_criteria = false
unless retry_based_on_exception
# call user retry criteria check blocks.
retry_criteria_checks.each do |criteria_check|
retry_based_on_criteria ||= !!instance_exec(exception, *args, &criteria_check)
end
end

should_retry
log "#{retry_based_on_criteria ? 'Retrying' : 'NOT Retrying'} based on criteria",args,exception

retry_based_on_exception || retry_based_on_criteria
end

# Retry criteria checks
Expand Down Expand Up @@ -297,11 +307,14 @@ def retry_criteria_check(&block)
#
# @api private
def try_again(exception, *args)
log "trying again",args,exception
# some plugins define retry_delay and have it take no arguments, so rather than break those,
# we'll just check here to see whether it takes the additional exception class argument or not
temp_retry_delay = ([-1, 1].include?(method(:retry_delay).arity) ? retry_delay(exception.class) : retry_delay)

retry_in_queue = retry_job_delegate ? retry_job_delegate : self
log "retry delay: #{temp_retry_delay} for queue #{retry_in_queue}", args,exception

if temp_retry_delay <= 0
# If the delay is 0, no point passing it through the scheduler
Resque.enqueue(retry_in_queue, *args_for_retry(*args))
Expand All @@ -322,12 +335,15 @@ def try_again(exception, *args)
#
# @api private
def before_perform_retry(*args)
log 'before_perform_retry', args
@on_failure_retry_hook_already_called = false

# store number of retry attempts.
retry_key = redis_retry_key(*args)
Resque.redis.setnx(retry_key, -1) # default to -1 if not set.
@retry_attempt = Resque.redis.incr(retry_key) # increment by 1.
log "attempt #{@retry_attempt} set in Redis", args
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a colon after "attempt"

@retry_attempt
end

# Resque after_perform hook
Expand All @@ -336,6 +352,7 @@ def before_perform_retry(*args)
#
# @api private
def after_perform_retry(*args)
log 'after_perform_retry, clearing retry key', args
clean_retry_key(*args)
end

Expand All @@ -350,11 +367,16 @@ def after_perform_retry(*args)
#
# @api private
def on_failure_retry(exception, *args)
return if @on_failure_retry_hook_already_called
log "on_failure_retry", args,exception
if @on_failure_retry_hook_already_called
log 'on_failure_retry_hook_already_called', args, exception
return
end

if retry_criteria_valid?(exception, *args)
try_again(exception, *args)
else
log 'retry criteria not sufficient for retry', args, exception
clean_retry_key(*args)
end

Expand All @@ -381,9 +403,27 @@ class << self; self end.class_eval{ undef_method(mname) } rescue nil
#
# @api private
def clean_retry_key(*args)
log "Clearing retry key",args
Resque.redis.del(redis_retry_key(*args))
end

# Log messages through the Resque logger. Generally not for appication
# logging-just for interworkings of Resque and plugins.
#
# Requires that RESQUE_RETRY_LOGGING be set to the string "true" in the environment.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this (and the following new-line).

#
# message:: message to log
# args:: args of the resque job in context
# exception:: the exception that might be causing a retry
def log(message, args, exception=nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few notes about this:

  • I didn't want to include Resque::Logging as a) we don't need the different severity levels and b) those method names are super-common and would squash any jobs that do something similar (like my jobs :)
  • Checking for Resque.logger is mirroring what's in Resque::Worker - presumably there is a good reason, but I guess generally logging should not take down your system
  • The environment variable is there so that a released version of this library would behave the same way, just in case the addition of logging causes some problem. This means people can opt-in, and they can opt-in on a per-worker basis, which seems handy. The value of "true" is debatable. I prefer it, since ENV can only store strings, but I know some people like 1. If someone feel strongly, we can change it.

# Resque::Worker will handle interpeting LOGGER, VERBOSE, and VVERBOSE
# since everyone is sharing Resque.logger.
if Resque.logger && ENV['RESQUE_RETRY_LOGGING'] == 'true'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned below, let's just check if Resque.logger exists here (and remove the env-var check entirely -- further details below).

exception_portion = exception.nil? ? '' : " [#{exception.class}/#{exception}]"
message = "#{args.inspect}#{exception_portion}: #{message}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just combine this line with the following:

Resque.logger.info "#{args.inspect}#{exception_portion}: #{message}"

Resque.logger.info message if Resque.logger
end
end
end
end
end
3 changes: 3 additions & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
`redis-server #{dir}/redis-test.conf`
Resque.redis = '127.0.0.1:9736'

# Ensure that log message code is executed and doesn't blow up.
ENV['RESQUE_RETRY_LOGGING'] = 'true'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more that I think about it, you have a good point (sorry for not realizing it earlier!). Let's remove the env-var entirely. let's always log and let what is output be controlled entirely by VERBOSE and VVERBOSE in the "Resque" internals.


Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default Resque.logger seems to go into the bit bucket, but the logging calls do get made, so this seemed sufficient to make sure logging doesn't cause anything to blow up, but without littering test output with log messages.

It didn't seem worth it to test what was being logged—I find that makes tests brittle, but if someone feels strongly, I'm happy to add some tests around it.

# Test helpers
class MiniTest::Unit::TestCase
def perform_next_job(worker, &block)
Expand Down