Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Added X-Request-Id tracking and TaggedLogging to easily log that and …

…other production concerns
  • Loading branch information...
commit afde6fdd5ef3e6b0693a7e330777e85ef4cffddb 1 parent 3a746f7
David Heinemeier Hansson dhh authored
2  actionpack/CHANGELOG
View
@@ -1,5 +1,7 @@
*Rails 3.2.0 (unreleased)*
+* Added ActionDispatch::RequestId middleware that'll make a unique X-Request-Id header available to the response and enables the ActionDispatch::Request#uuid method. This makes it easy to trace requests from end-to-end in the stack and to identify individual requests in mixed logs like Syslog [DHH]
+
* Limit the number of options for select_year to 1000.
Pass the :max_years_allowed option to set your own limit.
1  actionpack/lib/action_dispatch.rb
View
@@ -47,6 +47,7 @@ module ActionDispatch
end
autoload_under 'middleware' do
+ autoload :RequestId
autoload :BestStandardsSupport
autoload :Callbacks
autoload :Cookies
10 actionpack/lib/action_dispatch/http/request.rb
View
@@ -177,6 +177,16 @@ def remote_ip
@remote_ip ||= (@env["action_dispatch.remote_ip"] || ip).to_s
end
+ # Returns the unique request id, which is based off either the X-Request-Id header that can
+ # be generated by a firewall, load balancer, or web server or by the RequestId middleware
+ # (which sets the action_dispatch.request_id environment variable).
+ #
+ # This unique ID is useful for tracing a request from end-to-end as part of logging or debugging.
+ # This relies on the rack variable set by the ActionDispatch::RequestId middleware.
+ def uuid
+ @uuid ||= env["action_dispatch.request_id"]
+ end
+
# Returns the lowercase name of the HTTP server software.
def server_software
(@env['SERVER_SOFTWARE'] && /^([a-zA-Z]+)/ =~ @env['SERVER_SOFTWARE']) ? $1.downcase : nil
38 actionpack/lib/action_dispatch/middleware/request_id.rb
View
@@ -0,0 +1,38 @@
+require 'digest/md5'
+
+module ActionDispatch
+ # Makes a unique request id available to the action_dispatch.request_id env variable (which is then accessible through
+ # ActionDispatch::Request#uuid) and sends the same id to the client via the X-Request-Id header.
+ #
+ # The unique request id is either based off the X-Request-Id header in the request, which would typically be generated
+ # by a firewall, load balancer, or the web server, or, if this header is not available, a random uuid. If the
+ # header is accepted from the outside world, we sanitize it to a max of 255 chars and alphanumeric and dashes only.
+ #
+ # The unique request id can be used to trace a request end-to-end and would typically end up being part of log files
+ # from multiple pieces of the stack.
+ class RequestId
+ def initialize(app)
+ @app = app
+ end
+
+ def call(env)
+ env["action_dispatch.request_id"] = external_request_id(env) || internal_request_id
+
+ status, headers, body = @app.call(env)
+
+ headers["X-Request-Id"] = env["action_dispatch.request_id"]
Paul Donohue
PaulSD added a note

My organization has been doing exactly what your patch here does for a few years now (and amusingly enough, I was literally just updating our Rails-related documentation on it when I stumbled across this commit). However, our header is "X-Request-ID", and that case-sensitive name has ended up in a large number of applications (using a large number of different languages and frameworks, not just Rails), so it would not be trivial for me to change the case of my header name, and as currently written, this particular line of code won't work for me.

Any chance you could change this to X-Request-ID? If not, is there a way to make this more generic? Perhaps allow the application developer to specify the name of the header instead of hard-coding it here?

José Valim Owner

We could make it configurable. The header key could be a parameter given to the middleware on initialization here: afde6fd#L11R167

The configuration name could be config.action_dispatch.request_id_header and the default could be set here:

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/railtie.rb#L5

A patch with tests would be welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ [ status, headers, body ]
+ end
+
+ private
+ def external_request_id(env)
+ if env["HTTP_X_REQUEST_ID"].present?
+ env["HTTP_X_REQUEST_ID"].gsub(/[^\w\d\-]/, "").first(255)

Doesn't \w include [a-zA-Z0-9_], thus making the \d unnecessary?

José Valim Owner

Yup. Pull request please? OR do you prefer if I change it myself (faster)?

