fix: suppress deprecation warning in Rdkafka Instrumentation #884
Conversation
yoheyk
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle and
xuan-cao-swi
as code owners
| @@ -74,6 +74,14 @@ def extract_message_key(key) | |||
| rescue Encoding::UndefinedConversionError | |||
| nil | |||
| end | |||
|
|
|||
| def getter | |||
| if Gem::Version.new(::Rdkafka::VERSION) >= Gem::Version.new('0.13.0') | |||
There was a problem hiding this comment.
Is there a way to check/set this only once when the gem is activated as opposed to each time a message is processed?
There was a problem hiding this comment.
we could memoize like 661cd67 or do you think it's better to use a constant like this?
GETTER = if Gem::Version.new(::Rdkafka::VERSION) >= Gem::Version.new('0.13.0')
Context::Propagation.text_map_getter
else
OpenTelemetry::Common::Propagation.symbol_key_getter
end
private_constant :GETTER(btw we still need to support rdkafka 0.12.0, right?)
There was a problem hiding this comment.
fyi, 0.12.0 is released on June 17, 2022, and the latest version of rdkafka is 0.15.1 as of now
There was a problem hiding this comment.
The general rule is for any gems we try to support any non-EoL versions, however since all of these are pre 1.0 we have more flexibility and may choose to drop support for earlier versions at any time.
I also usually check with the original contributor of this instrumentation to provide a review and get their feedback on whether or not it makes sense to continue to support older versions.
We will generally provide guidance on pinning to an older version of the instrumentation in the README.
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
| @@ -74,6 +74,14 @@ def extract_message_key(key) | |||
| rescue Encoding::UndefinedConversionError | |||
| nil | |||
| end | |||
|
|
|||
| def getter | |||
| @getter ||= if Gem::Version.new(::Rdkafka::VERSION) >= Gem::Version.new('0.13.0') | |||
There was a problem hiding this comment.
I'm very sorry for the delayed review.
Is this something that can be checked at installation time as opposed to when the worker processes the first message?
There was a problem hiding this comment.
how about this? 21e4a43
arielvalentin
added
keep
Background
Observing the following warning with rdkafka
0.13.0and above:https://github.com/open-telemetry/opentelemetry-ruby-contrib/actions/runs/7980847049/job/21791353080#step:4:224
This warning is coming from here:
https://github.com/karafka/rdkafka-ruby/blob/v0.14.0/lib/rdkafka/consumer/headers.rb#L10-L11
Changes summary
Use
Context::Propagation.text_map_getterwith rdkafka0.13.0and above.if we can drop support for rdkafka
0.12.0, it's going to be much simpleropentelemetry-ruby-contrib/instrumentation/rdkafka/opentelemetry-instrumentation-rdkafka.gemspec
Line 39 in c95e919