-
Notifications
You must be signed in to change notification settings - Fork 599
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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+
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
DependencyDetection.defer do | |
named :rdkafka | |
depends_on do | |
defined?(Rdkafka) | |
end | |
executes do | |
NewRelic::Agent.logger.info('Installing rdkafka instrumentation') | |
require_relative 'rdkafka/instrumentation' | |
require_relative 'rdkafka/chain' | |
require_relative 'rdkafka/prepend' | |
If we need to check it, should the chain
file also have a defined?(::Rdkafka)
check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
DependencyDetection.defer do | |
named :rdkafka | |
depends_on do | |
defined?(Rdkafka) | |
end | |
executes do | |
NewRelic::Agent.logger.info('Installing rdkafka instrumentation') | |
require_relative 'rdkafka/instrumentation' | |
require_relative 'rdkafka/chain' | |
require_relative 'rdkafka/prepend' | |
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