Put some space for non-assets requests in development mode #23443

prathamesh-sonpatki · 2016-02-03T08:06:33Z

Fixes Better spacing with the default logger on Puma #23428.

Here is the sample output on an app created from rails master with this change - https://gist.github.com/prathamesh-sonpatki/ca9dbc32bcbae9066562

~~EDIT: Updated gist with Kasper's suggestion of printing a line before starting assets request - https://gist.github.com/prathamesh-sonpatki/c9075f78ac6f1f372c7c~~

kaspth · 2016-02-03T08:09:34Z

Shouldn't one space be enough? Having two lines between requests feels airy to me 😄

Can we make it so that there's a line just above the first asset request? Basically add a newline after: https://gist.github.com/prathamesh-sonpatki/ca9dbc32bcbae9066562#file-gistfile1-txt-L31

prathamesh-sonpatki · 2016-02-03T08:12:32Z

@kaspth like this - https://gist.github.com/prathamesh-sonpatki/c9075f78ac6f1f372c7c ?

kaspth · 2016-02-03T08:14:07Z

Almost! These should just be one line, IMO: https://gist.github.com/prathamesh-sonpatki/c9075f78ac6f1f372c7c#file-gistfile1-txt-L16-L17 😁

ArnonHongklay · 2016-02-03T08:15:34Z

railties/lib/rails/rack/logger.rb

+        # Put some space between for non-assets requests in development logs.
+        if development? && request.path !~ /^\/assets/
+          logger.debug ''
+          logger.debug ''


why? logger.debug ''

# Put some space between for non-assets requests in development logs. 😁

prathamesh-sonpatki · 2016-02-03T08:23:02Z

@kaspth For that we will need to know previous request path and based on that make a decision whether to show the line or not. I guess it will be more complicated.

kaspth · 2016-02-03T08:26:38Z

@prathamesh-sonpatki do we? Isn't outputting a space after every non asset request enough to get the result?

prathamesh-sonpatki · 2016-02-03T08:31:45Z

@kaspth That will work for non-asset request and asset request sequence but will not work for asset-request and non-asset request sequence.

For eg.

Started GET "/assets/application.self-b89234cf2659d7fedea75bca0b8d231ad7dfc2f3f57fcbaf5f44ed9dc384137b.js?body=1" for 127.0.0.1 at 2016-02-03 14:00:33 +0530
Started GET "/posts/new" for 127.0.0.1 at 2016-02-03 14:00:41 +0530
Processing by PostsController#new as HTML
  Rendered posts/_form.html.erb (11.2ms)
  Rendered posts/new.html.erb within layouts/application (13.5ms)
Completed 200 OK in 50ms (Views: 42.4ms | ActiveRecord: 3.2ms) (Views: 42.4ms | ActiveRecord: 3.2ms)

prathamesh-sonpatki · 2016-02-03T08:34:33Z

I think definitely we should keep Puma to multi threads but let's only send the line clears together with the line for completing a Rails request (the line reporting status code and timing). Don't want to send it together with assets etc.

I also tried this suggestion from DHH in #23428. With this approach we will get same result as in https://gist.github.com/prathamesh-sonpatki/ca9dbc32bcbae9066562. There will be no extra line before assets request as present here - https://gist.github.com/prathamesh-sonpatki/c9075f78ac6f1f372c7c

vipulnsward · 2016-02-03T08:45:03Z

railties/lib/rails/rack/logger.rb

@@ -79,6 +82,12 @@ def development?
      def logger
        Rails.logger
      end
+
+      def show_extra_line_in_development_for_non_assets_requests(path)
+        if development? && path !~ /^\/assets/


Does this work safely to handle public assets, assets from engines, vendored assets, etc?

btw, this won't work with http://guides.rubyonrails.org/asset_pipeline.html#changing-the-assets-path

Nice catch! It works with vendored assets but won't work with changing assets path.

prathamesh-sonpatki · 2016-02-03T09:30:00Z

I used DHH's original suggestion, it does not print a new line after all assets requests end but covers all other cases.

matthewd · 2016-02-03T10:05:20Z

actionpack/lib/action_controller/log_subscriber.rb

