Extend custom logger to allow taking the notifications directly and generate it's own string. #57
Conversation
… support custom logger taking the notifications directly We have a custom logger, but in order to inspect the callstack and customize the message, we end up throwing out the notification_str and reaching into Thread.current[:prosopite_notifications] directly to get it. We also have to do our own filtering and cleaning because they haven't been cleaned yet. I'm proposing this change to let the customer logger take the `notifications` keyword that gets the notifications OR a single argument that is the generated string. In support of that, I clean the callstacks before calling the custom logger.
There was a problem hiding this comment.
Thank you @technicalpickles! This is really useful :)
I think that an example of getting access to the notifications from the custom logger (from the new notifications keyword argument) in the README would be helpful too.
| Prosopite.custom_logger = MyLogger.new | ||
| ``` | ||
|
|
||
| Additionally, you can further customize how notifications are displayed by makni ga `warn` method that takes a `notifications` keyword: |
| ```ruby | ||
| class FancifulLogger | ||
| def warn(notifications:) | ||
| notification.each do |queries, kaller| |
There was a problem hiding this comment.
notification vs notifications - the singular form doesn't exist.
| class FancifulLogger | ||
| def warn(notifications:) | ||
| notification.each do |queries, kaller| | ||
| # queries is an array of strings of santitized queries |
There was a problem hiding this comment.
typo santitized - however what do you mean by sanitized queries?
I would write this as The SQL queries that constitute the N+1 case
| def warn(notifications:) | ||
| notification.each do |queries, kaller| | ||
| # queries is an array of strings of santitized queries | ||
| # kaller is the callstack where it was called, and has been cleaned by Prosopite.backtrace_cleaner |
There was a problem hiding this comment.
I think they were called is more appropriate since we are referring to multiple queries.
| @raise ||= false | ||
| def clean_notifications(notifications) | ||
| notifications.map do |queries, kaller| | ||
| kaller = backtrace_cleaner.clean(kaller) |
There was a problem hiding this comment.
backtrace_cleaner is also called in the generate_notification_message - should be removed from there
We have a custom logger, but in order to inspect the callstack and customize the message, we end up throwing out the notification_str and reaching into
Thread.current[:prosopite_notifications]directly to get it.We also have to do our own filtering and cleaning because they haven't been cleaned yet.
I'm proposing this change to let the customer logger take the
notificationskeyword that gets the notifications OR a single argument that is the generated string. In support of that, I clean the callstacks before calling the custom logger.cc @geshwho who wrote our custom logger. Hoping we can refactor it to be smaller with these changes 😁