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.

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

Unable to construct a default logger of non-default type #10

Closed
tbrosman opened this issue Oct 13, 2013 · 14 comments
Closed

Unable to construct a default logger of non-default type #10

tbrosman opened this issue Oct 13, 2013 · 14 comments

Comments

@tbrosman
Copy link
Contributor

The following line:

Fluent::Logger::FluentLogger.open(CompoundFluentLogger, :host => 'localhost', :port => 24224)

Should instantiate a CompoundFluentLogger and set @@default_logger. Currently a FluentLogger is instantiated instead. This is because LoggerBase#open passes self as the first parameter to Logger#open.

tbrosman added a commit to tbrosman/fluent-logger-ruby that referenced this issue Nov 6, 2013
@tbrosman
Copy link
Contributor Author

tbrosman commented Nov 7, 2013

I'm working on adding test coverage for this issue but I'm having issues getting rspec to run (see https://gist.github.com/tbrosman/7350939).

/usr/lib/ruby/1.9.1/rubygems/custom_require.rb:36:in `require': cannot load such file -- fluent/load (LoadError)

fluent-logger-ruby is at revision 35d7fd0 and fluentd is at revision 4e73be1

Am I missing some dependency?

@repeatedly
Copy link
Member

How about bundle exec rspec?

@tbrosman
Copy link
Contributor Author

tbrosman commented Dec 2, 2013

That worked, thanks! I'm attempting to get rspec passing on existing cases before I add anything new. Now I'm seeing:

vagrant@precise64:/vagrant/fluent-logger-ruby$ bundle exec rspec
Run options: include {:focus=>true}

All examples were filtered out; ignoring {:focus=>true}
...............F....2013-12-02 06:45:57 +0000 [trace]: plugin/in_forward.rb:152:initialize: accepted fluent socket objec
t_id=16657380.
........

Failures:

  1) Fluent::Logger::FluentLogger not running fluentd fluent logger interface log connect error once
     Failure/Error: logger_io.read.should =~ /Can't connect to/
       expected: /Can't connect to/
            got: "E, [2013-12-02T06:45:57.654985 #7261] ERROR -- : Failed to connect fluentd: Connection refused - conne
ct(2)\nE, [2013-12-02T06:45:57.655065 #7261] ERROR -- : Connection will be retried.\nD, [2013-12-02T06:45:57.656463 #726
1] DEBUG -- : event: tag {\"a\":\"b\"}\nD, [2013-12-02T06:45:57.757037 #7261] DEBUG -- : event: tag {\"a\":\"b\"}\n" (us
ing =~)
       Diff:
       @@ -1,2 +1,5 @@
       -/Can't connect to/
       +E, [2013-12-02T06:45:57.654985 #7261] ERROR -- : Failed to connect fluentd: Connection refused - connect(2)
       +E, [2013-12-02T06:45:57.655065 #7261] ERROR -- : Connection will be retried.
       +D, [2013-12-02T06:45:57.656463 #7261] DEBUG -- : event: tag {"a":"b"}
       +D, [2013-12-02T06:45:57.757037 #7261] DEBUG -- : event: tag {"a":"b"}

     # ./spec/fluent_logger_spec.rb:239:in `block (4 levels) in <top (required)>'

Finished in 10.11 seconds
29 examples, 1 failure

Failed examples:

rspec ./spec/fluent_logger_spec.rb:229 # Fluent::Logger::FluentLogger not running fluentd fluent logger interface log co
nnect error once
2013-12-02 06:45:57 +0000 [trace]: plugin/in_forward.rb:193:on_close: closed fluent socket object_id=16657380

I'm assuming the regex is out of date, but even after changing it to /Failed to connect/ it still fails:

vagrant@precise64:/vagrant/fluent-logger-ruby$ bundle exec rspec
Run options: include {:focus=>true}

All examples were filtered out; ignoring {:focus=>true}
...............F...2013-12-02 06:48:00 +0000 [trace]: plugin/in_forward.rb:152:initialize: accepted fluent socket object
_id=15761840.
.........

Failures:

  1) Fluent::Logger::FluentLogger not running fluentd fluent logger interface log connect error once
     Failure/Error: logger.should_receive(:log_reconnect_error).once.and_call_original
       (#<Fluent::Logger::FluentLogger:0x000000029d01a0>).log_reconnect_error(any args)
           expected: 1 time with any arguments
           received: 0 times with any arguments
     # ./spec/fluent_logger_spec.rb:232:in `block (4 levels) in <top (required)>'

Finished in 10.11 seconds
29 examples, 1 failure

Failed examples:

rspec ./spec/fluent_logger_spec.rb:229 # Fluent::Logger::FluentLogger not running fluentd fluent logger interface log co
nnect error once
2013-12-02 06:48:00 +0000 [trace]: plugin/in_forward.rb:193:on_close: closed fluent socket object_id=15761840

Is there some change that could cause this?

@repeatedly
Copy link
Member

I just disabled this spec and other maintainer will fix.
Please rebase the commit :)

