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

Sidekiq 4.x scope side effects #416

Closed
ryansch opened this issue Apr 1, 2016 · 4 comments
Closed

Sidekiq 4.x scope side effects #416

ryansch opened this issue Apr 1, 2016 · 4 comments

Comments

@ryansch
Copy link

ryansch commented Apr 1, 2016

I was doing a code review of this gem before adoption and I noticed that there isn't any code to clear thread locals between sidekiq jobs. Since sidekiq 4.x (and maybe earlier?) reuse Processor threads to run more than one job (https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/processor.rb#L66-L68), if I use Rollbar.scope! to mutate the current scope I think there may be side effects for the next job. I would expect to see sidekiq middleware that calls Rollbar.reset_notifier! before calling yield.

Note that the easiest way to test this would be to run sidekiq with only one Processor thread.

@jondeandres
Copy link
Contributor

Hey @ryansch, thanks for creating the issue, we'll take a look at this. Will Rollbar.scoped help you with your issues? You can always create a new notifier with Rollbar.scope and use the returned object instead of using Rollbar.

@ryansch
Copy link
Author

ryansch commented Apr 1, 2016

We have a multi-tenant app. I'd much rather be able to create a piece of sidekiq server middleware to inject the current tenant information into the scope with Rollbar.scoped! instead of having to pass scope objects around. We have many workers that call different service objects and it's infeasible to change all that code.

We're evaluating rollbar while we're currently using honeybadger. With their stuff I can just call Honeybadger.context and it works great. I'm looking for the same experience here.

@jondeandres
Copy link
Contributor

Thank you very much for the information @ryansch, we'll try to fix this as soon as possible. We'll contact you in this issue.

Thanks!

@jondeandres
Copy link
Contributor

Hi @ryansch, we've fixed and closed this issue. Thank you very much for reporting this, since it was a real bug.

We hope you integration with Rollbar is successfuly, and don't doubt in open any other issue if you have questions, problems with it or you want to purpose something new, improvements, etc...

Thanks again.

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

No branches or pull requests

2 participants