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.

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

Add Kafka integration #1070

Merged
merged 4 commits into from
Jul 9, 2020
Merged

Add Kafka integration #1070

merged 4 commits into from
Jul 9, 2020

Conversation

tjwp
Copy link
Contributor

@tjwp tjwp commented Jun 8, 2020

This change adds a kafka integration based on the ruby-kafka gem.

The implementation follows the template of the existing racecar integration which also builds on top of ActiveSupport::Notifications.

The motivation for this change was to provide more detailed information on interactions with Kafka. We use racecar but for publishing we have implemented our own "outbox pattern". This new integration provides more visibility into the message delivery to Kafka.

Support is included for all of the blocks that ruby-kafka instruments. Sorry about the size of the change :)

The events are broken into internal modules based on the ruby-kafka class that emits them. I was originally thinking that the events could be separately enabled based on these modules, but ultimately decided not to implement that.

Currently the span names are prefixed by kafka., e.g. kafka.producer.deliver_messages but I could be convinced to drop that first part of the name.

CC @dasch

@tjwp tjwp requested a review from a team June 8, 2020 12:25
@delner
Copy link
Contributor

delner commented Jun 8, 2020

@tjwp This is awesome! Thanks for putting this together. We'll definitely setup some time to review this as this is something I'd love to add to our library.

Before I get a chance to look deeper in the code, can you explain how ActiveSupport::Notifications plays in? The racecar instrumentation used this because it was the easiest way to hook in and generate spans. But from what I can see, ruby-kafka only has this listed as a development dependency?

@delner delner assigned delner and tjwp Jun 8, 2020
@delner delner self-requested a review June 8, 2020 14:39
@delner delner added community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations labels Jun 8, 2020
@tjwp
Copy link
Contributor Author

tjwp commented Jun 8, 2020

@delner As with racecar this was the easiest way to hook in to generate spans because ruby-kafka already contains instrumentation that is built on ActiveSupport::Notifications: https://github.com/zendesk/ruby-kafka#instrumentation

It is not a hard dependency for ruby-kafka but must required to use any of the provided instrumentation.

@tjwp
Copy link
Contributor Author

tjwp commented Jun 11, 2020

I noticed the changes here #1075 and I've updated this branch to use the new contrib spec helper.

@delner
Copy link
Contributor

delner commented Jun 15, 2020

@tjwp Gotcha, so this integration will only work if both ruby-kafka and activesupport of sufficient version is present?

If this is the case, we might need to make sure that this requirement is reflected appropriately in the Integration such that it'll raise warnings if c.use :kafka is activated but activesupport is not present.

@tjwp
Copy link
Contributor Author

tjwp commented Jun 15, 2020

@tjwp Gotcha, so this integration will only work if both ruby-kafka and activesupport of sufficient version is present?

If this is the case, we might need to make sure that this requirement is reflected appropriately in the Integration such that it'll raise warnings if c.use :kafka is activated but activesupport is not present.

Is there a pattern for this that you can point me to for this? The code in this PR handles it the same way as the existing Racecar integration: https://github.com/DataDog/dd-trace-rb/pull/1070/files#diff-4b58bb2b027c784de6ca989f77238eb9R20-R22

Appraisals Show resolved Hide resolved
docs/GettingStarted.md Show resolved Hide resolved
@tjwp tjwp force-pushed the tjwp-kafka branch 2 times, most recently from db280b4 to 1f190b2 Compare June 17, 2020 10:48
@tjwp
Copy link
Contributor Author

tjwp commented Jun 17, 2020

@marcotc Pushed the updates you mentioned.

It looks like there are some Excon related test failures that would be addressed by: #1083

@marcotc
Copy link
Member

marcotc commented Jun 18, 2020

Looks great @tjwp! Could you rebase with master, since #1086 was merged, which will pick up the latest excon release containing a fix for our CI issues?
We should be ready to merge your PR right after that!

@tjwp
Copy link
Contributor Author

tjwp commented Jun 18, 2020

@marcotc thanks, rebased!

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Thank you very much @tjwp! 🎉

We'll let you know when a release goes out including your changes.

@tjwp
Copy link
Contributor Author

tjwp commented Jul 9, 2020

@marcotc Any update on when this may be included in a release? Thanks!

@marcotc marcotc merged commit 0c7425b into DataDog:master Jul 9, 2020
@marcotc
Copy link
Member

marcotc commented Jul 9, 2020

Hey @tjwp, we are planning on releasing soon, tentatively next week. I'll keep you posted.

Thank you once again for contribution!

@marcotc marcotc added this to the 0.38.0 milestone Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants