Permalink
Browse files

Merge remote branch 'jose/perf'

  • Loading branch information...
Yehuda Katz Yehuda Katz
Yehuda Katz authored and Yehuda Katz committed Dec 27, 2009
2 parents 1c26ba4 + 97db79a commit 12e43494a748e0144195be12dc19161cc3e4d39f
Showing with 369 additions and 302 deletions.
  1. +0 −20 actionpack/lib/abstract_controller/logger.rb
  2. +0 −1 actionpack/lib/action_controller/base.rb
  3. +11 −0 actionpack/lib/action_controller/caching.rb
  4. +1 −1 actionpack/lib/action_controller/caching/fragments.rb
  5. +0 −72 actionpack/lib/action_controller/metal/benchmarking.rb
  6. +11 −11 actionpack/lib/action_controller/metal/filter_parameter_logging.rb
  7. +67 −16 actionpack/lib/action_controller/metal/logger.rb
  8. +3 −11 actionpack/lib/action_controller/rails.rb
  9. +45 −35 actionpack/lib/action_view/render/partials.rb
  10. +20 −17 actionpack/lib/action_view/render/rendering.rb
  11. +2 −4 actionpack/lib/action_view/template.rb
  12. +23 −0 actionpack/test/abstract_unit.rb
  13. +0 −7 actionpack/test/controller/base_test.rb
  14. +0 −33 actionpack/test/controller/benchmark_test.rb
  15. +1 −1 actionpack/test/controller/caching_test.rb
  16. +1 −1 actionpack/test/controller/filter_params_test.rb
  17. +46 −20 actionpack/test/controller/logging_test.rb
  18. +1 −31 actionpack/test/controller/render_test.rb
  19. +1 −0 activerecord/lib/active_record.rb
  20. +27 −0 activerecord/lib/active_record/controller_runtime.rb
  21. +5 −2 activerecord/lib/active_record/rails.rb
  22. +12 −0 activesupport/lib/active_support/core_ext/string/output_safety.rb
  23. +5 −6 activesupport/lib/active_support/notifications/instrumenter.rb
  24. +24 −0 activesupport/test/core_ext/string_ext_test.rb
  25. +10 −12 activesupport/test/notifications_test.rb
  26. +19 −1 railties/lib/rails/configuration.rb
  27. +18 −0 railties/lib/rails/plugin.rb
  28. +16 −0 railties/test/plugins/configuration_test.rb
