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

RackRequest - don't allow it to crash on bad URLs and only load Sidekiq middleware if running under Sidekiq server #289

Closed
wants to merge 5 commits into from

Conversation

perldork
Copy link
Contributor

@perldork perldork commented Apr 3, 2016

Discovered while using Bugsnag with Rails:

  • If the Sidekiq middleware is loaded under Rails, Bugsnag will crash when any error is found during middleware runs, causing the sent over stack trace to not have any custom nor app specific meta-data (for us this meant Rails3, RackRequest, Warden). Use the Sidekiq Sidekiq.server? test to ensure the middleware is only loaded when running under a Sidekiq process.
  • If an improperly formatted URL is sent to a Rails app and it causes a crash, the RackRequest middlleware will crash as well when it attempts to clean the URL's parameters. Add a begin/rescue statement to keep that from happening so that Rack meta-data still gets sent.

Initially applied to 3.0.0 (version we are running), same patches also applied cleanly to master HEAD.

@kattrali
Copy link
Contributor

kattrali commented Apr 4, 2016

Thank you for the contribution, @perldork. I merged and squashed this as 5cfae87 externally since there was a previous PR for handling the Sidekiq middleware loading.

@kattrali kattrali closed this Apr 4, 2016
@perldork
Copy link
Contributor Author

perldork commented Apr 4, 2016

Delisa,

It was the use statement for Bugsnag::Sidekiq that was causing Bugsnag to
crash, will the previous patch resolve that? i see the use statement is
still i. sidekiq.rb and not wrapped in the CLI conditonal .. my suspicion
that is that if i use that gersion i will get crashes again vs having the
use Bugsnag middleware statement in the CLI conditional in sidekiq.rb

On Monday, April 4, 2016, Delisa Mason notifications@github.com wrote:

Closed #289 #289.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#289 (comment)

@kattrali
Copy link
Contributor

kattrali commented Apr 5, 2016

@perldork, the new patch adds a dependency check for Sidekiq, which handles correctly setting app type. This is the same way Bugsnag itself is using the library in production as well as some others. Maybe the crash is unrelated?

@perldork
Copy link
Contributor Author

perldork commented Apr 5, 2016

@kattrali - could be, I will try out the 4.0.0 version in the near future and see what happens - if there are issues i'll see about a patch and open a new PR. thanks for your response.

@kattrali
Copy link
Contributor

kattrali commented Apr 5, 2016

Sounds good, let me know how it turns out!

@aquach
Copy link

aquach commented Oct 17, 2016

I'm seeing this behavior where API requests are logged with type "sidekiq". To the best of my understanding, bugsnag-ruby loads all the integrations it can find while swallowing load errors, assuming that only the integrations that you've installed will be loaded. But on my Sinatra server, I do need Sidekiq in order to queue Sidekiq jobs. The way I'm reading the code suggests that there's nothing preventing Bugsnag from setting app_type to "sidekiq" on my Sinatra server, when really it should be "rack". Is there something I'm missing here? Thanks for your help!

@aquach
Copy link

aquach commented Oct 17, 2016

My apologies, I figured it out :)

@kattrali
Copy link
Contributor

Ah np, @aquach!

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

Successfully merging this pull request may close these issues.

3 participants