umbrellio / lamian

Lamian is an extension to exception notification system, which helps to add logs of current request/job to the exception report
MIT License
8 stars 2 forks source link

Make rails dependency an optional #9

Open JelF opened 7 years ago

JelF commented 7 years ago

Idea:

@fiscal-cliff offered some help, would you do all this work for me?

JelF commented 7 years ago

@fiscal-cliff, I can do #8 to simplify this task for you

fiscal-cliff commented 7 years ago

I managed to sort out this issue First of all, the notifier itself has rails as a dependency. The only way to evade this requirement is to use simple_email_exception_notifier which we use already. The notifier provides plain text formatter which is not compatible with :email notifier. So rendering request_log section inside the notifier turned out to be way harder than I expected. I've patched the notifier as follows

module SimpleNotifierPatch
  def compose_message(exception, env, options)
    SimpleEmailExceptionNotifier::Message.compose do |m|
      m.print_summary(exception)
      m.print_section('Backtrace', exception.backtrace)

      m.print_section('Request log', Lamian.logger.dump.split("\n")) # <<< !!!

      unless env.empty?
        m.print_request(Rack::Request.new(env)) if defined?(Rack::Request)
        m.print_section('Environment', filter_env(env))
      end

      m.print_section('Data', extract_data(env, options))
    end
  end
end

ExceptionNotifier::SimpleEmailNotifier.prepend(SimpleNotifierPatch)

I would advice you to look up alternatives for ExceptionNotifier since this solution looks extremely dirty.

JelF commented 7 years ago

@fiscal-cliff

I would advice you to look up alternatives for ExceptionNotifier since this solution looks extremely dirty.

ExceptionNotifier-related stuff could be moved to rails-only section in this case, same as it's views and rails engine. Also, consider use Lamian.dump instead of Lamian.logger.dump, because Lamian.logger is part of private api and this comands do same stuff

fiscal-cliff commented 7 years ago

OK, that's just kinda demo, proof of concept

On Mon, Jan 16, 2017, 00:14 Alexandr Smirnov notifications@github.com wrote:

@fiscal-cliff https://github.com/fiscal-cliff

I would advice you to look up alternatives for ExceptionNotifier since this solution looks extremely dirty. ExceptionNotifier-related stuff could be moved to rails-only section in this case, same as it's views and rails engine. Also, consider use Lamian.dump instead of Lamian.logger.dump, because Lamian.logger is part of private api and this comands do same stuff

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JelF/lamian/issues/9#issuecomment-272724249, or mute the thread https://github.com/notifications/unsubscribe-auth/AFRVLlYXwIhpDzx5KrPqH-GtOly38Mzpks5rSowzgaJpZM4LY0Gt .

JelF commented 7 years ago

I will not add non-rails support untill i need it in production. However, pull requests are welcome

Core functionality is rails-independent, same is middleware itself. Rails engine should be required in "lamian/rails" and readme should be updated. Exception notifier API is not required in non-rails for oblivious reasons (exception_notifier bound to rails), so i suggest to add API for Raven