@tbrosman
Copy link
Contributor Author

I rebased and now logger_base_spec.rb fails (with my fix; without it works normally). I see that *args is nil, where is open being called from in this spec? Is there a mock somewhere I'm not seeing? Callstack:

  1) Fluent::Logger::LoggerBase subclass open
     Failure/Error: should be_kind_of(Fluent::Logger::LoggerBase)
     ArgumentError:
       wrong number of arguments (0 for 1)
     # ./lib/fluent/logger/fluent_logger.rb:58:in `initialize'
     # ./lib/fluent/logger/fluent_logger.rb:33:in `block in new'
     # ./lib/fluent/logger/fluent_logger.rb:33:in `instance_eval'
     # ./lib/fluent/logger/fluent_logger.rb:33:in `new'
     # ./lib/fluent/logger.rb:37:in `new'
     # ./lib/fluent/logger.rb:42:in `open'
     # ./lib/fluent/logger/logger_base.rb:23:in `open'
     # /var/lib/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/memoized_helpers.rb:434:in `block (3 levels) in its'
     # /var/lib/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/memoized_helpers.rb:433:in `each'
     # /var/lib/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/memoized_helpers.rb:433:in `inject'
     # /var/lib/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/memoized_helpers.rb:433:in `block (2 levels) in its'
     # /var/lib/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/memoized_helpers.rb:199:in `block (2 levels) in let'
     # /var/lib/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/memoized_helpers.rb:199:in `fetch'
     # /var/lib/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/memoized_helpers.rb:199:in `block in let'
     # /var/lib/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/memoized_helpers.rb:440:in `should'
     # ./spec/logger_base_spec.rb:8:in `block (3 levels) in <top (required)>'

Edit Ah, I see, open is called by the spec. This raises the question does it make sense for FluentLoggerBase#open to be called with no parameters? I guess for the sake of backwards compatibility it's irrelevant and should continue to work as-is.

@tbrosman
Copy link
Contributor Author

It looks like FluentLogger#new doesn't actually return a FluentLogger (it returns a Delegator). Should it return a FluentLogger instead? (The Delegator doesn't appear to be used past calling #new). Alternatively, if returning the Delegator is intended, should the spec handle this case or be removed?

@repeatedly
Copy link
Member

How about another method approach like open_with_logger method?
I'm not sure why FluentLogger uses complex initialization mechanizm.

@hotchpotch
Copy link
Member

@tbrosman What version of ruby?

$ ruby --version

@tbrosman
Copy link
Contributor Author

tbrosman commented Feb 9, 2014

My ruby version:
ruby 1.9.3p0 (2011-10-30 revision 33570) [x86_64-linux]

If you want to see what my environment looks like, I'm using this Vagrantfile: https://gist.github.com/tbrosman/7367254

I'm assuming you mean add open_with_logger to Logger and change new to only return one type of logger (FluentLogger), right? Will this break any backwards compatibility? (Specifically do end users normally call Logger#new directly for any reason? I didn't have to when I used it.)

@repeatedly
Copy link
Member

Hmm... I have another idea. Replaces weird Finalizable module with at_exit.
I did same change in td-logger-ruby.

treasure-data/td-logger-ruby@a72cc13

It is more safer and readable.

@repeatedly
Copy link
Member

@tbrosman I just merged #15. Could you rebase the your branch and retry spec?

@tbrosman
Copy link
Contributor Author

Thanks, that fixed it!

One more (tiny) issue: in my previous comment (#10 (comment)) I was attempting to construct FluentLogger without parameters (as apparently other logger types can, otherwise the spec would never have passed in the first place). My workaround is to have a default tag FluentLogger would be constructed with if no parameters were specified. Any preference on what that should be? (An empty string?)

@repeatedly
Copy link
Member

Sorry for the delay.

I think empty string is enough.
We can specify full tag with post method.

tbrosman added a commit to tbrosman/fluent-logger-ruby that referenced this issue Mar 29, 2014
tbrosman added a commit to tbrosman/fluent-logger-ruby that referenced this issue Mar 29, 2014
tbrosman added a commit to tbrosman/fluent-logger-ruby that referenced this issue Apr 12, 2014
repeatedly added a commit that referenced this issue Apr 12, 2014
@tbrosman
Copy link
Contributor Author

Closing. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants