Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Log failed queries #326

Merged
merged 1 commit into from
Mar 16, 2020
Merged

Log failed queries #326

merged 1 commit into from
Mar 16, 2020

Conversation

paulcsmith
Copy link
Member

Ran into this while working on #322. I wanted to see the failed query.

The downside to this is that if you happen to rescue a PQ::PQError later in your app the failed query will still be logged. I think this is acceptable though since I can't think of a reason not to.

I've added a setting though just in case you don't want to log failed queries

@paulcsmith paulcsmith requested a review from jwoertink March 16, 2020 20:14
@paulcsmith paulcsmith force-pushed the pcs/log-failed-queries branch 3 times, most recently from d5ccd08 to 993681e Compare March 16, 2020 20:19
@@ -32,5 +47,13 @@ module DB
Avram.logger.log(level, {query: @query, args: logging_args})
end
end

private def log_error(*args_, args : Array? = nil)
Avram.settings.query_log_level.try do |level|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be log_failed_log_level?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops!

@paulcsmith paulcsmith force-pushed the pcs/log-failed-queries branch from 993681e to e2f47bd Compare March 16, 2020 20:21
@@ -21,6 +21,7 @@ module Avram
setting logger : Dexter::Logger = Dexter::Logger.new(nil)
setting query_log_level : ::Logger::Severity?
setting save_failed_log_level : ::Logger::Severity? = ::Logger::Severity::WARN
setting query_failed_log_level : ::Logger::Severity? = ::Logger::Severity::ERROR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, that makes more sense 😅

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@paulcsmith paulcsmith force-pushed the pcs/log-failed-queries branch 3 times, most recently from 16d4d5b to 29a25e9 Compare March 16, 2020 20:26
@paulcsmith paulcsmith force-pushed the pcs/log-failed-queries branch from 29a25e9 to 4962a9a Compare March 16, 2020 20:30
@paulcsmith paulcsmith merged commit 2193efa into master Mar 16, 2020
@paulcsmith paulcsmith deleted the pcs/log-failed-queries branch March 16, 2020 20:35
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants