actindi / act-fluent-logger-rails

Fluent logger
MIT License
118 stars 72 forks source link

@logger.flush issue #1

Closed davidrenne closed 11 years ago

davidrenne commented 11 years ago

Great GEM! It solves the core level issues inside of fluent-logger-rails

https://github.com/huacnlee/fluent-logger-rails

But now I had to hack your gem to get it to call @logger.flush.

def add(severity, message = nil, progname = nil, &block)
  return true if severity < @level
  message = (block_given? ? block.call : progname) if message.blank?
  return true if message.blank?
  if message.match /^Completed/
    @logger.flush
  end
  @logger.add_message(severity, message)
  true
end

I copied and pasted the regex from huacnlee's

  if message.match /^Completed/
    @logger.flush
  end

This allowed the fluent D server to log the information.

Please let me know how you can merge this into your master branch and cut a new gem?

def tagged(*tags)
  super(*tags)
ensure
  @logger.flush
end

The tagged function never gets called in my rails application. Do you have any idea why? Maybe if you can explain, then I can just change my application so that no changes need to go into 0.0.4 of your next gem release.

Maybe you can support your localhost environment where tagged is called and my environment where tagged def is not called where you can merely match /^Completed/ like the repository you forked to create the rubygem?

I cannot use either your gem or huacnlee's github because neither are working exactly correct.

Let me know.

Also take a look at my first GEM.

https://github.com/davidrenne/widget_list

https://github.com/davidrenne/widget_list_example

quek commented 11 years ago

Thank's for your report.

I think ActFluentLoggerRails::Logger#tagged is called from Rails::Rack::Logger#call.

    def call(env)
      request = ActionDispatch::Request.new(env)

      if Rails.logger.respond_to?(:tagged)
        Rails.logger.tagged(compute_tags(request)) { call_app(request, env) }
      else
        call_app(request, env)
      end
    end

Can you see "use Rails::Rack::Logger" in output of 'rake middleware' command?

$ rake middleware
use ActionDispatch::Static
use Rack::Lock
use #<ActiveSupport::Cache::Strategy::LocalCache::Middleware:0x000000030abec0>
use Rack::Runtime
use Rack::MethodOverride
use ActionDispatch::RequestId
use Rails::Rack::Logger
use ActionDispatch::ShowExceptions
use ActionDispatch::DebugExceptions
use ActionDispatch::RemoteIp
use ActionDispatch::Reloader
use ActionDispatch::Callbacks
use ActiveRecord::ConnectionAdapters::ConnectionManagement
use ActiveRecord::QueryCache
use ActionDispatch::Cookies
use ActionDispatch::Session::CookieStore
use ActionDispatch::Flash
use Jpmobile::Rack::MobileCarrier
use Jpmobile::Rack::ParamsFilter
use Jpmobile::Rack::Filter
use ActionDispatch::ParamsParser
use Remotipart::Middleware
use ActionDispatch::Head
use Rack::ConditionalGet
use Rack::ETag
use ActionDispatch::BestStandardsSupport
run Outing::Application.routes
davidrenne commented 11 years ago

OK so after some more debugging it looks like your gem just doesnt support the case where @tags is nil.

the else will fall into call_app(env)

In this case Rails.logger.tagged() is never called.

Maybe you can check to see if @tags is nil and then perform the block of code to flush

if message.match /^Completed/ @logger.flush end

davidrenne commented 11 years ago

Looking further into what tags are:

Adding

config.log_tags = ["test"]

To development.rb allows your GEM to work.

You should really add that to your gem readme.

Funny enough, the word "test" doesnt show up in my fluentD logs. You would think it would prepend the word test to each log entry.

quek commented 11 years ago

I see now. I use Rails 3.2.11. https://github.com/rails/rails/blob/v3.2.11/railties/lib/rails/rack/logger.rb Even if you don't set config.log_tags, it calls Rails.logger.tagged. Could you update Rails 3.2.11?

davidrenne commented 11 years ago

no we cant upgrade our application quickly to new versions of rails without heavy regression tests.

I think for now I will just use the config.log_tags = ['nothing'] on version 0.0.4 once it becomes built and pushed to rubygems.org

config.log_tags = ['nothing'] will allow your gem to work with older versions. Feel free to test out locally on 3.2.3 rails.

Switching to rails 3.2.11 locally worked with "log_tags" configuration removed, but since 3.2.3 is not that far behind and many rails applications might be on older versions. Several people using this gem will see no logs unless they dive deeper into why it isnt sending to the fluent-d server to determine what is happening with tags.

I would definitely update the readme "Usage" section to include config.log_tags so developers can be up and running quicker and have it working and not second guess the GEM just in case they are on older versions or dont currently use the log_tags.

Thanks again.

quek commented 11 years ago

I included config.log_tags in v0.0.4 README. Thanks.