Karafka rdkafka bugfix #2880
Karafka rdkafka bugfix #2880
Conversation
| @@ -40,7 +40,8 @@ def each(**kwargs) | |||
| alias_method(:producer_without_new_relic, :producer) | |||
| alias_method(:consumer_without_new_relic, :consumer) | |||
|
|
|||
| if Gem::Version.new(::Rdkafka::VERSION) >= Gem::Version.new('0.16.0') | |||
| if Gem::Version.new(::Rdkafka::VERSION) >= Gem::Version.new('0.16.0') || | |||
| (Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.7.0')) | |||
There was a problem hiding this comment.
This checks the ruby version because on 2.7+, if kwargs is just empty it will also work with it if 0 params are defined, so now the kwargs version will always be used if it's on 2.7+
tannalynn
requested review from
fallwith,
hannahramadan and
kaylareopelle
as code owners
| if (defined?(::Rdkafka) && Gem::Version.new(::Rdkafka::VERSION) >= Gem::Version.new('0.16.0')) || | ||
| (Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.7.0')) |
There was a problem hiding this comment.
@tannalynn Should the defined?(::Rdkafka) check be applied no matter what? With this change, a Ruby version over 2.7.0 will cause the defined? check to be skipped.
There was a problem hiding this comment.
Yeah skipping the defined is cool for the ruby version check, the reason it's needed where it is currently, is because we use Rdkafka to get the gem version and that would raise an error if its not defined.
There was a problem hiding this comment.
Do we need to check if Rdkafka is defined?
I thought this file would only load if it's already defined because of the dependency detection check:
If we need to check it, should the chain file also have a defined?(::Rdkafka) check?
There was a problem hiding this comment.
When the agent is running normally it's not a problem, this is something I did in order to fix a test that was failing related to loading instrumentation. #2849
IIRC, that test will load every single file and make sure no errors are thrown when loading it, so of course this fails because it assumes rdkafka is present already. Adding the defined check was the simplest path forward with no real side effect since it was just an extra check one time on start up.
Also, It's not needed in chain because in the chain file, that code is in a method that gets called later, so it works differently than prepend.
| if (defined?(::Rdkafka) && Gem::Version.new(::Rdkafka::VERSION) >= Gem::Version.new('0.16.0')) || | ||
| (Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.7.0')) |
There was a problem hiding this comment.
Do we need to check if Rdkafka is defined?
I thought this file would only load if it's already defined because of the dependency detection check:
If we need to check it, should the chain file also have a defined?(::Rdkafka) check?
Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
SimpleCov Report
|
The rdkafka instrumentation gets installed on both the rdkafka gem and the karafka-rdkafka gem because it is a forked versions or rdkafka. However, they introduced a change in different versions that requires instrumentation to be installed different.
resolves #2879