Permalink
Browse files

Merge pull request #1021 from ruby-agent/RUBY-1407-urlmap-initialize

RUBY-1407 Revised Rack::URLMap instrumentation
  • Loading branch information...
mwear committed Mar 18, 2015
2 parents 3ee3553 + be6c7c5 commit 15d418338da543ff2717d7c2d07705e2e844fd1a
View
@@ -2,6 +2,12 @@
## v3.11.1 ##
+ * Better naming for Rack::URLMap
+
+ If a Rack app made direct use of Rack::URLMap, instrumentation would miss
+ out on using the clearest naming based on the app class. This has been
+ fixed.
+
* Avoid performance regression in makara database adapter
Delegation in the makara database adapter caused performance issues when the
@@ -1053,6 +1053,13 @@ def self.convert_to_list(value)
:dynamic_name => true,
:description => 'Defines whether the agent will hook into Rack::Builder\'s <code>to_app</code> method to find gems to instrument during application startup.'
},
+ :disable_rack_urlmap => {
+ :default => false,
+ :public => true,
+ :type => Boolean,
+ :dynamic_name => true,
+ :description => 'Defines whether the agent will hook into Rack::URLMap to install middleware tracing.'
+ },
:disable_rubyprof => {
:default => false,
:public => true,
@@ -114,6 +114,15 @@ def to_app_with_newrelic_deferred_dependency_detection
result
end
end
+
+ module RackURLMap
+ def self.generate_traced_map(map)
+ map.inject({}) do |traced_map, (url, handler)|
+ traced_map[url] = NewRelic::Agent::Instrumentation::MiddlewareProxy.wrap(handler, true)
+ traced_map
+ end
+ end
+ end
end
end
end
@@ -148,6 +157,30 @@ class << self
alias_method :use, :use_with_newrelic
end
end
+
end
end
+DependencyDetection.defer do
+ named :rack_urlmap
+
+ depends_on do
+ defined?(::Rack) && defined?(::Rack::URLMap)
+ end
+
+ depends_on do
+ ::NewRelic::Agent::Instrumentation::RackHelpers.middleware_instrumentation_enabled? &&
+ !::NewRelic::Agent.config[:disable_rack]
+ end
+
+ executes do
+ class ::Rack::URLMap
+ alias_method :initialize_without_newrelic, :initialize
+
+ def initialize(map = {})
+ traced_map = ::NewRelic::Agent::Instrumentation::RackURLMap.generate_traced_map(map)
+ initialize_without_newrelic(traced_map)
+ end
+ end
+ end
+end
@@ -1,3 +1,8 @@
+gemfile <<-RB
+ gem 'rack', '~>1.6.0'
+ gem 'rack-test'
+RB
+
gemfile <<-RB
gem 'rack', '~>1.5.0'
gem 'rack-test'
@@ -8,6 +13,11 @@ gemfile <<-RB
gem 'rack-test'
RB
+gemfile <<-RB
+ gem 'rack', '1.3.10'
+ gem 'rack-test'
+RB
+
gemfile <<-RB
gem 'rack', '1.2.8'
gem 'rack-test'
@@ -22,4 +32,4 @@ RB
gemfile <<-RB
gem 'rack', '1.0.1'
gem 'rack-test'
-RB
+RB
@@ -0,0 +1,128 @@
+# encoding: utf-8
+# This file is distributed under New Relic's license terms.
+# See https://github.com/newrelic/rpm/blob/master/LICENSE for complete details.
+
+# These tests confirm functionality when using the Rack::Builder class. In
+# particular, combinations of map, use and run should result in the right
+# metrics. In internals changed across Rack versions, so it's important to
+# check as our middleware and Rack instrumentation has grown.
+
+require 'multiverse_helpers'
+
+if NewRelic::Agent::Instrumentation::RackHelpers.rack_version_supported?
+
+class BuilderMapTest < Minitest::Test
+ include MultiverseHelpers
+
+ setup_and_teardown_agent
+
+ include Rack::Test::Methods
+
+ class SimpleMiddleware
+ def initialize(app)
+ @app = app
+ end
+
+ def call(env)
+ @app.call(env)
+ end
+ end
+
+ class MiddlewareOne < SimpleMiddleware; end
+ class MiddlewareTwo < SimpleMiddleware; end
+ class MiddlewareThree < SimpleMiddleware; end
+
+ class ExampleApp
+ def call(env)
+ [200, {}, [self.class.name]]
+ end
+ end
+
+ class PrefixAppOne < ExampleApp; end
+ class PrefixAppTwo < ExampleApp; end
+
+ def app
+ Rack::Builder.app do
+ use MiddlewareOne
+ use MiddlewareTwo
+
+ map '/prefix1' do
+ run PrefixAppOne.new
+ end
+
+ map '/prefix2' do
+ use MiddlewareThree
+ run PrefixAppTwo.new
+ end
+
+ # Rack versions prior to 1.4 did not support combining map and run at the
+ # top-level in the same Rack::Builder.
+ if Rack::VERSION[1] >= 4
+ run ExampleApp.new
+ end
+ end
+ end
+
+ if Rack::VERSION[1] >= 4
+ def test_metrics_for_default_prefix
+ get '/'
+
+ assert_metrics_recorded_exclusive([
+ 'Apdex',
+ 'ApdexAll',
+ 'HttpDispatcher',
+ 'Middleware/all',
+ 'Controller/Rack/BuilderMapTest::ExampleApp/call',
+ 'Apdex/Rack/BuilderMapTest::ExampleApp/call',
+ 'Middleware/Rack/BuilderMapTest::MiddlewareOne/call',
+ 'Middleware/Rack/BuilderMapTest::MiddlewareTwo/call',
+ 'Nested/Controller/Rack/BuilderMapTest::ExampleApp/call',
+ ['Middleware/Rack/BuilderMapTest::MiddlewareOne/call', 'Controller/Rack/BuilderMapTest::ExampleApp/call'],
+ ['Middleware/Rack/BuilderMapTest::MiddlewareTwo/call', 'Controller/Rack/BuilderMapTest::ExampleApp/call'],
+ ['Nested/Controller/Rack/BuilderMapTest::ExampleApp/call', 'Controller/Rack/BuilderMapTest::ExampleApp/call']
+ ])
+ end
+ end
+
+ def test_metrics_for_mapped_prefix
+ get '/prefix1'
+
+ assert_metrics_recorded([
+ 'Apdex',
+ 'ApdexAll',
+ 'HttpDispatcher',
+ 'Middleware/all',
+ 'Controller/Rack/BuilderMapTest::PrefixAppOne/call',
+ 'Apdex/Rack/BuilderMapTest::PrefixAppOne/call',
+ 'Middleware/Rack/BuilderMapTest::MiddlewareOne/call',
+ 'Middleware/Rack/BuilderMapTest::MiddlewareTwo/call',
+ 'Nested/Controller/Rack/BuilderMapTest::PrefixAppOne/call',
+ ['Middleware/Rack/BuilderMapTest::MiddlewareOne/call', 'Controller/Rack/BuilderMapTest::PrefixAppOne/call'],
+ ['Middleware/Rack/BuilderMapTest::MiddlewareTwo/call', 'Controller/Rack/BuilderMapTest::PrefixAppOne/call'],
+ ['Nested/Controller/Rack/BuilderMapTest::PrefixAppOne/call', 'Controller/Rack/BuilderMapTest::PrefixAppOne/call']
+ ])
+ end
+
+ def test_metrics_for_mapped_prefix_with_extra_middleware
+ get '/prefix2'
+
+ assert_metrics_recorded([
+ 'Apdex',
+ 'ApdexAll',
+ 'HttpDispatcher',
+ 'Middleware/all',
+ 'Controller/Rack/BuilderMapTest::PrefixAppTwo/call',
+ 'Apdex/Rack/BuilderMapTest::PrefixAppTwo/call',
+ 'Middleware/Rack/BuilderMapTest::MiddlewareOne/call',
+ 'Middleware/Rack/BuilderMapTest::MiddlewareTwo/call',
+ 'Middleware/Rack/BuilderMapTest::MiddlewareThree/call',
+ 'Nested/Controller/Rack/BuilderMapTest::PrefixAppTwo/call',
+ ['Middleware/Rack/BuilderMapTest::MiddlewareOne/call', 'Controller/Rack/BuilderMapTest::PrefixAppTwo/call'],
+ ['Middleware/Rack/BuilderMapTest::MiddlewareTwo/call', 'Controller/Rack/BuilderMapTest::PrefixAppTwo/call'],
+ ['Middleware/Rack/BuilderMapTest::MiddlewareThree/call', 'Controller/Rack/BuilderMapTest::PrefixAppTwo/call'],
+ ['Nested/Controller/Rack/BuilderMapTest::PrefixAppTwo/call', 'Controller/Rack/BuilderMapTest::PrefixAppTwo/call']
+ ])
+ end
+end
+
+end
@@ -2,6 +2,10 @@
# This file is distributed under New Relic's license terms.
# See https://github.com/newrelic/rpm/blob/master/LICENSE for complete details.
+# These tests are for confirming that our direct support Rack::URLMap works
+# properly. Tests against the builder interface more commonly used (i.e. map)
+# can be found elsewhere in this suite.
+
if NewRelic::Agent::Instrumentation::RackHelpers.rack_version_supported?
class UrlMapTest < Minitest::Test
@@ -23,7 +27,6 @@ def call(env)
class MiddlewareOne < SimpleMiddleware; end
class MiddlewareTwo < SimpleMiddleware; end
- class MiddlewareThree < SimpleMiddleware; end
class ExampleApp
def call(env)
@@ -39,20 +42,9 @@ def app
use MiddlewareOne
use MiddlewareTwo
- map '/prefix1' do
- run PrefixAppOne.new
- end
-
- map '/prefix2' do
- use MiddlewareThree
- run PrefixAppTwo.new
- end
-
- # Rack versions prior to 1.4 did not support combining map and run at the
- # top-level in the same Rack::Builder.
- if Rack::VERSION[1] >= 4
- run ExampleApp.new
- end
+ run Rack::URLMap.new(
+ '/prefix1' => PrefixAppOne.new,
+ '/prefix2' => PrefixAppTwo.new)
end
end
@@ -69,10 +61,12 @@ def test_metrics_for_default_prefix
'Apdex/Rack/UrlMapTest::ExampleApp/call',
'Middleware/Rack/UrlMapTest::MiddlewareOne/call',
'Middleware/Rack/UrlMapTest::MiddlewareTwo/call',
+ 'Nested/Controller/Rack/Rack::URLMap/call',
'Nested/Controller/Rack/UrlMapTest::ExampleApp/call',
['Middleware/Rack/UrlMapTest::MiddlewareOne/call', 'Controller/Rack/UrlMapTest::ExampleApp/call'],
['Middleware/Rack/UrlMapTest::MiddlewareTwo/call', 'Controller/Rack/UrlMapTest::ExampleApp/call'],
- ['Nested/Controller/Rack/UrlMapTest::ExampleApp/call', 'Controller/Rack/UrlMapTest::ExampleApp/call']
+ ['Nested/Controller/Rack/UrlMapTest::ExampleApp/call', 'Controller/Rack/UrlMapTest::ExampleApp/call'],
+ ['Nested/Controller/Rack/Rack::URLMap/call', 'Controller/Rack/UrlMapTest::ExampleApp/call']
])
end
end
@@ -89,9 +83,11 @@ def test_metrics_for_mapped_prefix
'Apdex/Rack/UrlMapTest::PrefixAppOne/call',
'Middleware/Rack/UrlMapTest::MiddlewareOne/call',
'Middleware/Rack/UrlMapTest::MiddlewareTwo/call',
+ 'Nested/Controller/Rack/Rack::URLMap/call',
'Nested/Controller/Rack/UrlMapTest::PrefixAppOne/call',
['Middleware/Rack/UrlMapTest::MiddlewareOne/call', 'Controller/Rack/UrlMapTest::PrefixAppOne/call'],
['Middleware/Rack/UrlMapTest::MiddlewareTwo/call', 'Controller/Rack/UrlMapTest::PrefixAppOne/call'],
+ ['Nested/Controller/Rack/Rack::URLMap/call', 'Controller/Rack/UrlMapTest::PrefixAppOne/call'],
['Nested/Controller/Rack/UrlMapTest::PrefixAppOne/call', 'Controller/Rack/UrlMapTest::PrefixAppOne/call']
])
end
@@ -108,11 +104,11 @@ def test_metrics_for_mapped_prefix_with_extra_middleware
'Apdex/Rack/UrlMapTest::PrefixAppTwo/call',
'Middleware/Rack/UrlMapTest::MiddlewareOne/call',
'Middleware/Rack/UrlMapTest::MiddlewareTwo/call',
- 'Middleware/Rack/UrlMapTest::MiddlewareThree/call',
+ 'Nested/Controller/Rack/Rack::URLMap/call',
'Nested/Controller/Rack/UrlMapTest::PrefixAppTwo/call',
['Middleware/Rack/UrlMapTest::MiddlewareOne/call', 'Controller/Rack/UrlMapTest::PrefixAppTwo/call'],
['Middleware/Rack/UrlMapTest::MiddlewareTwo/call', 'Controller/Rack/UrlMapTest::PrefixAppTwo/call'],
- ['Middleware/Rack/UrlMapTest::MiddlewareThree/call', 'Controller/Rack/UrlMapTest::PrefixAppTwo/call'],
+ ['Nested/Controller/Rack/Rack::URLMap/call', 'Controller/Rack/UrlMapTest::PrefixAppTwo/call'],
['Nested/Controller/Rack/UrlMapTest::PrefixAppTwo/call', 'Controller/Rack/UrlMapTest::PrefixAppTwo/call']
])
end

0 comments on commit 15d4183

Please sign in to comment.