phobos icon indicating copy to clipboard operation
phobos copied to clipboard

create_kafka_client leads to: undefined method `formatter=' for #<Logging::Logger:0x000055aa7dcb8ae0> (NoMethodError)

Open olleolleolle opened this issue 5 years ago • 14 comments

This Issue is about compatibility re: what is passed into ruby-kafka:

      with a delivery interval set
#<Thread:0x000055aa7e6877d8@/usr/local/bundle/gems/logging-2.2.2/lib/logging/diagnostic_context.rb:471 run> terminated with exception (report_on_exception is true):
Traceback (most recent call last):
	7: from /usr/local/bundle/gems/logging-2.2.2/lib/logging/diagnostic_context.rb:474:in `block in create_with_logging_context'
	6: from /opt/phobos/spec/lib/phobos/producer_spec.rb:142:in `block (6 levels) in <top (required)>'
	5: from /opt/phobos/lib/phobos/producer.rb:85:in `create_async_producer'
	4: from /opt/phobos/lib/phobos.rb:53:in `create_kafka_client'
	3: from /usr/local/bundle/gems/ruby-kafka-0.7.6/lib/kafka.rb:366:in `new'
	2: from /usr/local/bundle/gems/ruby-kafka-0.7.6/lib/kafka.rb:366:in `new'
	1: from /usr/local/bundle/gems/ruby-kafka-0.7.6/lib/kafka/client.rb:75:in `initialize'
/usr/local/bundle/gems/ruby-kafka-0.7.6/lib/kafka/tagged_logger.rb:57:in `new': undefined method `formatter=' for #<Logging::Logger:0x000055aa7dcb8ae0> (NoMethodError)
Did you mean?  formatter

Originally posted by @olleolleolle in https://github.com/phobos/phobos/pull/110#issuecomment-482020383

olleolleolle avatar Apr 11 '19 08:04 olleolleolle

Snooping around notes

Hm, #formatter= is not a part of the generic API: (Narrator: "It was.")

https://ruby-doc.org/stdlib-2.6.2/libdoc/logger/rdoc/Logger.html#top

#formatter is added by this conditional include: https://github.com/TwP/logging/blob/master/lib/logging/rails_compat.rb

olleolleolle avatar Apr 11 '19 08:04 olleolleolle

It is: You can use formatter= for escaping all data. - from the 2.6.2 docs.

mensfeld avatar Apr 11 '19 08:04 mensfeld

@mensfeld ~Ah, I ought to make a tiny docs amendment to the stdlib.~ (Update: 👓 Now I see why I didn't find it immediately: Attributes are not listed among the methods, in the handy sidebar.)

olleolleolle avatar Apr 11 '19 09:04 olleolleolle

This is actually more of an issue with ruby-kafka - this is my fault since a PR I wrote which was merged seems to be causing some of these crashes. We'll likely be pulling that out and trying it from a different perspective.

dorner avatar Apr 11 '19 13:04 dorner

We could add ruby-kafka <= $someVersion to the gemspec until this issue is sorted.

klippx avatar Apr 11 '19 15:04 klippx

Any updates? I just work around this issue by disabling ruby-kafka logs.

In config/phobos.yml.

  # Comment the block to disable ruby-kafka logs
  #ruby_kafka:
  #  level: error

peco8 avatar Jun 17 '19 09:06 peco8

There are a couple of workarounds. Another one is to monkey-patch the logging logger so it actually conforms to the Logger interface:

module Logging
  class Logger
    def formatter=(*args); end
  end
end

You can also manually configure Phobos with a custom logger:

Phobos.configure do |c| 
  c.custom_logger = Logger.new(STDOUT)
  c.custom_kafka_logger = Logger.new(STDOUT)
end

The monkey patch is included in this PR: https://github.com/phobos/phobos/pull/112 and a full fix is merged into ruby-kafka but not released yet.

dorner avatar Jun 17 '19 13:06 dorner

Can we create a new issue to track removing the monkey patch?

We can make the monkey patch an optional thing Phobos users can opt in to.

klippx avatar Jun 18 '19 07:06 klippx

Sure, I can make an issue to remove it once RubyKafka does the next release. Honestly the monkey patch is incredibly benign (it just defines a couple of no-op methods).

dorner avatar Jun 18 '19 14:06 dorner

@dorner have you reproduced this issue? And verified #112 solves this issue?

Would it be possible for @peco8 and/or @olleolleolle to verify that the branch cache-sync-producer solves this issue?

klippx avatar Jun 19 '19 09:06 klippx

@mriska Perhaps you can verify the solving of this?

(Thanks for asking, but I no longer can check.)

olleolleolle avatar Jun 19 '19 11:06 olleolleolle

@klippx it's easy to confirm the fix - if you run bundle upgrade ruby-kafka and run rspecs on Phobos, they'll all crash. With the monkey patch in place they'll succeed.

dorner avatar Jun 19 '19 13:06 dorner

I don't think the patch #112 resolves https://github.com/zendesk/ruby-kafka/pull/732

Should we make Logging::Logger inherit from ::Logger?

capripot avatar Jul 25 '19 21:07 capripot

We can't do that unfortunately. I've added one more patch to RubyKafka - here's hoping it will fix it for good and all. https://github.com/zendesk/ruby-kafka/pull/762

dorner avatar Jul 26 '19 17:07 dorner