Respect action_dispatch.log_rescued_responses #266
Conversation
This is a new configuration option in Rails 7 which defaults to true. When an app sets this to false, this method does not log anything at all for rescued responses. Because rails_semantic_logger monkey patches ActionDispatch::DebugExceptions and overrides this method, this configuration option does not have any effect. Let's correct that. More context on this configuration option: - PR which introduced it rails/rails#42592 - Evidence that this implementation should work fine in Rails 7.1+ https://github.com/rails/rails/blob/v7.1.0/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb#L137 I also noticed that rails_semantic_logger is not respecting the action_dispatch.debug_exception_log_level configuration, and it makes its own decisions about what is an appropriate level. I updated this to also follow that configuration. This is also how it works in Rails 7.1+: https://github.com/rails/rails/blob/v7.1.0/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb#L154
| def log_error(request, wrapper) | ||
| Rails.application.deprecators.silence do | ||
| level = wrapper.respond_to?(:rescue_response?) && wrapper.rescue_response? ? :debug : :fatal | ||
| return if !log_rescued_responses?(request) && wrapper.rescue_response? |
There was a problem hiding this comment.
In #179, there was a concern about the rescue_responses? method not working in Rails 5 and 6, which led to this respond_to? check being added. I don't believe it's necessary, however, because this is within a conditional that checks (Rails::VERSION::MAJOR == 7 && Rails::VERSION::MINOR >= 1) || Rails::VERSION::MAJOR > 7, so we can feel confident that we are in Rails 7.1+. Given that, I took the liberty of removing the respond_to? check
| level = wrapper.respond_to?(:rescue_response?) && wrapper.rescue_response? ? :debug : :fatal | ||
| return if !log_rescued_responses?(request) && wrapper.rescue_response? | ||
|
|
||
| level = request.get_header("action_dispatch.debug_exception_log_level") |
There was a problem hiding this comment.
Making this more aligned with the default behavior of ActionDispatch::DebugExceptions makes sense to me. I don't think this is a breaking change, because the previous behavior was merged to the master branch, but never released. Let me know if you'd like me to remove this part though.
|
I merged this PR because the test failures appeared to be what we saw previously on master. Had to revert the merge because of new test failures. Please review the test failures an resubmit the PR. |
|
👍 here's the followup: #269 |
3 participants
This PR is iterating a bit on #179
Description of changes
action_dispatch.log_rescued_responses is a new configuration option in Rails 7 which defaults to true. When an app sets this to false, this method does not log anything at all for rescued responses.
Because rails_semantic_logger monkey patches
ActionDispatch::DebugExceptions and overrides this method, this configuration option does not have any effect. Let's correct that.
More context on this configuration option:
rescue_responsesrails/rails#42592I also noticed that rails_semantic_logger is not respecting the action_dispatch.debug_exception_log_level configuration, and it makes its own decisions about what is an appropriate level. I updated this to also follow that configuration.
This is also how it works in Rails 7.1+:
https://github.com/rails/rails/blob/v7.1.0/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb#L154
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.