-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[Exporter/Elasticsearch] Fix HTTP 413 (Payload Too Large) in Elasticsearch exporter #46022
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
base: main
Are you sure you want to change the base?
[Exporter/Elasticsearch] Fix HTTP 413 (Payload Too Large) in Elasticsearch exporter #46022
Conversation
Signed-off-by: SoumyaRaikwar <[email protected]>
|
Thank you for working on this! I have a couple questions:
|
|
mauri870
left a comment
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.
Code-wise, LGTM. I also tested it locally with some custom test cases I had for this, and it seems to work as intended. Unfortunately, I don’t have a deep understanding of the internals, so I'll wait for feedback from the code owners.
carsonip
left a comment
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, a few questions
|
|
||
| import ( | ||
| "bytes" | ||
| "compress/gzip" |
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.
q: is the dep swap intentional?
| if gzipErr != nil { | ||
| return nil, gzipErr | ||
| } | ||
| defer gr.Close() |
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.
q: can we close gr sooner rather than leaving it open while recursive calls are made?
| content = bodyBytes | ||
| } | ||
|
|
||
| lines := bytes.Split(content, []byte("\n")) |
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.
bytes.Split and bytes.Join memory usage footprint can be high. can we walk the bytes and get index of the middle \n, maybe using a slow and fast pointer algorithm, then split the payload by slicing the byte slice?
|
Just gave a brief look at the PR, and I don't think this is the way we should fix it. There are already conflicts between how ES exporter does things and how exporterhelper does things (take retries for example). A better way to fix this is to emplement |
carsonip
left a comment
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 was talking to @lahsivjar about the HTTP 413 issue and it looks more like a misconfiguration (user issue) on batching config. Let's discuss the problem further in #45834 before jumping to a fix like this which may be a footgun, if we're splitting requests after merging them in a batcher.
@lahsivjar @carsonip |
Description
This PR implements automatic request splitting and retry logic for the
elasticsearchexporter. When the Elasticsearch server responds with an HTTP 413 (Payload Too Large) error, the exporter now intercepts the response, splits the large bulk request body (NDJSON) into two smaller chunks, and retries them sequentially. This prevents data loss for batches that exceedhttp.max_content_length.Link to tracking issue
Fixes #45834
Testing
413 Request Entity Too Largefor the initial request.200 OK.make lintandmake testlocally to ensure no regressions.Documentation
.chloggen/fix-elasticsearch-413.yaml.