refactor sidekiq handler #197

jjb · 2015-01-08T05:36:31Z

DRYed up the code by moving the error handling code into its own method that can be reused
DRYed up the code by removing redundant invocations of Sidekiq.configure_server do |config|
moved PARAM_BLACKLIST out of the global namespace

(the first one is what inspired me to do the refactoring in the first place, because in my project i need to change the behavior and currently have to do a monkeypatch which replicates all the code. after this refactoring, i can use alias_method_chain)

let me know what you think!

/cc @dadadadave

brianr · 2015-01-08T05:46:56Z

Nice, thanks @jjb!

@jondeandres can you review this?

jondeandres · 2015-01-08T10:36:48Z

lib/rollbar/sidekiq.rb

 config.server_middleware do |chain|
- chain.add Rollbar::Sidekiq
+ chain.add Rollbar::SidekiqErrorHandlingMiddleware


I think you are expecting here to use Rollbar::SidekiqErrorHandler, which is the module name you've defined above.

jjb · 2015-01-08T14:55:36Z

@jondeandres 🤦 thanks for pointing out those obvious errors -- I was tired last night when I threw that together.

I've now fixed everything and also split it up into multiple commits.

There are no tests for this, perhaps we can write some before merging.

jjb · 2015-01-08T14:55:59Z

Are there tests for something else that I can look at to base tests for this off of?

jondeandres · 2015-01-08T15:59:39Z

@jjb cool 👍

you can write a test for this using the inline option in sidekiq, https://github.com/mperham/sidekiq/wiki/Testing. Just write a job that will crash, enqueue it, and check that the report was sent. Rollbar.last_report should return something after the report is sent.

I'll be on freenode as 'jandres' if you want to chat it.

jjb · 2015-01-09T18:20:11Z

@jondeandres thanks

seems like the block given to Sidekiq.configure_server does not get executed in the testing environment

i'm exploring why that might be... let me know if you have an idea

jjb · 2015-01-09T18:52:19Z

@jondeandres okay I figured out it's because of my sidekiq test environment -- I submitted this ticket, we'll see what @mperham says sidekiq/sidekiq#2128

jondeandres · 2015-01-09T18:59:32Z

it could because this?

https://github.com/mperham/sidekiq/blob/master/lib/sidekiq.rb#L53
https://github.com/mperham/sidekiq/blob/master/lib/sidekiq.rb#L67

require 'sidekiq/cli' and it should work.

But now I'm thinking that perhaps middlewares will not be executed in tests environments? I've never tested Sidekiq jobs, but probably Sidekiq will have this documented.

jjb · 2015-02-02T21:59:49Z

@jondeandres finally got around to finishing this up, take a look

jjb · 2015-02-02T22:01:44Z

when #209 is merged i'll rebase, for a tidier history on this one

jondeandres · 2015-02-02T22:35:35Z

Thanks @jjb. Tests are failing cause sidekiq is not supported in Ruby 1.8.7, take a look at our gemspec file, https://github.com/rollbar/rollbar-gem/blob/master/rollbar.gemspec#L25.

jjb · 2015-02-04T03:16:23Z

@jondeandres 1.8.6 stuff is fixed now.

any idea why this lone failure happened? https://travis-ci.org/rollbar/rollbar-gem/jobs/49406361