Paul Donohue
PaulSD added a note

Why are other characters stripped out? My load balancers seem to be including @ characters in the request ID sometimes, and the fact that this code strips that out ends up breaking things. Would it hurt anything to leave it there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+ end
+
+ def internal_request_id
+ SecureRandom.uuid
Pascal Hurni
phurni added a note

This is only available on MRI 1.9, not on 1.8.7, don't known for other rubies.
Maybe should be wrapped behind ActiveSupport

Steve Klabnik Collaborator

Rails only supports 1.9.3. (on master)

Pascal Hurni
phurni added a note

Maybe rails4 but master is still rails 3.2, no?
I just looked at the guides and even edge guides (http://edgeguides.rubyonrails.org/getting_started.html#guide-assumptions), they still set the minimum to 1.8.7.

Rafael Mendonça França Owner

@phurni also master is 4.0. So we need to change that guide. Doing now.

Pascal Hurni
phurni added a note

Thanx for these clarifications!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+ end
+end
59 actionpack/test/dispatch/request_id_test.rb
View
@@ -0,0 +1,59 @@
+require 'abstract_unit'
+
+class RequestIdTest < ActiveSupport::TestCase
+ test "passing on the request id from the outside" do
+ assert_equal "external-uu-rid", stub_request('HTTP_X_REQUEST_ID' => 'external-uu-rid').uuid
+ end
+
+ test "ensure that only alphanumeric uurids are accepted" do
+ assert_equal "X-Hacked-HeaderStuff", stub_request('HTTP_X_REQUEST_ID' => '; X-Hacked-Header: Stuff').uuid
+ end
+
+ test "ensure that 255 char limit on the request id is being enforced" do
+ assert_equal "X" * 255, stub_request('HTTP_X_REQUEST_ID' => 'X' * 500).uuid
+ end
+
+ test "generating a request id when none is supplied" do
+ assert_match /\w+-\w+-\w+-\w+-\w+/, stub_request.uuid
+ end
+
+ private
+ def stub_request(env = {})
+ ActionDispatch::RequestId.new(->(env) { [ 200, env, [] ] }).call(env)
Daniel Schierbeck
dasch added a note

So Rails 3.2 is going to be Ruby 1.9 only?

Vijay Dev Collaborator

No, it won't be. This needs to change to the 1.8.7 syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ ActionDispatch::Request.new(env)
+ end
+end
+
+# FIXME: Testing end-to-end doesn't seem to work
+#
+# class RequestIdResponseTest < ActionDispatch::IntegrationTest
+# class TestController < ActionController::Base
+# def index
+# head :ok
+# end
+# end
+#
+# test "request id is passed all the way to the response" do
+# with_test_route_set do
+# get '/'
+# puts @response.headers.inspect
+# assert_equal "internal-uu-rid", @response.headers["X-Request-Id"]
+# end
+# end
+#
+#
+# private
+# def with_test_route_set
+# with_routing do |set|
+# set.draw do
+# match ':action', to: ::RequestIdResponseTest::TestController
+# end
+#
+# @app = self.class.build_app(set) do |middleware|
+# middleware.use ActionDispatch::RequestId
+# end
+#
+# yield
+# end
+# end
+# end
7 activesupport/CHANGELOG
View
@@ -1,5 +1,12 @@
*Rails 3.2.0 (unreleased)*
+* Added ActiveSupport:TaggedLogging that can wrap any standard Logger class to provide tagging capabilities [DHH]
+
+ Logger = ActiveSupport::TaggedLogging.new(Logger.new(STDOUT))
+ Logger.tagged("BCX") { Logger.info "Stuff" } # Logs "[BCX] Stuff"
+ Logger.tagged("BCX", "Jason") { Logger.info "Stuff" } # Logs "[BCX] [Jason] Stuff"
+ Logger.tagged("BCX") { Logger.tagged("Jason") { Logger.info "Stuff" } } # Logs "[BCX] [Jason] Stuff"
+
* Added safe_constantize that constantizes a string but returns nil instead of an exception if the constant (or part of it) does not exist [Ryan Oblak]
* ActiveSupport::OrderedHash is now marked as extractable when using Array#extract_options! [Prem Sichanugrist]
1  activesupport/lib/active_support.rb
View
@@ -71,6 +71,7 @@ module ActiveSupport
autoload :OrderedOptions
autoload :Rescuable
autoload :StringInquirer
+ autoload :TaggedLogging
autoload :XmlMini
end
68 activesupport/lib/active_support/tagged_logging.rb
View
@@ -0,0 +1,68 @@
+module ActiveSupport
+ # Wraps any standard Logger class to provide tagging capabilities. Examples:
+ #
+ # Logger = ActiveSupport::TaggedLogging.new(Logger.new(STDOUT))
+ # Logger.tagged("BCX") { Logger.info "Stuff" } # Logs "[BCX] Stuff"
+ # Logger.tagged("BCX", "Jason") { Logger.info "Stuff" } # Logs "[BCX] [Jason] Stuff"
+ # Logger.tagged("BCX") { Logger.tagged("Jason") { Logger.info "Stuff" } } # Logs "[BCX] [Jason] Stuff"
+ #
+ # This is used by the default Rails.logger as configured by Railties to make it easy to stamp log lines
+ # with subdomains, request ids, and anything else to aid debugging of multi-user production applications.
+ class TaggedLogging
+ def initialize(logger)
+ @logger = logger
+ @tags = []
José Valim Owner

This is not thread safe. We should probably keep a hash where the key is the thread id and the tags the values.

Tim Pease
TwP added a note

Use a thread local variable to store the tags, and provide an accessor method. All access to the tags would be through the accessor - any code that references @tags would need to be modified.

def tags
  Thread.current[:tagged_logging] ||= []
end

def formatted_tags
  if tags.any?
    tags.collect { |tag| "#{tag}" }.join(" ") + " "
  end
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+
+ def tagged(*tags)
+ new_tags = Array.wrap(tags).flatten
+ @tags += new_tags
+ yield
+ ensure
+ new_tags.size.times { @tags.pop }
+ end
+
+
+ def add(severity, message = nil, progname = nil, &block)
+ @logger.add(severity, "#{tags}#{message}", progname, &block)
+ end
+
+
+ def fatal(progname = nil, &block)
+ add(@logger.class::FATAL, progname, &block)
+ end
+
+ def error(progname = nil, &block)
+ add(@logger.class::ERROR, progname, &block)
+ end
+
+ def warn(progname = nil, &block)
+ add(@logger.class::WARN, progname, &block)
+ end
+
+ def info(progname = nil, &block)
+ add(@logger.class::INFO, progname, &block)
+ end
+
+ def debug(progname = nil, &block)
+ add(@logger.class::DEBUG, progname, &block)
+ end
+
+ def unknown(progname = nil, &block)
+ add(@logger.class::UNKNOWN, progname, &block)
+ end
+
+
+ def method_missing(method, *args)
+ @logger.send(method, *args)
+ end
+
+
+ private
+ def tags
+ if @tags.any?
+ @tags.collect { |tag| "[#{tag}]" }.join(" ") + " "
+ end
+ end
+ end
+end
34 activesupport/test/tagged_logging_test.rb
View
@@ -0,0 +1,34 @@
+require 'abstract_unit'
+require 'active_support/core_ext/logger'
+require 'active_support/tagged_logging'
+
+class TaggedLoggingTest < ActiveSupport::TestCase
+ setup do
+ @output = StringIO.new
+ @logger = ActiveSupport::TaggedLogging.new(Logger.new(@output))
+ end
+
+ test "tagged once" do
+ @logger.tagged("BCX") { @logger.info "Funky time" }
+ assert_equal "[BCX] Funky time\n", @output.string
+ end
+
+ test "tagged twice" do
+ @logger.tagged("BCX") { @logger.tagged("Jason") { @logger.info "Funky time" } }
+ assert_equal "[BCX] [Jason] Funky time\n", @output.string
+ end
+
+ test "tagged thrice at once" do
+ @logger.tagged("BCX", "Jason", "New") { @logger.info "Funky time" }
+ assert_equal "[BCX] [Jason] [New] Funky time\n", @output.string
+ end
+
+ test "mixed levels of tagging" do
+ @logger.tagged("BCX") do
+ @logger.tagged("Jason") { @logger.info "Funky time" }
+ @logger.info "Junky time!"
+ end
+
+ assert_equal "[BCX] [Jason] Funky time\n[BCX] Junky time!\n", @output.string
+ end
+end
2  railties/CHANGELOG
View
@@ -1,5 +1,7 @@
*Rails 3.2.0 (unreleased)*
+* Added Rails::Rack::TaggedLogging middleware by default that will apply any tags set in config.log_tags to the newly ActiveSupport::TaggedLogging Rails.logger. This makes it easy to tag log lines with debug information like subdomain and request id -- both very helpful in debugging multi-user production applications [DHH]
+
* Default options to `rails new` can be set in ~/.railsrc [Guillermo Iguaran]
* Added destroy alias to Rails engines. [Guillermo Iguaran]
3  railties/guides/code/getting_started/config/environments/production.rb
View
@@ -33,6 +33,9 @@
# See everything in the log (default is :info)
# config.log_level = :debug
+ # Prepend all log lines with the following tags
+ # config.log_tags = [ :subdomain, :uuid ]
+
# Use a different logger for distributed setups
# config.logger = SyslogLogger.new
2  railties/lib/rails/application.rb
View
@@ -164,6 +164,8 @@ def default_middleware_stack
middleware.use ::Rack::Lock unless config.allow_concurrency
middleware.use ::Rack::Runtime
middleware.use ::Rack::MethodOverride
+ middleware.use ::ActionDispatch::RequestId
+ middleware.use ::Rails::Rack::TaggedLogging, config.log_tags
middleware.use ::Rails::Rack::Logger # must come after Rack::MethodOverride to properly log overridden methods
middleware.use ::ActionDispatch::ShowExceptions, config.consider_all_requests_local
middleware.use ::ActionDispatch::RemoteIp, config.action_dispatch.ip_spoofing_check, config.action_dispatch.trusted_proxies
4 railties/lib/rails/application/bootstrap.rb
View
@@ -24,12 +24,12 @@ module Bootstrap
initializer :initialize_logger, :group => :all do
Rails.logger ||= config.logger || begin
path = config.paths["log"].first
- logger = ActiveSupport::BufferedLogger.new(path)
+ logger = ActiveSupport::TaggedLogging.new(ActiveSupport::BufferedLogger.new(path))
logger.level = ActiveSupport::BufferedLogger.const_get(config.log_level.to_s.upcase)
logger.auto_flushing = false if Rails.env.production?
logger
rescue StandardError
- logger = ActiveSupport::BufferedLogger.new(STDERR)
+ logger = ActiveSupport::TaggedLogging.new(ActiveSupport::BufferedLogger.new(STDERR))
logger.level = ActiveSupport::BufferedLogger::WARN
logger.warn(
"Rails Error: Unable to access log file. Please ensure that #{path} exists and is chmod 0666. " +
2  railties/lib/rails/application/configuration.rb
View
@@ -8,7 +8,7 @@ class Configuration < ::Rails::Engine::Configuration
attr_accessor :allow_concurrency, :asset_host, :asset_path, :assets,
:cache_classes, :cache_store, :consider_all_requests_local,
:dependency_loading, :filter_parameters,
- :force_ssl, :helpers_paths, :logger, :preload_frameworks,
+ :force_ssl, :helpers_paths, :logger, :log_tags, :preload_frameworks,
:reload_plugins, :secret_token, :serve_static_assets,
:ssl_options, :static_cache_control, :session_options,
:time_zone, :whiny_nils
1  railties/lib/rails/rack.rb
View
@@ -3,5 +3,6 @@ module Rack
autoload :Debugger, "rails/rack/debugger"
autoload :Logger, "rails/rack/logger"
autoload :LogTailer, "rails/rack/log_tailer"
+ autoload :TaggedLogging, "rails/rack/tagged_logging"
end
end
4 railties/lib/rails/rack/logger.rb
View
@@ -21,8 +21,8 @@ def before_dispatch(env)
request = ActionDispatch::Request.new(env)
path = request.filtered_path
- info "\n\nStarted #{request.request_method} \"#{path}\" " \
- "for #{request.ip} at #{Time.now.to_default_s}"
+ info "\n\n"
+ info "Started #{request.request_method} \"#{path}\" for #{request.ip} at #{Time.now.to_default_s}"
end
def after_dispatch(env)
39 railties/lib/rails/rack/tagged_logging.rb
View
@@ -0,0 +1,39 @@
+module Rails
+ module Rack
+ # Enables easy tagging of any logging activity that occurs within the Rails request cycle. The tags are configured via the
+ # config.log_tags setting. The tags can either be strings, procs taking a request argument, or the symbols :uuid or :subdomain.
+ # The latter two are then automatically expanded to request.uuid and request.subdaomins.first -- the two most common tags
+ # desired in production logs.
+ class TaggedLogging
+ def initialize(app, tags = nil)
+ @app, @tags = app, tags
+ end
+
+ def call(env)
+ if @tags
+ Rails.logger.tagged(compute_tags(env)) { @app.call(env) }
+ else
+ @app.call(env)
+ end
+ end
+
+ private
+ def compute_tags(env)
+ request = ActionDispatch::Request.new(env)
+
+ @tags.collect do |tag|
+ case tag
+ when Proc
+ tag.call(request)
+ when :uuid
José Valim Owner

I believe we have a :subdomain method in the request (if not, we could add it). That said, this could probably be something like:

when Symbol
  request.send(tag)
David Heinemeier Hansson Owner
dhh added a note

Good idea. Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ request.uuid
+ when :subdomain
+ request.subdomains.first
+ else
+ tag
+ end
+ end
+ end
+ end
+ end
+end

5 comments on commit afde6fd

José Valim

I believe we have a :subdomain method in the request (if not, we could add it). That said, this could probably be something like:

when Symbol
  request.send(tag)
José Valim

This is not thread safe. We should probably keep a hash where the key is the thread id and the tags the values.

Daniel Schierbeck

So Rails 3.2 is going to be Ruby 1.9 only?

Vijay Dev

No, it won't be. This needs to change to the 1.8.7 syntax.

Jason Morrison

Would it be useful to default to including this header in XHR calls initiated from the page too, or should that be counted as a new request stack? E.g. an addition to jquery_ujs.js

José Valim
Owner

This is to identify all logging for the same request. It does not intend to identity a user across requests.

Erik Peterson

This is quite useful...off to see if an F5 can generate the uuid.

Marc Bowes

Doesn't \w include [a-zA-Z0-9_], thus making the \d unnecessary?

José Valim

Yup. Pull request please? OR do you prefer if I change it myself (faster)?

Tim Pease

Use a thread local variable to store the tags, and provide an accessor method. All access to the tags would be through the accessor - any code that references @tags would need to be modified.

def tags
  Thread.current[:tagged_logging] ||= []
end

def formatted_tags
  if tags.any?
    tags.collect { |tag| "#{tag}" }.join(" ") + " "
  end
end
Jeremy Kemper
Owner

Build red! Need to update Railties' test/application/rack/logger_test.rb

Aaron Patterson
Owner

Seems like a good candidate for reverting. :heart:

Paul Donohue

My organization has been doing exactly what your patch here does for a few years now (and amusingly enough, I was literally just updating our Rails-related documentation on it when I stumbled across this commit). However, our header is "X-Request-ID", and that case-sensitive name has ended up in a large number of applications (using a large number of different languages and frameworks, not just Rails), so it would not be trivial for me to change the case of my header name, and as currently written, this particular line of code won't work for me.

Any chance you could change this to X-Request-ID? If not, is there a way to make this more generic? Perhaps allow the application developer to specify the name of the header instead of hard-coding it here?

José Valim

We could make it configurable. The header key could be a parameter given to the middleware on initialization here: afde6fd#L11R167

The configuration name could be config.action_dispatch.request_id_header and the default could be set here:

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/railtie.rb#L5

A patch with tests would be welcome.

Pascal Hurni

This is only available on MRI 1.9, not on 1.8.7, don't known for other rubies.
Maybe should be wrapped behind ActiveSupport

Steve Klabnik

Rails only supports 1.9.3. (on master)

Pascal Hurni

Maybe rails4 but master is still rails 3.2, no?
I just looked at the guides and even edge guides (http://edgeguides.rubyonrails.org/getting_started.html#guide-assumptions), they still set the minimum to 1.8.7.

Rafael Mendonça França

@phurni also master is 4.0. So we need to change that guide. Doing now.

Paul Donohue

Why are other characters stripped out? My load balancers seem to be including @ characters in the request ID sometimes, and the fact that this code strips that out ends up breaking things. Would it hurt anything to leave it there?

Please sign in to comment.
Something went wrong with that request. Please try again.