@@ -9,25 +9,5 @@ module Logger
cattr_accessor :logger
extend ActiveSupport::Benchmarkable
end
-
- # A class that allows you to defer expensive processing
- # until the logger actually tries to log. Otherwise, you are
- # forced to do the processing in advance, and send the
- # entire processed String to the logger, which might
- # just discard the String if the log level is too low.
- #
- # TODO: Require that Rails loggers accept a block.
- class DelayedLog < ActiveSupport::BasicObject
- def initialize(&block)
- @str, @block = nil, block
- end
-
- def method_missing(*args, &block)
- unless @str
- @str, @block = @block.call, nil
- end
- @str.send(*args, &block)
- end
- end
end
end
@@ -15,7 +15,6 @@ class Base < Metal
include ActionController::ConditionalGet
include ActionController::RackDelegation
include ActionController::Logger
- include ActionController::Benchmarking
include ActionController::Configuration
# Legacy modules
@@ -60,6 +60,17 @@ module ClassMethods
def cache_configured?
perform_caching && cache_store
end
+
+ def log_event(name, before, after, instrumenter_id, payload)
+ if name.to_s =~ /(read|write|cache|expire|exist)_(fragment|page)\??/
+ key_or_path = payload[:key] || payload[:path]
+ human_name = name.to_s.humanize
+ duration = (after - before) * 1000
+ logger.info("#{human_name} #{key_or_path.inspect} (%.1fms)" % duration)
+ else
+ super
+ end
+ end
end
def caching_allowed?
@@ -74,7 +74,7 @@ def fragment_exist?(key, options = nil)
return unless cache_configured?
key = fragment_cache_key(key)
- ActiveSupport::Notifications.instrument(:fragment_exist?, :key => key) do
+ ActiveSupport::Notifications.instrument(:exist_fragment?, :key => key) do
cache_store.exist?(key, options)
end
end
@@ -1,72 +0,0 @@
-require 'active_support/core_ext/benchmark'
-
-module ActionController #:nodoc:
- # The benchmarking module times the performance of actions and reports to the logger. If the Active Record
- # package has been included, a separate timing section for database calls will be added as well.
- module Benchmarking #:nodoc:
- extend ActiveSupport::Concern
-
- protected
- def render(*args, &block)
- if logger
- if Object.const_defined?("ActiveRecord") && ActiveRecord::Base.connected?
- db_runtime = ActiveRecord::Base.connection.reset_runtime
- end
-
- render_output = nil
- @view_runtime = Benchmark.ms { render_output = super }
-
- if Object.const_defined?("ActiveRecord") && ActiveRecord::Base.connected?
- @db_rt_before_render = db_runtime
- @db_rt_after_render = ActiveRecord::Base.connection.reset_runtime
- @view_runtime -= @db_rt_after_render
- end
-
- render_output
- else
- super
- end
- end
-
- private
- def process_action(*args)
- if logger
- ms = [Benchmark.ms { super }, 0.01].max
- logging_view = defined?(@view_runtime)
- logging_active_record = Object.const_defined?("ActiveRecord") && ActiveRecord::Base.connected?
-
- log_message = 'Completed in %.0fms' % ms
-
- if logging_view || logging_active_record
- log_message << " ("
- log_message << view_runtime if logging_view
-
- if logging_active_record
- log_message << ", " if logging_view
- log_message << active_record_runtime + ")"
- else
- ")"
- end
- end
-
- log_message << " | #{response.status}"
- log_message << " [#{complete_request_uri rescue "unknown"}]"
-
- logger.info(log_message)
- else
- super
- end
- end
-
- def view_runtime
- "View: %.0f" % @view_runtime
- end
-
- def active_record_runtime
- db_runtime = ActiveRecord::Base.connection.reset_runtime
- db_runtime += @db_rt_before_render if @db_rt_before_render
- db_runtime += @db_rt_after_render if @db_rt_after_render
- "DB: %.0f" % db_runtime
- end
- end
-end
@@ -2,8 +2,6 @@ module ActionController
module FilterParameterLogging
extend ActiveSupport::Concern
- include AbstractController::Logger
-
module ClassMethods
# Replace sensitive parameter data from the request log.
# Filters parameters that have any of the arguments as a substring.
@@ -54,23 +52,25 @@ def filter_parameter_logging(*filter_words, &block)
end
protected :filter_parameters
end
- end
- INTERNAL_PARAMS = [:controller, :action, :format, :_method, :only_path]
+ protected
- def process(*)
- response = super
- if logger
- parameters = filter_parameters(params).except!(*INTERNAL_PARAMS)
- logger.info { " Parameters: #{parameters.inspect}" } unless parameters.empty?
+ # Overwrite log_process_action to include parameters information.
+ # If this method is invoked, it means logger is defined, so don't
+ # worry with such scenario here.
+ def log_process_action(controller) #:nodoc:
+ params = controller.send(:filter_parameters, controller.request.params)
+ logger.info " Parameters: #{params.inspect}" unless params.empty?
+ super
end
- response
end
+ INTERNAL_PARAMS = [:controller, :action, :format, :_method, :only_path]
+
protected
def filter_parameters(params)
- params.dup
+ params.dup.except!(*INTERNAL_PARAMS)
end
end
@@ -1,34 +1,85 @@
require 'abstract_controller/logger'
module ActionController
+ # Adds instrumentation to <tt>process_action</tt> and a <tt>log_event</tt> method
+ # responsible to log events from ActiveSupport::Notifications. This module handles
+ # :process_action and :render_template events but allows any other module to hook
+ # into log_event and provide its own logging facilities (as in ActionController::Caching).
module Logger
- # Override process_action in the AbstractController::Base
- # to log details about the method.
+ extend ActiveSupport::Concern
+
+ attr_internal :view_runtime
+
def process_action(action)
- result = ActiveSupport::Notifications.instrument(:process_action,
- :controller => self, :action => action) do
+ ActiveSupport::Notifications.instrument(:process_action, :controller => self, :action => action) do
super
end
+ end
+ def render(*args, &block)
if logger
- log = AbstractController::Logger::DelayedLog.new do
- "\n\nProcessing #{self.class.name}\##{action_name} " \
- "to #{request.formats} (for #{request_origin}) " \
- "[#{request.method.to_s.upcase}]"
+ render_output = nil
+
+ self.view_runtime = cleanup_view_runtime do
+ Benchmark.ms { render_output = super }
end
- logger.info(log)
+ render_output
+ else
+ super
end
+ end
- result
+ # If you want to remove any time taken into account in :view_runtime
+ # wrongly, you can do it here:
+ #
+ # def cleanup_view_runtime
+ # super - time_taken_in_something_expensive
+ # end
+ #
+ # :api: plugin
+ def cleanup_view_runtime #:nodoc:
+ yield
end
- private
+ module ClassMethods
+ # This is the hook invoked by ActiveSupport::Notifications.subscribe.
+ # If you need to log any event, overwrite the method and do it here.
+ def log_event(name, before, after, instrumenter_id, payload) #:nodoc:
+ if name == :process_action
+ duration = [(after - before) * 1000, 0.01].max
+ controller = payload[:controller]
+ request = controller.request
+
+ logger.info "\n\nProcessed #{controller.class.name}##{payload[:action]} " \
+ "to #{request.formats} (for #{request.remote_ip} at #{before.to_s(:db)}) " \
+ "[#{request.method.to_s.upcase}]"
- # Returns the request origin with the IP and time. This needs to be cached,
- # otherwise we would get different results for each time it calls.
- def request_origin
- @request_origin ||= "#{request.remote_ip} at #{Time.now.to_s(:db)}"
+ log_process_action(controller)
+
+ message = "Completed in %.0fms" % duration
+ message << " | #{controller.response.status}"
+ message << " [#{request.request_uri rescue "unknown"}]"
+
+ logger.info(message)
+ elsif name == :render_template
+ # TODO Make render_template logging work if you are using just ActionView
+ duration = (after - before) * 1000
+ message = "Rendered #{payload[:identifier]}"
+ message << " within #{payload[:layout]}" if payload[:layout]
+ message << (" (%.1fms)" % duration)
+ logger.info(message)
+ end
+ end
+
+ protected
+
+ # A hook which allows logging what happened during controller process action.
+ # :api: plugin
+ def log_process_action(controller) #:nodoc:
+ view_runtime = controller.send :view_runtime
+ logger.info(" View runtime: %.1fms" % view_runtime.to_f) if view_runtime
+ end
end
end
-end
+end
@@ -1,6 +1,7 @@
module ActionController
class Plugin < Rails::Plugin
plugin_name :action_controller
+ include_modules_in "ActionController::Base"
initializer "action_controller.set_configs" do |app|
app.config.action_controller.each do |k,v|
@@ -14,10 +15,6 @@ class Plugin < Rails::Plugin
end
# Routing must be initialized after plugins to allow the former to extend the routes
- # ---
- # If Action Controller is not one of the loaded frameworks (Configuration#frameworks)
- # this does nothing. Otherwise, it loads the routing definitions and sets up
- # loading module used to lazily load controllers (Configuration#controller_paths).
initializer "action_controller.initialize_routing" do |app|
app.route_configuration_files << app.config.routes_configuration_file
app.route_configuration_files << app.config.builtin_routes_configuration_file
@@ -88,13 +85,8 @@ class Plugin < Rails::Plugin
initializer "action_controller.notifications" do |app|
require 'active_support/notifications'
- ActiveSupport::Notifications.subscribe(/(read|write|cache|expire|exist)_(fragment|page)\??/) do |*args|
- event = ActiveSupport::Notifications::Event.new(*args)
-
- if logger = ActionController::Base.logger
- human_name = event.name.to_s.humanize
- logger.info("#{human_name} (%.1fms)" % event.duration)
- end
+ ActiveSupport::Notifications.subscribe do |*args|
+ ActionController::Base.log_event(*args) if ActionController::Base.logger
end
end
Oops, something went wrong.

0 comments on commit 12e4349

Please sign in to comment.