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.

Sign up for GitHub

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

Jump to bottom

Karafka rdkafka bugfix #2880

Merged
merged 5 commits into from
Oct 9, 2024
Merged

Karafka rdkafka bugfix #2880

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c7ebec7
add ruby version check
tannalynn Oct 1, 2024
6bddd91
Merge branch 'dev' into rdkafka_karafka_bugfix
tannalynn Oct 3, 2024
17afe8b
add changelog entry
tannalynn Oct 3, 2024
8f77323
Merge branch 'dev' into rdkafka_karafka_bugfix
tannalynn Oct 3, 2024
e890bd0
Update CHANGELOG.md
tannalynn Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
# New Relic Ruby Agent Release Notes

## dev

Version <dev> resolves a bug in rdkafka instrumentation when using the karafka-rdkafka gem.

- **Bugfix: Instrumentation errors when using the karafka-rdkafka gem**

Due to version differences between the rdkafka gem and karafka-rdkafka gem, the agent was installing instrumentation on some versions of karafka-rdkafka that encountered errors. This has now been resolved. Thank you to @krisdigital for bringing this issue to our attention. [PR#2880](https://github.com/newrelic/newrelic-ruby-agent/pull/2880)
tannalynn marked this conversation as resolved.
Show resolved Hide resolved


## v9.14.0

Version 9.14.0 adds Apache Kafka instrumentation for the rdkafka and ruby-kafka gems, introduces a configuration-based, automatic way to add custom instrumentation method tracers, correctly captures MIME type for ActionDispatch 7.0+ requests, properly handles Boolean coercion for `newrelic.yml` configuration, fixes a JRuby bug in the configuration manager, fixes a bug related to `Bundler.rubygems.installed_specs`, and fixes a bug to make the agent compatible with ViewComponent v3.15.0+.
Expand Down
3 changes: 2 additions & 1 deletion lib/new_relic/agent/instrumentation/rdkafka/chain.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'))
Copy link
Contributor Author

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+

def producer(**kwargs)
producer_without_new_relic(**kwargs).tap do |producer|
set_nr_config(producer)
Expand Down
3 changes: 2 additions & 1 deletion lib/new_relic/agent/instrumentation/rdkafka/prepend.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ module RdkafkaConfig
module Prepend
include NewRelic::Agent::Instrumentation::RdkafkaConfig

if defined?(::Rdkafka) && Gem::Version.new(::Rdkafka::VERSION) >= Gem::Version.new('0.16.0')
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'))
Comment on lines +39 to +40
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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:

newrelic-ruby-agent/lib/new_relic/agent/instrumentation/rdkafka.rb

Lines 5 to 18 in b9c118e

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?

Copy link
Contributor Author

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.

def producer(**kwargs)
super.tap do |producer|
set_nr_config(producer)
Expand Down
Loading