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

Fix negative response times for MongoDB. #231

Merged
merged 2 commits into from
Sep 8, 2015
Merged

Fix negative response times for MongoDB. #231

merged 2 commits into from
Sep 8, 2015

Conversation

tsg
Copy link
Contributor

@tsg tsg commented Sep 8, 2015

The message timestamp could have been accidentally reused from an
old message in the same stream. For example, in this sequence of
messages in the same stream:

  • REPLY1 (with no matching request)
  • REQUEST2
  • REPLY2

packetbeat was correctly matching the REQUEST2 with REPLY2 but was using
the timestamp of REPLY1 for computing the response time. The fix is to
simply through away the message and wait for it to be created with the
new message received.

Should fix most of the issues from #216.

This also removes parseOffset and bytesReceived from the stream struct as
they were not used.

The message timestamp could have was accidentally reused from an
old message in the same stream. For example, in this sequence of
messages in the same stream:

* `REPLY1` (with no matching request)
* `REQUEST2`
* `REPLY2`

packetbeat was correctly matching the REQUEST2 with REPLY2 but was using
the timestamp of REPLY1 for computing the response time. The fix is to
simply through away the message and wait for it to be created with the
new message received.

Should fix most of the issues from #216.

This also removes parseOffset and bytesReceived from the stream struct as
they were not used.
@tsg tsg added the review label Sep 8, 2015
@ruflin
Copy link
Member

ruflin commented Sep 8, 2015

@tsg Looks good. Should we do the same for Redis? https://github.com/elastic/packetbeat/blob/master/protos/redis/redis.go#L296

@urso
Copy link

urso commented Sep 8, 2015

+1

Checked all protocols. Only redis and mongodb use this logic when resetting the Stream for new messages. All other protocols should be fine.

Same as for MongoDB, this can be a cause of negative response
times.
@tsg
Copy link
Contributor Author

tsg commented Sep 8, 2015

Right, I guess the Redis code is where this came from originally. I changed it there as well, tests still pass.

ruflin added a commit that referenced this pull request Sep 8, 2015
Fix negative response times for MongoDB.
@ruflin ruflin merged commit 93afb9b into elastic:master Sep 8, 2015
ruflin added a commit that referenced this pull request Dec 2, 2015
urso pushed a commit that referenced this pull request Dec 2, 2015
Update to most recent libbeat version and shared files
tsg pushed a commit to tsg/beats that referenced this pull request Jan 20, 2016
Fix negative response times for MongoDB.
tsg pushed a commit to tsg/beats that referenced this pull request Jan 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants