Read whole body on each request #8894
Conversation
|
@andrewvc Not sure I fully understand why we should always do this. But as you marked it as a bug, I assume it has some side effects. |
|
@ruflin apologies, the source of this is this forum issue: https://discuss.elastic.co/t/using-heartbeat-as-a-speed-test/155033/3 The bug is that our timing, in the case where there are no body checks, is really only accurate for connection setup/teardown + whatever happens to come over the wire along the way. We should be timing the amount of time to actually receive all the data from the specified endpoint. |
|
Apologies, my previous comment linked to the wrong post. It now links to the correct one https://discuss.elastic.co/t/using-heartbeat-as-a-speed-test/155033/3 |
| for { | ||
| read, err := resp.Body.Read(buf) | ||
| if read < 0 { | ||
| panic("negative read encountered in http response!") |
There was a problem hiding this comment.
Do you really want to stop the application if this happens? I don't see a reason why it should happen but still not sure if it would be helpful to exit the application? I would rather print out an error message to be able to debug it and everything else keeps running.
There was a problem hiding this comment.
Yes, this is what the stdlib does. I shamelessly copied the behavior from: https://github.com/golang/go/blob/master/src/bytes/buffer.go#L202
There was a problem hiding this comment.
I was curious about why we check for that, I think its more of a defensive check against bad implementation of io.Reader. Looking at the method signature we do return an int and not an uint. But returning a negative number would make everything fails, I haven't personnaly added an assert to a Read return values but I do not think it hurts. :)
type Reader interface {
Read(p []byte) (n int, err error)
}
There was a problem hiding this comment.
What about using io.Copy? or io.ReadFull?
There was a problem hiding this comment.
@ph I didn't use those because for the case of large bodies I didn't want to have to read everything into memory. In the forum post that helped me find this issue https://discuss.elastic.co/t/using-heartbeat-as-a-speed-test/155033/3 the user was using heartbeat to check very large bodies. In that case we should do the equivalent of a read to dev null.
There was a problem hiding this comment.
I've now simplified this with a Discard writer.
|
@ruflin added a changelog entry and an integration test here. |
|
This will need a |
CHANGELOG.asciidoc
Outdated
| @@ -78,6 +78,7 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff] | |||
| *Heartbeat* | |||
|
|
|||
| - Fixed bug where HTTP responses with larger bodies would incorrectly report connection errors. {pull}8660[8660] | |||
| - Heartbeat now always downloads the entire body of HTTP endpoints, even if no checks against the body content are declared. This fixes an issue where timing metrics would be incorrect in scenarios where the body wasn't used since the connection would be closed soon after the headers were sent, but before the entire body was. | |||
There was a problem hiding this comment.
Can you add the PR id at the end?
|
Test failure unrelated |
We did not read the entire HTTP body unless the user had a regex match declared. We should always do this as part of our HTTP checks, otherwise timing is incorrect. (cherry picked from commit 7a02ee0)
We currently do not read the entire HTTP body unless the user has a regex match declared. We should always do this as part of our HTTP checks.