@@ -26,6 +26,9 @@ def process_action(event)
        end
        message = "Completed #{status} #{Rack::Utils::HTTP_STATUS_CODES[status]} in #{event.duration.round}ms"
        message << " (#{additions.join(" | ".freeze)})" unless additions.blank?
+        message << " (#{additions.join(" | ".freeze)})" unless additions.blank?


Don't think we need this 😉

- Fixes rails#23428.

kaspth · 2016-02-03T12:41:15Z

schneems · 2016-02-03T15:15:11Z

…elopment Put some space for non-assets requests in development mode

rafaelfranca · 2016-02-06T20:47:42Z

actionpack/lib/action_controller/log_subscriber.rb

@@ -26,6 +26,8 @@ def process_action(event)
        end
        message = "Completed #{status} #{Rack::Utils::HTTP_STATUS_CODES[status]} in #{event.duration.round}ms"
        message << " (#{additions.join(" | ".freeze)})" unless additions.blank?
+        message << "\n\n" if Rails.env.development?


This is coupling Action Pack with Railties we should check if Rails.env is defined before calling it.

@rafaelfranca fixed @ 4e4bcae

rails-bot assigned dhh Feb 3, 2016

prathamesh-sonpatki added the railties label Feb 3, 2016

prathamesh-sonpatki added this to the 5.0.0 milestone Feb 3, 2016

ArnonHongklay reviewed Feb 3, 2016
View reviewed changes

prathamesh-sonpatki force-pushed the fix-logging-in-development branch from 230e400 to 45d76a7 Compare February 3, 2016 08:21

vipulnsward reviewed Feb 3, 2016
View reviewed changes

prathamesh-sonpatki force-pushed the fix-logging-in-development branch from 45d76a7 to 0932edc Compare February 3, 2016 09:25

matthewd reviewed Feb 3, 2016
View reviewed changes

Put some space for non-assets requests in development mode

7ca7c0e

- Fixes rails#23428.

prathamesh-sonpatki force-pushed the fix-logging-in-development branch from 0932edc to 7ca7c0e Compare February 3, 2016 10:18

matthewd added a commit that referenced this pull request Feb 3, 2016

Merge pull request #23443 from prathamesh-sonpatki/fix-logging-in-dev…

8a84f1c

…elopment Put some space for non-assets requests in development mode

matthewd merged commit 8a84f1c into rails:master Feb 3, 2016

prathamesh-sonpatki deleted the fix-logging-in-development branch February 3, 2016 15:55

rafaelfranca reviewed Feb 6, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put some space for non-assets requests in development mode #23443

Put some space for non-assets requests in development mode #23443

prathamesh-sonpatki commented Feb 3, 2016

kaspth commented Feb 3, 2016

prathamesh-sonpatki commented Feb 3, 2016

kaspth commented Feb 3, 2016

ArnonHongklay Feb 3, 2016

kaspth Feb 3, 2016

prathamesh-sonpatki commented Feb 3, 2016

kaspth commented Feb 3, 2016

prathamesh-sonpatki commented Feb 3, 2016

prathamesh-sonpatki commented Feb 3, 2016

vipulnsward Feb 3, 2016

prathamesh-sonpatki Feb 3, 2016

prathamesh-sonpatki commented Feb 3, 2016

matthewd Feb 3, 2016

prathamesh-sonpatki Feb 3, 2016

kaspth commented Feb 3, 2016

schneems commented Feb 3, 2016

rafaelfranca Feb 6, 2016

kaspth Feb 6, 2016

rafaelfranca Feb 6, 2016

Put some space for non-assets requests in development mode #23443

Put some space for non-assets requests in development mode #23443

Conversation

prathamesh-sonpatki commented Feb 3, 2016

kaspth commented Feb 3, 2016

prathamesh-sonpatki commented Feb 3, 2016

kaspth commented Feb 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prathamesh-sonpatki commented Feb 3, 2016

kaspth commented Feb 3, 2016

prathamesh-sonpatki commented Feb 3, 2016

prathamesh-sonpatki commented Feb 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prathamesh-sonpatki commented Feb 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspth commented Feb 3, 2016

schneems commented Feb 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment