-
Notifications
You must be signed in to change notification settings - Fork 375
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
Fix Rack middleware name patching #354
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This improvement looks good to me. Let's keep it on-hold so that we can test it properly and provide the deprecation warning.
lib/ddtrace/contrib/rack/patcher.rb
Outdated
|
||
require_relative 'middlewares' | ||
@patched = true | ||
if !@middleware_patched && get_option(:middleware_names) && get_option(:application) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds great, maybe we should add a deprecation warning with a condition such as:
if get_option(:middleware_names) && !get_option(:application)
log_deprecation_warning('you should pass the application or use Rails') # change it with a proper message
end
end | ||
|
||
def patched? | ||
@patched ||= false | ||
end | ||
|
||
def enable_middleware_names | ||
root = get_option(:application) || rails_app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -28,7 +28,9 @@ def self.setup | |||
Datadog.configuration.use( | |||
:rack, | |||
tracer: tracer, | |||
application: ::Rails.application, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since it's already initialized, this one works great
@@ -10,6 +10,7 @@ module Patcher | |||
option :controller_service | |||
option :cache_service | |||
option :database_service | |||
option :middleware_names, default: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's keep these functionalities disabled by default; we'll revisit it before going 1.0.
… supplied as an option.
7f7013a
to
b7e4d18
Compare
Spans for Rack are sometimes getting bad resource names like
#GET
whenmiddleware_names
is enabled and, somehow, the Rack middleware doesn't get properly patched.This pull request changes a few things in an attempt to address this:
middleware_names
option to Rails, so Rails can properly patch Rack after the Rails application is fully initialized, when the middleware will no longer change. This change also allows users to removeuse :rack
if this was the only option they needed it for.middleware_names
is enabled, and a good resource name isn't available, it reverts to the old behavior instead of printing a bad resource.middleware_names
option is true. So that ifuse :rack
is called before the Rails application is finished initializing, it allows a subsequentuse :rack, middleware_names: true
call to do the actual patching at the correct time.middleware_names
withuse :rack
, you must also provideapplication
. Helps prevent the Rack patcher from causing the bug in point 1. (Breaking change for anyone usinguse :rack, middleware_names
. They'll need to addapplication
, otherwise the option will have no effect.)