-
Notifications
You must be signed in to change notification settings - Fork 339
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 producer consumer interceptors #837
Conversation
c26c1e6
to
a57700e
Compare
ping @dasch to see if he can schedule some time to review this, thanks! |
We already use |
@dasch with interceptors we can add the span and trace ID to the message headers so that traces emitted by producers can be linked by the consumer. |
@vvuibert ah OK, that’s a good use case, and something we need at Zendesk as well. I’ll review this then. |
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.
In general, I think this implementation follows the Java design too closely. ruby-kafka tries to follow Ruby idioms to the degree possible.
To me, that means dropping a class hierarchy in favor of either a simple contract with a couple of expected methods on the objects being passed or, better yet, a simplified API consisting of only callables.
Specifically, unless there’s an actual need for a close
method, we could do away with having separate concepts for producer and consumer interceptors, instead just expecting the passed objects to have a call
method. This allows passing in arbitrary lambdas, procs, method objects, etc., and is the typical way to do it in Ruby.
We could take that a step further and allow setting these in a simple DSL, e.g.
producer = kafka.producer(...)
producer.on_send do |message|
# mutate message here!
end
consumer = kafka.consumer(...)
consumer.on_batch do |batch|
# do you stuff here!
end
consumer.on_message do |message|
# simplified API for per-message stuff!
end
a57700e
to
3f30f58
Compare
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.
I still think we need to simplify the API and have just a single container for the interceptor lists.
3f30f58
to
3232f8e
Compare
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.
Awesome! Just a little spit and polish and I’ll be happy to merge!
Thanks for the contribution!
linter asyn producer interceptor nil consumer docs typo error details pending message revert lint empty interceptor linter reviews1 intercept batch test error reviews2 interceptors typos extra line rm nil check more reviews linter functional test revert dockercompose rm close revert docker compose hoost more tests rm close and base class single interceptors class typo
3232f8e
to
efb67e3
Compare
Thanks! |
Thanks @dasch for the detailed review! I wonder what are the plans for an upcoming release that includes this change. I see there are some issue with the We'll keep and 👀 on new releases! Thanks |
Yeah, hoping to get that resolved before issuing a new release. There’s no firm cycle. |
Addressing that issue would certainly expedite a new release :D |
Add producer and consumer interceptors that can intercept messages at different points on producer and consumer in order to trace the path of individual messages across the cluster.