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

Expose request latency metric #766

Merged
merged 1 commit into from
Nov 22, 2016

Conversation

slaunay
Copy link
Contributor

@slaunay slaunay commented Oct 17, 2016

The Java client exposes two metrics about request latency (request-latency-avg as the average request latency in ms and request-latency-max as the maximum request latency in ms).
This metric is extremely valuable when optimizing the communication between a remote producer and a Kafka cluster through TCP tuning, typically when the socket buffers are too small on either side to reach the bandwidth-delay product of the connection.

Code change:

  • new request-latency-in-ms histogram metric (global and per registered broker)
  • addng 10ms latency to brokers in producer functional tests for validation
  • updating documentation

@eapache
Copy link
Contributor

eapache commented Oct 24, 2016

Looks good to me. @wvanbergen ?

@wvanbergen
Copy link
Contributor

Do we care that time.Now() is not monotonic? (golang/go#12914)

Other than that - LGTM.

@slaunay
Copy link
Contributor Author

slaunay commented Oct 25, 2016

Non monotonic clock can lead to negative or inaccurate latency metric which might not be that big of deal I believe and it does not seem that there is an easy and portable way around it in Go.

time.Now() is also used when calling Set[Read|Write]Deadline on a TCP connection.

@eapache
Copy link
Contributor

eapache commented Nov 22, 2016

I tried to rebase this myself but got confused trying to merge the documentation... I think the name column currently in master is more correct?

@slaunay
Copy link
Contributor Author

slaunay commented Nov 22, 2016

I did remove the type prefix typo (histogram) on the other PR and had to fix indentation on this one, so none of them are more correct than the other 😞.

AFAIK the only way around is to resolve the conflicts manually as the changes are on the same block of lines so I will rebase the changes against master...

@slaunay slaunay force-pushed the feature/request-latency-metric branch from 88d47e8 to 3005dec Compare November 22, 2016 17:53
- new request-latency-in-ms histogram metric (global and per registered broker)
- addng 10ms latency to brokers in producer functional tests for validation
- updating documentation
@slaunay slaunay force-pushed the feature/request-latency-metric branch from 3005dec to 7b65537 Compare November 22, 2016 17:54
@eapache
Copy link
Contributor

eapache commented Nov 22, 2016

Thanks!

@eapache eapache merged commit 15bc166 into IBM:master Nov 22, 2016
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.

3 participants