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

WIP: Errors in logs should show log tags as well. #23203

Merged
merged 1 commit into from Feb 16, 2016

Conversation

Projects
None yet
5 participants
@vipulnsward
Member

vipulnsward commented Jan 22, 2016

  • Changed formatted_code_for to return array of logs to be tagged for each line
  • Changed some render tests to match new behaviour of return

Fixes #22979

r? @schneems

Opening to get feedback on the logger to be used. We construct new logger from env, that doesn't log tags, which for example- ActionView::Base.logger does.
What's an appropriate logger here though? I believe ActionView::Base.logger, since thats where the error arises from.

@schneems

This comment has been minimized.

Member

schneems commented Feb 12, 2016

Sorry for taking so long, this looks good. Can you take a look at failing tests?

  1) Failure:
DebugExceptionsTest#test_uses_logger_from_env [/home/travis/build/rails/rails/actionpack/test/dispatch/debug_exceptions_test.rb:351]:
Expected /puke/ to match "".
  2) Failure:
DebugExceptionsTest#test_logs_exception_backtrace_when_all_lines_silenced [/home/travis/build/rails/rails/actionpack/test/dispatch/debug_exceptions_test.rb:374]:
Expected 0 to be > 10.

@schneems schneems added this to the 5.0.0 milestone Feb 12, 2016

WIP: Errors in logs should show log tags as well.
- Changed formatted_code_for to return array of logs to be tagged for each line
- Changed some render tests to match new behaviour of return

Fixes #22979

@vipulnsward vipulnsward force-pushed the vipulnsward:22979-show-tags-on-exception branch to 632938c Feb 12, 2016

@vipulnsward

This comment has been minimized.

Member

vipulnsward commented Feb 12, 2016

@schneems all green now.

@schneems

This comment has been minimized.

Member

schneems commented Feb 16, 2016

🚀 thanks!

schneems added a commit that referenced this pull request Feb 16, 2016

Merge pull request #23203 from vipulnsward/22979-show-tags-on-exception
WIP: Errors in logs should show log tags as well.

@schneems schneems merged commit ac317e7 into rails:master Feb 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vipulnsward vipulnsward deleted the vipulnsward:22979-show-tags-on-exception branch Feb 16, 2016

@@ -149,22 +149,26 @@ def render(status, body, format)
def log_error(request, wrapper)
logger = logger(request)
return unless logger

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 17, 2016

Member

😢 Why was this removed?

end
end
def logger(request)
request.logger || stderr_logger
def log_array logger, array

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 17, 2016

Member

Parentheses please 🙏

rafaelfranca added a commit that referenced this pull request Feb 17, 2016

Fix code style
This change was added in #23203 and it was not conforming our code style.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment