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

firstResponseLatency is always negative #740

Closed
nickvollmar opened this issue Mar 23, 2018 · 2 comments
Closed

firstResponseLatency is always negative #740

nickvollmar opened this issue Mar 23, 2018 · 2 comments
Labels
type: bug A general bug
Milestone

Comments

@nickvollmar
Copy link
Contributor

First: Thank you for creating/maintaining this library! It's been very useful to me and my colleagues.

Lettuce version: 4.4.2.Final (though the issue appears to exist in current master as well)

Steps to reproduce: https://gist.github.com/nickvollmar/a833681149fa27560b190799c02f6699

While playing with the metrics feature, I notice the firstResponse metrics never seem to change. The reported statistics are always the following:

firstResponse=[min=0, max=1, percentiles={50.0=1, 90.0=1, 95.0=1, 99.0=1, 99.9=1}]

This appears to be due to the following line in com.lambdaworks.redis.protocol.CommandHandler:

long firstResponseLatency = withLatency.getSent() - withLatency.getFirstResponse();

My understanding is that "sent" must be strictly before "firstResponse", because Redis cannot respond to a command that has not been sent. Therefore firstResponseLatency will always be less than zero, and DefaultCommandLatencyCollector.rangify() will always truncate the firstResponse value to MIN_LATENCY (1000 microseconds = 1 nanosecond, which is what we observe in the percentiles above).

A quick fix is to swap the order of the terms: long firstResponseLatency = withLatency.getFirstResponse() - withLatency.getSent(). I will make a PR with this change.

@nickvollmar
Copy link
Contributor Author

Created PR #741.

And thanks again for maintaining Lettuce!!

@mp911de mp911de added the type: bug A general bug label Mar 24, 2018
@mp911de mp911de added this to the Lettuce 4.4.4 milestone Mar 24, 2018
@mp911de
Copy link
Collaborator

mp911de commented Mar 24, 2018

Good catch, you're right, a command must be sent before it can receive a response. Thanks for your pull request, going to take a look.

mp911de pushed a commit that referenced this issue Mar 24, 2018
Swapped calculation to subtract sent time from first response (response is usually later than sent).

Original pull request: #741.
mp911de added a commit that referenced this issue Mar 24, 2018
Add test. Add author tag.

Original pull request: #741.
mp911de pushed a commit that referenced this issue Mar 24, 2018
Swapped calculation to subtract sent time from first response (response is usually later than sent).

Original pull request: #741.
mp911de added a commit that referenced this issue Mar 24, 2018
Add test. Add author tag.

Original pull request: #741.
mp911de pushed a commit that referenced this issue Mar 24, 2018
Swapped calculation to subtract sent time from first response (response is usually later than sent).

Original pull request: #741.
mp911de added a commit that referenced this issue Mar 24, 2018
Add test. Add author tag.

Original pull request: #741.
mp911de pushed a commit that referenced this issue Mar 24, 2018
Swapped calculation to subtract sent time from first response (response is usually later than sent).

Original pull request: #741.
mp911de added a commit that referenced this issue Mar 24, 2018
Add test. Add author tag.

Original pull request: #741.
@mp911de mp911de closed this as completed Mar 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants