Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Include exception causes into log messages #50145

Merged
merged 1 commit into from
Nov 28, 2023
Merged

Include exception causes into log messages #50145

merged 1 commit into from
Nov 28, 2023

Conversation

fatkodima
Copy link
Member

@fatkodima fatkodima commented Nov 23, 2023 •
edited
Loading

Fixes #50125.

#39723 changed bad connection related errors so that we now have some (useful for debugging) details from the original database exceptions lost.

This PR adds original database errors to these new exceptions classes, so we can get the exception details. It also prints these added exceptions messages by default as part of the new exceptions messages.

@fatkodima fatkodima mentioned this pull request Nov 23, 2023
Closed
@fatkodima fatkodima force-pushed the include-db-errors-in-connection-errors branch from 7a54ffa to 7f518eb Compare November 23, 2023 00:41
@matthewd
Copy link
Member

Can we achieve this at the reporting end? cause should already be there without us needing to manually carry it along.

@fatkodima
Copy link
Member Author

Can we achieve this at the reporting end?

Can you elaborate?

Yes, cause is there when this exception is rescued somewhere. I initially tried to not use a :db_error, but use cause.message here https://github.com/rails/rails/pull/50145/files#diff-805517f2e613a421493817fa15ecd42e583caf1b4f6f6972ec6e8103f41e04a2R66 but it is nil at the time of creation of this error in the constructor.

@matthewd
Copy link
Member

I'm claiming that it shouldn't be up to the wrapper exception to store & regurgitate the wrapped exception's message -- if the cause's message is not also visible in the place where our exception is appearing (terminal? log file? HTML 500 response?), maybe that's what we should try to fix.

@byroot
Copy link
Member

byroot commented Nov 23, 2023

Agree that this is Exception#cause's job.

@fatkodima
Copy link
Member Author

if the cause's message is not also visible in the place where our exception is appearing (terminal? log file? HTML 500 response?), maybe that's what we should try to fix.

I am stuck. Can you please point me where to look in the code to implement this fix?

@byroot
Copy link
Member

byroot commented Nov 23, 2023

@fatkodima depends what you means exactly, but for what prints the error in the console, it's at

rails/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb

Lines 133 to 149 in e7ea111

def log_error(request, wrapper)
logger = logger(request)
return unless logger
return if !log_rescued_responses?(request) && wrapper.rescue_response?
trace = wrapper.exception_trace
message = []
message << " "
message << "#{wrapper.exception_class_name} (#{wrapper.message}):"
message.concat(wrapper.annotated_source_code)
message << " "
message.concat(trace)
log_array(logger, message, request)
end

@byroot
Copy link
Member

byroot commented Nov 23, 2023

But the standard debug HTML page would also be updated to display the error cause(s) if it exist.

@fatkodima fatkodima force-pushed the include-db-errors-in-connection-errors branch from 7f518eb to 4fc2df2 Compare November 23, 2023 15:21
@rails-bot rails-bot bot added the actionpack label Nov 23, 2023
@fatkodima fatkodima changed the title Include original database errors into connection related errors Include exception causes into log messages Nov 23, 2023
@fatkodima
Copy link
Member Author

Debug HTML page and console already print exception causes, it was only missing in the log messages.

@byroot byroot merged commit 4467b26 into rails:main Nov 28, 2023
3 checks passed
@fatkodima fatkodima deleted the include-db-errors-in-connection-errors branch November 28, 2023 21:13
@skipkayhil skipkayhil mentioned this pull request Nov 29, 2023
Closed
4 tasks
@MaxLap MaxLap mentioned this pull request Dec 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byroot byroot byroot approved these changes

Assignees
No one assigned
Labels
actionpack
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error Message Degradation for Connection-Related Error Especially for Production Environment
3 participants
@fatkodima @matthewd @byroot