x-pack/filebeat/input/{cel,httpjson,http_endpoint}: prevent complete loss of long request trace data #37836
Conversation
efd6
added
enhancement
Filebeat
botelastic
bot
added
needs_team
| // Limit request logging body size to 10kiB. | ||
| const maxBodyLen = 10 * (1 << 10) | ||
| httplog.LogRequest(h.reqLogger, r, maxBodyLen, extra...) |
There was a problem hiding this comment.
The question is whether this should be configurable or whether this is enough.
There was a problem hiding this comment.
Let's go with what you have. If it's too limiting we can add a new control on the wall later.
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
| @@ -174,7 +180,8 @@ func logRequest(log *zap.Logger, req *http.Request, extra ...zapcore.Field) (_ * | |||
| errorsMessages = append(errorsMessages, fmt.Sprintf("failed to read request body: %s", err)) | |||
| } else { | |||
| reqParts = append(reqParts, | |||
| zap.ByteString("http.request.body.content", body), | |||
| zap.ByteString("http.request.body.content", body[:max(0, min(len(body), maxBodyLen))]), | |||
There was a problem hiding this comment.
Does this need the max() call? I think the purpose is to protect against negative maxBodyLen values, but didn't trace if those are possible.
If yes, then it seems like the same protection is needed at L114.
There was a problem hiding this comment.
This is a residue. Will remove. The max calls are done at the call site now.
| @@ -71,6 +71,7 @@ func (t *valueTpl) Unpack(in string) error { | |||
| "mul": mul, | |||
| "now": now, | |||
| "parseDate": parseDate, | |||
| "parseDateInTZ": parseDateInTZ, | |||
There was a problem hiding this comment.
This looks like it should be done in a separate PR...
There was a problem hiding this comment.
😆 Yeah, noodling while making this change and accidentally committed this.
| // Limit request logging body size to 10kiB. | ||
| const maxBodyLen = 10 * (1 << 10) | ||
| httplog.LogRequest(h.reqLogger, r, maxBodyLen, extra...) |
There was a problem hiding this comment.
Let's go with what you have. If it's too limiting we can add a new control on the wall later.
…loss of long request trace data The lumberjack logger drops lines that are longer than the max size, so truncate bodies that are near the limit to ensure that at least some logging data is retained. Also truncate requests that are too long, including in http_endpoint.
💚 Build Succeeded
cc @efd6 |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
…loss of long request trace data (elastic#37836) The lumberjack logger drops lines that are longer than the max size, so truncate bodies that are near the limit to ensure that at least some logging data is retained. Also truncate requests that are too long, including in http_endpoint.
Proposed commit message
The lumberjack logger drops lines that are longer than the max size, so truncate bodies that are near the limit to ensure that at least some logging data is retained. Also truncate requests that are too long, including in http_endpoint.
Checklist
CHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs