Add logging #99
Add logging #99
Conversation
I like gemsets, but that doesn't mean anyone else should...
| # 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) |
There was a problem hiding this comment.
A few notes about this:
- I didn't want to
include Resque::Loggingas 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.loggeris mirroring what's inResque::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, sinceENVcan only store strings, but I know some people like1. If someone feel strongly, we can change it.
|
I think I fixed all the invocation syntax locations (at least according to my greps :) |
|
OK, I'm gonna wait for a few minutes before sending up diffs while you are still commenting. I realize that is making things confusing. |
| @@ -12,3 +12,5 @@ stdout | |||
| tags | |||
| tags.* | |||
| *.gem | |||
| .ruby-version | |||
|
@davetron5000 sorry for the litany of comments/requests. I think we're really close now. Please give it a once over and let's get this merged! |
|
Trying to get caught up here and not entirely sure I follow. Did you decide to go with a specific |
| @@ -40,6 +46,8 @@ def save | |||
| } | |||
|
|
|||
| Resque.redis.setex(failure_key, 2*retry_delay, Resque.encode(data)) | |||
| else | |||
| log "retry_delay: #{retry_delay} <= 0 - ignoring", payload, exception | |||
There was a problem hiding this comment.
Again, not sure if this captures the intent of doing nothing for this case. Obviously I can use this message to see what happened, but if there's a better way to state what this case is please let me know and I can make a better log message
|
Even if there are no further code review comments, I'd like to wait on this until I've run it in production with the new changes I added. |
@jzaleski suggested to just remove the extra env var and allow |
|
@lantins we decided to remove the additional environment variable and rely on "VERBOSE" and "VVERBOSE" that way anyone using the plugin will get it for free when either of the env-vars is set. |
Thisi makes our logging totally independent of whatever logging methods are provided by Resque. We use Resque.logger underneath, but the plugin log statements now go through on interface that we control
The log message in question needs to distinguish between "job is not retriable" and "job IS retriable, but isn't actually being retried right now". We also don't want to go to Redis more than is required, so intermediate variables to the rescue. Also happens to straighten out that double-negative, DeMorgan's Law-requiring `if` statement expression.
| @@ -82,6 +93,10 @@ def retrying? | |||
| def cleanup_retry_failure_log! | |||
| Resque.redis.del(failure_key) if retryable? | |||
| end | |||
|
|
|||
| def args_from(payload) | |||
There was a problem hiding this comment.
Just being super paranoid about what's in payload - log messages shouldn't crash the system
There was a problem hiding this comment.
That's a good idea. Thank you
|
This has been running in production for me for several days without incident and helped me figure out #97. I'm good with this if everyone else is, but still willing to make whatever other changes we want. |
|
I've not fully reviewed it as @jzaleski seems to be on the case and I've had some other things to deal with! Will be happy to see it in a new release, thanks for reporting back @davetron5000 :) |
| module Plugins | ||
| module Retry | ||
| module Logging | ||
| # Log messages through the Resque logger. Generally not for appication |
There was a problem hiding this comment.
Very minor change.. "application" is spelled incorrectly here.
|
@davetron5000 you did a great job on this! Thank you very much for your patience. Just a couple more minor cosmetic things to do to get this one over the finish line. As soon as this is in I plan to revisit #97 Thank you for all the hard work! Also, if possible, please see if you can get this ready to auto-merge.. If you have any difficulty I will handle it from the command-line. Thanks! |
|
Cool. Made those two changes. I'm not sure why it's not showing the Big Green Merge Button™—I fetched the latest from this repo and a merge gives me "Already up-to-date". Maybe something got borked cross-repo? I usually do this from within a repo where I can commit… |
|
Try this:
Resolve any conflicts and repush |
Conflicts: lib/resque/plugins/retry.rb
|
Despite having fetched this repo, I was typing |
|
Looks great @davetron5000. Travis just passed.. Merging! |
- Log to DEBUG rather then INFO level to reduce noise during operation. - Adjust yardoc comment style to match other sections of the repo. - Remove 1 level of indentation. Ref: #99
Per the discussion in #97, this adds a log of logging to what's going on. It does so in a conservative way, in that no behavior will change without taking extra steps. Will comment on a few specific things in the pull request.