Adds instrumentation for rdkafka #978
Conversation
andyfleming
requested review from
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
mwear and
robertlaurin
as code owners
instrumentation/rdkafka/opentelemetry-instrumentation-rdkafka.gemspec
Outdated
Show resolved
Hide resolved
…gemspec Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
instrumentation/rdkafka/lib/opentelemetry/instrumentation/rdkafka/patches/consumer.rb
Show resolved
Hide resolved
…fka/patches/consumer.rb
instrumentation/rdkafka/lib/opentelemetry/instrumentation/rdkafka/patches/consumer.rb
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks @andyfleming for your contribution!
I left a few questions and comments for you to review.
instrumentation/rdkafka/opentelemetry-instrumentation-rdkafka.gemspec
Outdated
Show resolved
Hide resolved
instrumentation/rdkafka/lib/opentelemetry/instrumentation/rdkafka/instrumentation.rb
Show resolved
Hide resolved
| module Consumer | ||
| def each | ||
| super do |message| | ||
| attributes = { |
There was a problem hiding this comment.
Although it is not heavily in use in other instrumentations, I would ask you to consider using the Semantic Conventions Gem to reference these keys.
There was a problem hiding this comment.
I don't think we're using it in any other instrumentation gem. Should we just do a wholesale swap to it in another PR? Did we decide if we wanted every instrumentation gem to use this? I think yes.
instrumentation/rdkafka/test/opentelemetry/instrumentation/rdkafka/patches/consumer_test.rb
Outdated
Show resolved
Hide resolved
| module Rdkafka | ||
| module Patches | ||
| # CustomerGetter for needed for propagation | ||
| class CustomGetter |
There was a problem hiding this comment.
@open-telemetry/ruby-maintainers Does it make sense to extract this into a SymbolMapGetter and make it part of higher level package?
There was a problem hiding this comment.
Yeah we should definitely extract it out, seems like it would be something that could get re-used again in the future.
We have the rack one here. I'm guessing this could live in the same folder under the api as well.
There was a problem hiding this comment.
I'm kind of wishing we didn't include that in the API gem 😞 . I know it's convenient. I feel like it belongs in a convenience gem, like opentelemetry-common.
Co-authored-by: Ariel Valentin <arielvalentin@users.noreply.github.com>
Co-authored-by: Andy Fleming <89227684+andyfleming-shopify@users.noreply.github.com>
|
Thanks all for the comments. I think I've addressed all of the open issues. 👍 |
| if error | ||
| span.record_exception(error) | ||
| span.status = OpenTelemetry::Trace::Status.error("Unhandled exception of type: #{e.class}") | ||
| end |
There was a problem hiding this comment.
I'm not sure this is correct. IIUC, the error here comes from the receive operation. It isn't logically part of the process operation. I think we just want the default error handling behaviour from in_span here.
| if error | |
| span.record_exception(error) | |
| span.status = OpenTelemetry::Trace::Status.error("Unhandled exception of type: #{e.class}") | |
| end |
instrumentation/rdkafka/opentelemetry-instrumentation-rdkafka.gemspec
Outdated
Show resolved
Hide resolved
instrumentation/rdkafka/lib/opentelemetry/instrumentation/rdkafka/patches/consumer.rb
Show resolved
Hide resolved
|
There's a bunch of Rubocop failures 😞 |
…o add-rdkafka-instrumentation
|
Feedback addressed and lint errors resolved 👍 |
…opentelemetry-ruby into add-rdkafka-instrumentation
|
It looks like rdkafka-ruby is not supported on windows. |
Closes #919.