/home/travis/.rvm/gems/ruby-2.1.0/gems/genspec-0.2.7/lib/genspec.rb:8:in `<top (required)>': Unsupported Rails version: 4.0.4 (RuntimeError)
    from /home/travis/.rvm/gems/ruby-2.1.0/gems/activesupport-4.0.4/lib/active_support/dependencies.rb:229:in `require'
    from /home/travis/.rvm/gems/ruby-2.1.0/gems/activesupport-4.0.4/lib/active_support/dependencies.rb:229:in `block in require'

jondeandres · 2015-02-04T18:54:54Z

@jjb is because the version of genspec:

0.2.7 https://github.com/sinisterchipmunk/genspec/blob/v0.2.7/lib/genspec.rb
0.2.8 https://github.com/sinisterchipmunk/genspec/blob/v0.2.8/lib/genspec.rb

It seems to not be crashing in master, so probably is something in your branch. You can change gemfiles/rails40.gemfile and check that is solved:

gem 'genspec', '> 0.2.7'

jjb · 2015-02-04T21:02:04Z

@jondeandres gotcha. i increased the version for the whole project, no reason to not do this right?

(after it runs i'll check it to only be for rails 4 if you prefer that)

jondeandres · 2015-02-04T21:13:26Z

Nice!

One comment in specs. Can you replace this style in general?

let(:foo){:bar} to let(:foo) { :bar }

This covers the core functionality of the handler. Here are ways the tests could be further improved, which I deemed not worth the effort: 1. Testing if the handler is attached properly for Rails < 3 vs. >=3. I have tested it manually for the >=3 case and it works. 2. higher level functional test in the style of how DelayedJob is tested, where actual failing jobs are queued up: https://github.com/rollbar/rollbar-gem/blob/master/spec/rollbar/delayed_job_spec.rb I didn't do this because it's more difficult to do with Sidekiq, for reasons explored here: sidekiq/sidekiq#2128

jjb · 2015-02-05T20:10:07Z

@jondeandres all set

jondeandres · 2015-02-05T20:20:02Z

spec/rollbar/sidekiq_spec.rb

+ require 'rollbar/sidekiq'
+end
+
+describe "Sidekiq integration", :reconfigure_notifier => false do


This should be describe Rollbar::Sidekiq.

Then it will blow up under 1.8.7 because that class isn't loaded in lines 3-6 above

Humm! And can we use an string here then instead of a constant?

jjb · 2015-02-05T20:28:09Z

@jondeandres perhaps it will be more efficient for you to simply fork my branch and make the style changes you want? It's probably faster to make the changes directly than to explain to me the rationale.

jondeandres · 2015-02-05T20:33:13Z

@jjb yah! probably! I'll fork this and make a PR to your repo. I hope we can merge this today. Sorry for this style comments, it's just to maintain some order here :-D.

jjb · 2015-02-05T20:33:58Z

no problem, i enjoyed the process.

Style changes for tests.

jjb · 2015-02-05T21:52:26Z

@jondeandres merged

jondeandres · 2015-02-05T21:54:43Z

Nice! Thanks @jjb for this!

jjb · 2015-02-05T22:58:13Z

@jondeandres were you going to merge this one today?

refactor sidekiq handler

jondeandres · 2015-02-05T23:04:32Z

Merged, thanks @jjb!!

jjb · 2015-02-05T23:07:33Z

🍆

jondeandres reviewed Jan 8, 2015
View reviewed changes

jjb force-pushed the refactor-sidekiq-handler branch from 50c5c4f to c8eb84c Compare January 8, 2015 14:54

jjb force-pushed the refactor-sidekiq-handler branch from c8eb84c to e2e64e4 Compare January 8, 2015 14:59

jjb mentioned this pull request Jan 9, 2015

best practice for running configure_server block when testing? sidekiq/sidekiq#2128

Closed

jjb mentioned this pull request Feb 2, 2015

Style tweaks for sidekiq handler #209

Merged

jjb force-pushed the refactor-sidekiq-handler branch from e2e64e4 to 3ed7ca4 Compare February 2, 2015 20:31

jjb added 3 commits February 3, 2015 15:02

make error-handling code a reusable method

02c927b

refactor away redundant invocations of Sidekiq.configure_server

cc6abeb

move PARAM_BLACKLIST into sidekiq namespace

93fc5d8

jjb force-pushed the refactor-sidekiq-handler branch 4 times, most recently from c76588a to a95b4ea Compare February 4, 2015 02:37

jjb force-pushed the refactor-sidekiq-handler branch from f5bdaa2 to a75a1f4 Compare February 4, 2015 21:46

jjb added 2 commits February 4, 2015 17:08

increased required version of genspec

bcee4e8

jjb force-pushed the refactor-sidekiq-handler branch from a75a1f4 to bcee4e8 Compare February 4, 2015 22:08

jondeandres reviewed Feb 5, 2015
View reviewed changes

Jon de Andres and others added 2 commits February 5, 2015 22:20

Style changes for tests.

613a601

Merge pull request #1 from rollbar/sidekiq-refactor-tests

40418c4

Style changes for tests.

jondeandres added a commit that referenced this pull request Feb 5, 2015

Merge pull request #197 from jjb/refactor-sidekiq-handler

d1289b2

refactor sidekiq handler

jondeandres merged commit d1289b2 into rollbar:master Feb 5, 2015

jjb deleted the refactor-sidekiq-handler branch February 5, 2015 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor sidekiq handler #197

refactor sidekiq handler #197

jjb commented Jan 8, 2015

brianr commented Jan 8, 2015

jondeandres Jan 8, 2015

jjb commented Jan 8, 2015

jjb commented Jan 8, 2015

jondeandres commented Jan 8, 2015

jjb commented Jan 9, 2015

jjb commented Jan 9, 2015

jondeandres commented Jan 9, 2015

jjb commented Feb 2, 2015

jjb commented Feb 2, 2015

jondeandres commented Feb 2, 2015

jjb commented Feb 4, 2015

jondeandres commented Feb 4, 2015

jjb commented Feb 4, 2015

jondeandres commented Feb 4, 2015

jjb commented Feb 5, 2015

jondeandres Feb 5, 2015

jjb Feb 5, 2015

jondeandres Feb 5, 2015

jjb commented Feb 5, 2015

jondeandres commented Feb 5, 2015

jjb commented Feb 5, 2015

jjb commented Feb 5, 2015

jondeandres commented Feb 5, 2015

jjb commented Feb 5, 2015

jondeandres commented Feb 5, 2015

jjb commented Feb 5, 2015

refactor sidekiq handler #197

refactor sidekiq handler #197

Conversation

jjb commented Jan 8, 2015

brianr commented Jan 8, 2015

jondeandres Jan 8, 2015

Choose a reason for hiding this comment

jjb commented Jan 8, 2015

jjb commented Jan 8, 2015

jondeandres commented Jan 8, 2015

jjb commented Jan 9, 2015

jjb commented Jan 9, 2015

jondeandres commented Jan 9, 2015

jjb commented Feb 2, 2015

jjb commented Feb 2, 2015

jondeandres commented Feb 2, 2015

jjb commented Feb 4, 2015

jondeandres commented Feb 4, 2015

jjb commented Feb 4, 2015

jondeandres commented Feb 4, 2015

jjb commented Feb 5, 2015

jondeandres Feb 5, 2015

Choose a reason for hiding this comment

jjb Feb 5, 2015

Choose a reason for hiding this comment

jondeandres Feb 5, 2015

Choose a reason for hiding this comment

jjb commented Feb 5, 2015

jondeandres commented Feb 5, 2015

jjb commented Feb 5, 2015

jjb commented Feb 5, 2015

jondeandres commented Feb 5, 2015

jjb commented Feb 5, 2015

jondeandres commented Feb 5, 2015

jjb commented Feb 5, 2015