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

feat: ruby-kafka instrumentation #476

Merged
merged 35 commits into from
Dec 24, 2020
Merged

feat: ruby-kafka instrumentation #476

merged 35 commits into from
Dec 24, 2020

Conversation

robertlaurin
Copy link
Contributor

@robertlaurin robertlaurin commented Nov 18, 2020 •
edited
Loading

Todo:

  • Getting kafka working in ci
  • Add readme
  • Add examples
  • Configure appraisals and minimum version supported
  • Test with consumer

@robertlaurin robertlaurin self-assigned this Nov 18, 2020
@robertlaurin robertlaurin force-pushed the ruby-kafka-instrumentation branch 7 times, most recently from 092fb97 to 09c98d0 Compare November 19, 2020 18:28
@robertlaurin robertlaurin force-pushed the ruby-kafka-instrumentation branch 15 times, most recently from 27b8614 to 32337c8 Compare November 20, 2020 21:30
robertlaurin
robertlaurin commented Nov 20, 2020
View reviewed changes
Copy link
Contributor Author

@robertlaurin robertlaurin left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this out of draft to get some reviews in, I don't feel that this is 100% yet but I think having a review will speed things up a bit.

Review ready.

instrumentation/ruby_kafka/opentelemetry-instrumentation-ruby_kafka.gemspec Outdated Show resolved Hide resolved
instrumentation/ruby_kafka/Appraisals Show resolved Hide resolved
...ntation/ruby_kafka/lib/opentelemetry/instrumentation/ruby_kafka/events/connection/request.rb Outdated Show resolved Hide resolved
...n/ruby_kafka/lib/opentelemetry/instrumentation/ruby_kafka/events/consumer/process_message.rb Outdated Show resolved Hide resolved
...ion/ruby_kafka/lib/opentelemetry/instrumentation/ruby_kafka/events/consumer/process_batch.rb Outdated Show resolved Hide resolved
instrumentation/ruby_kafka/lib/opentelemetry/instrumentation/ruby_kafka/patches/client.rb Show resolved Hide resolved
@robertlaurin robertlaurin marked this pull request as ready for review November 20, 2020 21:43
@robertlaurin
Copy link
Contributor Author

Updated to use links in this commit 8a1ef1b

fbogsany
fbogsany reviewed Dec 24, 2020
View reviewed changes
...umentation/ruby_kafka/test/opentelemetry/instrumentation/ruby-kafka/patches/consumer_test.rb Outdated
@counter = 0
consumer.each_message do |_msg|
@counter += 1
raise 'oops' if @counter >= 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I guess it is just to terminate the each_message loop. Does it need to be a member variable? Wouldn't a local be sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A local is sufficient, I think there was a reason why it was this way before but it's no longer valid. Updated!

robertlaurin and others added 2 commits December 24, 2020 11:31
@robertlaurin
fix: stop using member variable in test
aa03504
@robertlaurin @fbogsany
fix: typo in test comment
f1a80a6 
Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
robertlaurin and others added 2 commits December 24, 2020 13:03
@robertlaurin @fbogsany
fix: use span context in links
221893d 
Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
@robertlaurin @fbogsany
fix: use links properly in each_message
6f0f73c 
Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
robertlaurin and others added 2 commits December 24, 2020 13:26
@robertlaurin @fbogsany
Update instrumentation/ruby_kafka/test/opentelemetry/instrumentation/…
5a8c452 
…ruby-kafka/patches/consumer_test.rb

Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
@robertlaurin
fix: span link tests
b56367d
fbogsany
fbogsany approved these changes Dec 24, 2020
View reviewed changes
Copy link
Contributor

@fbogsany fbogsany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, other than appeasing Rubocop.

Thanks @robertlaurin! I know it was challenging to get this over the finish line.

@fbogsany fbogsany merged commit cbe9ce4 into open-telemetry:master Dec 24, 2020
@robertlaurin robertlaurin deleted the ruby-kafka-instrumentation branch December 24, 2020 20:20
@fbogsany fbogsany mentioned this pull request Dec 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnnyshields johnnyshields johnnyshields left review comments

@d1egoaz d1egoaz d1egoaz left review comments

@fbogsany fbogsany fbogsany approved these changes

@mwear mwear mwear left review comments

@ericmustin ericmustin ericmustin left review comments

@dazuma dazuma Awaiting requested review from dazuma

Assignees

@robertlaurin robertlaurin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@robertlaurin @fbogsany @johnnyshields @d1egoaz @mwear @ericmustin