-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Heartbeat] Capture HTTP Response Bodies #13022
Conversation
Pinging @elastic/uptime |
We should store hashes of the bodies with it |
@@ -49,6 +51,11 @@ type Config struct { | |||
Check checkConfig `config:"check"` | |||
} | |||
|
|||
type responseConfig struct { | |||
IncludeBody string `config:"include_body"` | |||
IncludeBodyMaxBytes int `config:"include_body_max_bytes"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does also not seem to be documented. How does it overlap with the truncate? Probably see it in the code below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in the documentation! I documented the whole response
block as one thing here. Glad to change it, but it's in keeping with the style of some other feature's documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, missed that. Seems my command - F
foo failed me.
@@ -103,6 +114,20 @@ var defaultConfig = Config{ | |||
}, | |||
} | |||
|
|||
func (r *responseConfig) Validate() error { | |||
switch strings.ToLower(r.IncludeBody) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really want to do the strings to lower? I would enforce users to write it correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, I don't see one way as being correct or another. Some people like typing in upper case. It's not a big deal to me however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, just personal preference. Both options ok for me.
@@ -144,7 +144,7 @@ func (t *SimpleTransport) RoundTrip(req *http.Request) (*http.Response, error) { | |||
// finally, return the real error. No need to return a response here | |||
return nil, errors.New("http: request timed out while waiting for response") | |||
} | |||
close(readerDone) | |||
//close(readerDone) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That actually should still be in place AFAICT, undid this change.
} | ||
return start, end, errReason | ||
// Add response.status_code | ||
eventext.MergeEventFields(event, common.MapStr{"http": common.MapStr{"response": common.MapStr{"status_code": resp.StatusCode}}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if you want to do that only after the handleRespBody to not have that also in the timing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason this is first is because handleRespBody
can return an error causing an early exit. It would be a little awkward to inject this before the error check/return.
This is pretty fast code to execute, and given that we aren't writing real time code, the effect should be negligible.
- name: hash | ||
type: keyword | ||
description: > | ||
Hash of the response body. Can be used to group responses with identical hashes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the hash of the full response body of the partial response body that is returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full body, clarified it in the latest push.
Test failures in filebeat and metricbeat seem unrelated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can be merged as is from my perspective but would be great to still get a response to the questions for a potential follow up.
@@ -103,6 +114,20 @@ var defaultConfig = Config{ | |||
}, | |||
} | |||
|
|||
func (r *responseConfig) Validate() error { | |||
switch strings.ToLower(r.IncludeBody) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, just personal preference. Both options ok for me.
break | ||
} | ||
|
||
if readErr != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, just seeing this now. Shouldn't this line directly be after line 95? I get that if you have an EOF that you also want to get the content first, but if you have an actual error, I assume also the readSize is probably 0 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope! See https://golang.org/pkg/io/#Reader
Callers should always process the n > 0 bytes returned before considering the error err. Doing so correctly handles I/O errors that happen after reading some bytes and also both of the allowed EOF behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reminder 👍
Co-Authored-By: Nicolas Ruflin <spam@ruflin.com>
This PR adds optional support for capturing HTTP response bodies in heartbeat events. By default it sets the body only on responses that return errors, but can optionally be set to never report the body or report it on all checks. (cherry picked from commit 1e3e65c)
Updates the go-lookslike library we maintain to 0.3.0 which uses a much improved reflection strategy. This fixed an issue in the last release where Strict() which checked for extra fields wasn't working. As a result some of our tests developed since then didn't assert that the new fields added in #13022 were actually valid. Those tests are fixed in this PR.
Currently, when using an HTTP body validator (either regexp or JSON) will break the storage of HTTP bodies with response.include_body (introduced in #13022). The root cause is that both validation and reading the body for inclusion in the event share the same ReadCloser provided by *http.Response. This patch looks at both validation and body settings to determine how much of the body to read, reads that much, then passes that to the validation and body inclusion code as a []byte. Resolves #13751
Currently, when using an HTTP body validator (either regexp or JSON) will break the storage of HTTP bodies with response.include_body (introduced in elastic#13022). The root cause is that both validation and reading the body for inclusion in the event share the same ReadCloser provided by *http.Response. This patch looks at both validation and body settings to determine how much of the body to read, reads that much, then passes that to the validation and body inclusion code as a []byte. Resolves elastic#13751 (cherry picked from commit 6c6b396)
) Currently, when using an HTTP body validator (either regexp or JSON) will break the storage of HTTP bodies with response.include_body (introduced in #13022). The root cause is that both validation and reading the body for inclusion in the event share the same ReadCloser provided by *http.Response. This patch looks at both validation and body settings to determine how much of the body to read, reads that much, then passes that to the validation and body inclusion code as a []byte. Resolves #13751 (cherry picked from commit 6c6b396)
Currently, when using an HTTP body validator (either regexp or JSON) will break the storage of HTTP bodies with response.include_body (introduced in elastic#13022). The root cause is that both validation and reading the body for inclusion in the event share the same ReadCloser provided by *http.Response. This patch looks at both validation and body settings to determine how much of the body to read, reads that much, then passes that to the validation and body inclusion code as a []byte. Resolves elastic#13751
… (elastic#14310) Currently, when using an HTTP body validator (either regexp or JSON) will break the storage of HTTP bodies with response.include_body (introduced in elastic#13022). The root cause is that both validation and reading the body for inclusion in the event share the same ReadCloser provided by *http.Response. This patch looks at both validation and body settings to determine how much of the body to read, reads that much, then passes that to the validation and body inclusion code as a []byte. Resolves elastic#13751 (cherry picked from commit 46643b0)
This PR adds optional support for capturing HTTP response bodies in heartbeat events.
By default it sets the body only on responses that return errors, but can optionally be set to never report the body or report it on all checks.
Fixes elastic/uptime#37