Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 3 commits
c7ebec7
6bddd91
17afe8b
8f77323
e890bd0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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+
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 thedefined?
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:
newrelic-ruby-agent/lib/new_relic/agent/instrumentation/rdkafka.rb
Lines 5 to 18 in b9c118e
If we need to check it, should the
chain
file also have adefined?(::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.