Add Rails middleware option #552
Conversation
delner
added
integrations
|
We were given a choice in this pull request on how users should enable Datadog tracing with Rails:
In this pull request I ultimately decided to purse option 2. My main motivations here were to avoid a breaking change (by requiring users to update their applications with a |
| @@ -39,7 +39,8 @@ def patch | |||
| @patched = true | |||
| end | |||
|
|
|||
| if !@middleware_patched && get_option(:middleware_names) | |||
| if (!instance_variable_defined?(:@middleware_patched) || !@middleware_patched) \ | |||
There was a problem hiding this comment.
This prevents a deprecation warning.
| # Add a callback hook to add the trace middleware before the application initializes. | ||
| # Otherwise the middleware stack will be frozen. | ||
| do_once(:rails_before_initialize_hook) do | ||
| ::ActiveSupport.on_load(:before_initialize) do |
There was a problem hiding this comment.
This is equivalent to the Railtie config.before_initialize call. Middleware previously was being added to the Rails global configuration. By putting it in this callback, we can make it load at the appropriate time instead. Has the additional benefit of making our Rails test app setup simpler.
|
|
||
| # Add a callback hook to finish configuring the tracer after the application is initialized. | ||
| # We need to wait for some things, like application name, middleware stack, etc. | ||
| do_once(:rails_after_initialize_hook) do |
There was a problem hiding this comment.
This is equivalent to the Railtie config.after_initialize call. This is basically what we had before in the Railtie.
| # Add instrumentation to Rails components | ||
| Datadog::Contrib::Rails::ActionController.instrument | ||
| Datadog::Contrib::Rails::ActionView.instrument | ||
| Datadog::Contrib::Rails::ActiveSupport.instrument |
There was a problem hiding this comment.
Note: its important we do this after_initialize because it depends on the application to be loaded to properly instrument.
| @@ -49,5 +87,3 @@ def compatible? | |||
| end | |||
| end | |||
| end | |||
|
|
|||
| require 'ddtrace/contrib/rails/railtie' if Datadog.registry[:rails].compatible? | |||
There was a problem hiding this comment.
Removing this line effectively nullifies the Railtie, and also prevents the instrumentation from forcefully loading. Very important.
| # it catches and swallows the error. If it's too far after, custom middleware can find itself | ||
| # between, and raise exceptions that don't end up getting tagged on the request properly (e.g lost stack trace.) | ||
| config.app_middleware.insert_after(ActionDispatch::ShowExceptions, Datadog::Contrib::Rails::ExceptionMiddleware) | ||
| # Add the trace middleware to the application stack |
There was a problem hiding this comment.
Updated the Railtie to use proper callbacks. Users could use require 'ddtrace/contrib/rails/railtie' directly if they wanted, but shouldn't have to.
| @@ -113,16 +112,10 @@ def draw_test_routes!(mapper) | |||
| def reset_rails_configuration! | |||
| Rails.class_variable_set(:@@application, nil) | |||
| Rails::Application.class_variable_set(:@@instance, nil) | |||
| Rails::Railtie::Configuration.class_variable_set(:@@app_middleware, app_middleware) | |||
| if Rails::Railtie::Configuration.class_variable_defined?(:@@app_middleware) | |||
| Rails::Railtie::Configuration.class_variable_set(:@@app_middleware, Rails::Configuration::MiddlewareStackProxy.new) | |||
There was a problem hiding this comment.
This used to carry over the trace middleware on the global constant in order to preserve it between tests. But now that middleware configuration is wrapped in a proper callback, we can now reset the middleware completely and allow the tests to re-add it more naturally.
|
Awesome Stuff! Thanks for looking into it - not autoloading rails instrumentation is the way to go! |
| # Add a callback hook to add the trace middleware before the application initializes. | ||
| # Otherwise the middleware stack will be frozen. | ||
| do_once(:rails_before_initialize_hook) do | ||
| ::ActiveSupport.on_load(:before_initialize) do |
As per #149, Rails instrumentation is auto-injecting a Railtie and trace middleware into the Rails application by virtue of loading. This means that it isn't really possible to disable trace middleware in this case.
This pull request seeks to make Rails instrumentation optional, by:
middlewareoption, which defaults totrue. When set to false, trace middleware will not be added to the application.This would be good to have for users who want to use instrumentation in Rails apps without instrumenting Rails, and for OpenTracing users who'd want to prevent conflicting middleware from running in their Rails applications.