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 non-blocking send to kafka provider #149

Closed

Conversation

nemanjamikic
Copy link

Because zipkin is not critical component if kafka producer queue is full all messages should be logged and ignored. This is optional feature.

@coveralls
Copy link

coveralls commented Oct 2, 2019

Coverage Status

Coverage decreased (-2.8%) to 72.599% when pulling a97fc6c on nemanjamikic:feature/non-blocking-send into c29478e on openzipkin:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-14.8%) to 60.653% when pulling 6989a5b on nemanjamikic:feature/non-blocking-send into c29478e on openzipkin:master.

@nemanjamikic nemanjamikic force-pushed the feature/non-blocking-send branch 2 times, most recently from 7f8938a to 8a228a0 Compare October 2, 2019 16:48
@jcchavezs
Copy link
Contributor

jcchavezs commented Oct 21, 2019

Hi @nemanjamikic, thanks for the contribution and sorry for the delay in answering. I am curious about the options Config.Producer.Timeout from Sarama library. Will that do the trick instead? So as far as I see the main issue is that when kafka is not responding, the messages get stuck and the queue grows up because the buffered channel blocks. Would it be better if one decides the timeout so that the requests to kafka don't get stuck for 10s (default in Sarama).

What do you think?

cc @jeqo @basvanbeek @adriancole

@nemanjamikic
Copy link
Author

nemanjamikic commented Oct 21, 2019

Hi @jcchavezs , thank you for responding to issue.
We solved current issue with Config.Producer.Timeout set to 1s. However, i think that zipkin should not depend how client is configured because it can do harm if misconfigured on production server.

@jcchavezs
Copy link
Contributor

Hi @nemanjamikic I have mixed feelings here. While I believe what you say it's true, I am ITOH not sure about the fact that one accept the Producer as part of the properties of the reporter (see https://github.com/openzipkin/zipkin-go/blob/master/reporter/kafka/kafka.go#L56) but don't fully respect the configurations on it.

Any input @adriancole @basvanbeek

@basvanbeek
Copy link
Member

I don't see a technical problem to solve here only a documentation / expectation issue. I suggest we handle this by improving documentation and probably an example

Copy link
Member

@basvanbeek basvanbeek left a comment

Choose a reason for hiding this comment

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

Instead of importing the concern of Sarama configuration into the Zipkin Go package I'd much rather see a fix of documentation as well as addition of proper usage of this reporter in an example.

@jcchavezs
Copy link
Contributor

Closed in favour of #158. Still thanks a lot for the initiative @nemanjamikic!

@jcchavezs jcchavezs closed this Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants