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

Fix Rack middleware name patching #354

Merged
merged 9 commits into from
Mar 1, 2018

Conversation

delner
Copy link
Contributor

@delner delner commented Feb 23, 2018

Spans for Rack are sometimes getting bad resource names like #GET when middleware_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:

  1. Prevents the Rack patcher from prematurely initializing the Rails middleware stack, which could cause serious problems. (e.g. missing middleware if added via initializer.)
  2. Adds 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 remove use :rack if this was the only option they needed it for.
  3. If middleware_names is enabled, and a good resource name isn't available, it reverts to the old behavior instead of printing a bad resource.
  4. Only patch Rack middleware if the middleware_names option is true. So that if use :rack is called before the Rails application is finished initializing, it allows a subsequent use :rack, middleware_names: true call to do the actual patching at the correct time.
  5. If you want to use middleware_names with use :rack, you must also provide application. Helps prevent the Rack patcher from causing the bug in point 1. (Breaking change for anyone using use :rack, middleware_names. They'll need to add application, otherwise the option will have no effect.)

@delner delner added bug Involves a bug integrations Involves tracing integrations breaking-change Involves a breaking change labels Feb 23, 2018
@delner delner self-assigned this Feb 23, 2018
Copy link
Contributor

@palazzem palazzem left a 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.


require_relative 'middlewares'
@patched = true
if !@middleware_patched && get_option(:middleware_names) && get_option(:application)
Copy link
Contributor

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
Copy link
Contributor

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,
Copy link
Contributor

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
Copy link
Contributor

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.

@palazzem palazzem added this to the 0.12.0 milestone Mar 1, 2018
@delner delner force-pushed the bugfix/rack_middleware_name_patching branch from 7f7013a to b7e4d18 Compare March 1, 2018 19:24
@delner delner changed the base branch from master to 0.12-dev March 1, 2018 19:24
@delner delner merged commit 6ccb56f into 0.12-dev Mar 1, 2018
@palazzem palazzem deleted the bugfix/rack_middleware_name_patching branch April 4, 2018 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Involves a breaking change bug Involves a bug integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants