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

Autoloading in Rails initializer #2709

Closed
oskarpearson opened this issue Mar 21, 2023 · 5 comments · Fixed by #2726
Closed

Autoloading in Rails initializer #2709

oskarpearson opened this issue Mar 21, 2023 · 5 comments · Fixed by #2726
Assignees
Labels
community Was opened by a community member feature-request A request for a new feature or change to an existing one
Milestone

Comments

@oskarpearson
Copy link

Is your feature request related to a problem? Please describe.

In Rails 7 autoloading is disabled when an application boots. So things in the config/initalizers folder don't support autoloading.

However, the Datadog dd-trace-rb gem Rails integration takes a exception_controller parameter which needs to point at the class. Something like this:

Datadog.configure do |c|
  c.tracing.instrument :rails, exception_controller: ErrorsController

To use this, we need to jump through some hoops - like wrapping the whole of the datadog initialisation in Rails.application.reloader.to_prepare do (which I expect won't work anyway?), or the config.autoload_once_paths Rails feature. Using the latter (as I read it) means we can't autoload code when ErrorsController loads.

Describe the goal of the feature

I'd like to be able to pass a Symbol or string as the exception_controller parameter, rather than the actual class.

So the Datadog config would change to something like c.tracing.instrument :rails, exception_controller: :ErrorsController or potentially c.tracing.instrument :rails, exception_controller: "ErrorsController"

Describe alternatives you've considered

None

Additional context

https://github.com/DataDog/dd-trace-rb/blob/cb6f3a4ed44d41719570ed30ae8da19dd1f73c51/lib/datadog/tracing/contrib/action_pack/action_controller/instrumentation.rb would need to try and use constantize of the supplied parameter (currently it just returns false if it's not a class or module)

How does ddtrace help you?

N/A

@oskarpearson oskarpearson added community Was opened by a community member feature-request A request for a new feature or change to an existing one labels Mar 21, 2023
@TonyCTHsu TonyCTHsu self-assigned this Mar 22, 2023
@TonyCTHsu
Copy link
Contributor

TonyCTHsu commented Mar 22, 2023

👋 @oskarpearson , Thanks for sharing this. I think it make sense exception_controller should be a string instead of a constant. I guess the changes will not be difficult with constantize.

However, I would like learn more about the trouble of our current implementation with Rails 7 autoloading.

Is it broken in some ways we overlooked? or it is working as expected but having some kind of bad developer experience?

Could you provide me a bit more description about the current behaviour and what is causing trouble? So that I can search through the project to see with there are issues with similar patterns.

@jens
Copy link

jens commented Mar 23, 2023

For us the application fails to boot with a NameError uninitialized constant ErrorsController.
So using the exception_controller with Rails 7 doesn't work.

@oskarpearson
Copy link
Author

@TonyCTHsu Thanks for the followup here

The breakage is that using the suggested c.tracing.instrument :rails, exception_controller: ErrorsController syntax

  • Worked ok in Rails 5.
  • In Rails 6, we would get the warning DEPRECATION WARNING: Initialization autoloaded the constant ErrorsController.
  • In Rails 7 it won't work at all, and the application will fail to boot

For more detail, see
https://rubyonrails.org/2021/9/3/autoloading-in-rails-7-get-ready

You can replicate this easily - build a new Rails 7 application, add datadog to it, add an ErrorsController class, and then add a config as follows:

Datadog.configure do |c|
  c.tracing.instrument :rails, exception_controller: ErrorsController
end

On Rails 7 it will refuse to boot, and you will receive the error:

uninitialized constant ErrorsController (NameError)

  c.tracing.instrument :rails, exception_controller: ErrorsController
                                                     ^^^^^^^^^^^^^^^^

@marcotc
Copy link
Member

marcotc commented Mar 23, 2023

I suggest allowing a fully qualified class name String to be provided in the exception_controller: option.
The symbol approach won't work so well with namespaces.
We would still support passing the class directly for backwards compatibility.

@delner
Copy link
Contributor

delner commented Mar 24, 2023

What's the next step here @TonyCTHsu and @marcotc ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member feature-request A request for a new feature or change to an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants