Handled sinatra errors being reported to Appsignal about appsignal-ruby HOT 15 CLOSED

Thanks for the report @derekkraan! I'm doing some research on this later this week and will get back to you.

from appsignal-ruby.

I take back the part where users should enable raise_errors. The correct approach (again, IMO) is to add Appsignal.add_exception(env['sinatra.errors']) to your error handler that shows your 500 page.

from appsignal-ruby.

Also, on a minor note, I think your sinatra instructions are out of date in your app. It still says to include this line: use Appsignal::Rack::Listener, but Appsignal::Rack::Listener disappeared somewhere pre-1.0.

from appsignal-ruby.

I'm not sure how to handle this issue yet. I agree with you that encouraging raise_errors is probably a better approach.

The problem is that I have no way of knowing which customers rely on the existing behaviour. So if we remove this line error reporting might stop when they upgrade without them noticing. I'm thinking we could introduce a config option to disable this. Would that work for you?

from appsignal-ruby.

Actually, I think raise_errors is not the way to go. With Rails, the problem is easy to solve: you put your error-reporting middleware somewhere underneath the Rails error page middleware. Sinatra doesn't handle error pages with middleware: error pages are handled within the app, before you go back up the middleware stack. So there is no "after the error is generated, but before the error is caught by the error pages" place in the middleware stack to put your middleware.

Therefore, the most correct way (IMO) is for users to add Appsignal.add_exception(env['sinatra.errors']) to their error page handler and for the gem to do no further detection.

I suspect that all your Sinatra users are reliant on the current behaviour of the gem. Otherwise, they are silently using a monkeypatched version of your gem, which seems unlikely. There is apparently a way to print a message on the screen when the user installs or upgrades a gem (at least, I see many gems doing this, so it must be possible).

Anyways, a config option would work for us. It's certainly better than maintaining a monkeypatched version of the gem.

from appsignal-ruby.

I've finally had some time today to dive into this. It turns out the raise_errors setting is accessible from the middleware. Could you take a look at #111? Does that fix the issue for you?

from appsignal-ruby.

I don't think so. In our setup, we have raise_errors turned off, which means that we use sinatra's error handling mechanism to show error pages when an error occurs. So Sinatra does not propagate exceptions up through to the middleware (except through env['sinatra.error']).

This is pretty typical of a Sinatra app, I think, which is why you guys have added the line
transaction.set_error(env['sinatra.error']) if env['sinatra.error']
in the ensure block of your error-reporting middleware.

The problem with this approach is that Sinatra never clears env['sinatra.error'] even when the error is handled. In fact, if the user has raise_errors turned off, there is no way for you to know in the middleware whether a particular error has been handled or not by the application.

The main architectural issue (at least, the reason why this approach does work in Rails but not in Sinatra) is that Rails does error pages in a middleware (so you can put your middleware between the app and the error pages middleware), but Sinatra does error handling (if raise_errors is false) in the app, so there is no way for you to get your middleware between the app and the error pages.

That's why I suggested that your users might have to add Appsignal.add_exception(env['sinatra.errors']) to their error page handler when applicable (we have this line in our "500" error handler but not in our "404" error handler for example).

The other possible solution for us would be to turn on raise_errors, extract our error_pages handler to a middleware and insert that higher in the middleware stack than the AppSignal middleware. In that case, your fix would be sufficient.

from appsignal-ruby.

I understand the issue you're describing, I just don't have a clear fix for it at the moment. We cannot change the current behaviour since I have no way of knowing who is relying on the current behaviour and we would completely break their error reporting next time they do a bundle update.

I actually got the idea for this fix from the last paragraph of the original issue text. Did anything change about this possible solution in the meantime?

Maybe a dumb question: But why don't you just clear env['sinatra.errors'] in your error handler?

from appsignal-ruby.

I agree, there is no obvious good fix. To be clear: I think your fix is an improvement (for those with raise_errors enabled), but with raise_errors disabled, it still won't work properly.

We also tried clearing env['sinatra.errors'] in our error handler, so it's definitely not a dumb question. This is already a while ago, so I don't remember the details completely, but apparently Sinatra doesn't like it when you do that and will run some error handlers twice.

I could look at this again, but I think what we will do when you release a version of the gem with this fix in it is to enable raise_errors, extract our error handlers to a middleware, and place that above the AppSignal middleware in the stack. Then at least we will have a workable solution with the stock Appsignal gem.

from appsignal-ruby.

There is one other solution: We could add unless env['sinatra.skip_appsignal_error'] to the ensure block. You could then set this to true to skip the error. Would that be a better solution for you?

from appsignal-ruby.

This would help us out, yes. Then we could go back to the vanilla appsignal gem.

from appsignal-ruby.

@derekkraan I've implemented this mechanism, does this work for you?

from appsignal-ruby.

@thijsc yes, this will work for us. Thanks!

from appsignal-ruby.

This has now been released in 1.2.1.beta.2. Could you give it a try @derekkraan?

from appsignal-ruby.

This has now been released in 1.2.1.

from appsignal-ruby.