appsignal / appsignal-ruby

🟥 AppSignal for Ruby gem
https://www.appsignal.com/ruby
MIT License
183 stars 116 forks source link

Sinatra apps mounted in Rails don't set the correct root_path #149

Closed tombruijn closed 3 years ago

tombruijn commented 8 years ago

Problem found in our test-setups repo in the Rails application.

Sinatra will return the config directory as the root directory if mounted in a Rails application: https://github.com/appsignal/appsignal-ruby/blob/b11b9478b082b8c98b33b7caaeb4eeed0081ab77/lib/appsignal/integrations/sinatra.rb#L8

$ app_settings.root
# => "/Users/tom/appsignal/tests/applications/rails/config"

Not sure why it thinks the config dir is its root dir.

This causes the Sinatra integration to not load in the Sinatra application.

Workaround would be to set the root directory in the sinatra application manually.

thijsc commented 8 years ago

Wasn't this fixed @tombruijn?

tombruijn commented 8 years ago

@thijsc you mean this commit https://github.com/appsignal/appsignal-ruby/commit/bfa920b8a65b1edb45379057750c86ddbb3de834 on PR https://github.com/appsignal/appsignal-ruby-private/pull/297 ?

It's similar, but unrelated. That happens when you're only using Sinatra in modular apps. What I'm referring to is the path that sinatra deduces when it's mounted in a (Rack/)Rails app.

tombruijn commented 8 years ago

Running into this problem again, debugging something else..

Sinatra bases the root path of its config on the app_file: https://github.com/sinatra/sinatra/blob/14db1440b53f8b21519b3674a0157d680eb72dc7/lib/sinatra/base.rb#L1882-L1883

Which if you track back far enough comes from here: https://github.com/sinatra/sinatra/blob/14db1440b53f8b21519b3674a0157d680eb72dc7/lib/sinatra/base.rb#L1797-L1801

So depending on where you require the sinatra file in the rails application the root path is different.

# config/application.rb
app_settings.app_file
# => "/Users/tom/appsignal/tests/applications/rails/config/application.rb"
app_settings.root
# => "/Users/tom/appsignal/tests/applications/rails/config"
# config.ru
app_settings.root
# => "/Users/tom/appsignal/tests/applications/rails/lib"
app_settings.app_file
# => "/Users/tom/appsignal/tests/applications/rails/lib/sinatra_app.rb"
wesoudshoorn commented 7 years ago

@tombruijn can you give your opinion on wether we should fix this issue or ignore because it's a rare occurrence?

tombruijn commented 7 years ago

It's quite an rare issue. It's only for Sinatra apps that are monitored by AppSignal while being mounted in a Rails app. I don't actually know if we can reliably detect this. We can see if Rails is active and use the Rails.root path instead for the Sinatra app, but is that always what a user would want?

Or we add this setup to the documentation and describe the issue there and how to set the root path of the Sinatra app to get the desired behavior.

I'll take another look on Wednesday and try a Rails.root path or otherwise document it in the docs.

Edit: Didn't get around to it on Wednesday. Maybe next time. But regardless, we need to at least document this.

thijsc commented 7 years ago

I'm wondering if we should do something with this. We could drop down one level if Rails is loaded and the root path starts with config?