[pkg/stanza/fileconsumer] Fix long line parsing #32100
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OverOrion
force-pushed
the
stanza-fix-scanner
branch
2 times, most recently
from
9683fd8 to
954249f
Compare
djaglowski
requested changes
Apr 2, 2024
There was a problem hiding this comment.
Thanks @OverOrion and @ChrsMark for finding this. Unfortunately, I do not believe this implementation works.
The underlying array may point to data that will be overwritten by a subsequent call to Scan.
This certainly seems to be a problem. To articulate the issue a bit more, we are not calling Scan again until we emit the token. However, because the token is emitted as a slice which directly references the scanner's buffer, it's contents may change later.
Possible solutions then would seem to be:
- Copy the token into a new slice to ensure the underlying contents will not change.
- Clearly advise emit funcs that they may need to do this, depending on their need for correctness vs performance. (And then handle the copy in our emit funcs)
I think it's fair of us to prioritize correctness but it will be interesting to see the performance impact associated with copying every token. To that point, we will certainly need unit tests for this as well as benchmark results to understand the tradeoff we are introducing.
Longer term, we may want to look at replacing the scanner altogether, as it's not necessarily the most performant solution. It does however provide a high degree of robustness that will be difficult to reproduce.
OverOrion
force-pushed
the
stanza-fix-scanner
branch
6 times, most recently
from
224b323 to
36c8f1d
Compare
djaglowski
reviewed
Apr 16, 2024
|
Thanks @djaglowski @ChrsMark, will address this today! |
Flush should only happen if the scanner reached EOF Signed-off-by: Szilard Parrag <szilard.parrag@axoflow.com>
OverOrion
force-pushed
the
stanza-fix-scanner
branch
from
36c8f1d to
c8e274b
Compare
djaglowski
approved these changes
Apr 22, 2024
Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
ChrsMark
approved these changes
Apr 23, 2024
djaglowski
approved these changes
Apr 23, 2024
djaglowski
added a commit
that referenced
this pull request
Feb 3, 2025
…37596) Fixes #35042 (and #32100 again) The issue affected unterminated logs of particular lengths. Specifically, longer than our internal `scanner.DefaultBufferSize` (16kB) and shorter than `max_log_size`. The failure mode was described in #32100 but was apparently only fixed in some circumstances. I believe this is a more robust fix. I'll articulate the exact failure mode again here: 1. During a poll cycle, `reader.ReadToEnd` is called. Within this, a scanner is created which starts with a default buffer size. The buffer is filled, but no terminator is found. Therefore the scanner resizes the buffer to accommodate more data, hoping to find a terminator. Eventually, the buffer is large enough to contain all content until EOF, but still no terminator was found. At this time, the flush timer has not expired, so `reader.ReadToEnd` returns without emitting anything. 2. During the _next_ poll cycle, `reader.ReadToEnd` creates a new scanner, also with default buffer size. The first time is looks for a terminator, it of course doesn't find one, but at this time the flush timer has expired. Therefore, instead of resizing the buffer and continuing to look for a terminator, it just emits what it has. What should happen instead is the scanner continues to resize the buffer to find as much of the unterminated token as possible before emitting it. Therefore, this fix introduces a simple layer into the split func stack which allows us to reason about unterminated tokens more carefully. It captures the length of unterminated tokens and ensures that when we recreate a scanner, we will start with a buffer size that is appropriate to read the same content as last time, plus one additional byte. The extra byte allows us to check if new content has been added, in which case we will resume resizing. If no new content is found, the flusher will emit the entire unterminated token as one.
chengchuanpeng
pushed a commit
to chengchuanpeng/opentelemetry-collector-contrib
that referenced
this pull request
Feb 8, 2025
…pen-telemetry#37596) Fixes open-telemetry#35042 (and open-telemetry#32100 again) The issue affected unterminated logs of particular lengths. Specifically, longer than our internal `scanner.DefaultBufferSize` (16kB) and shorter than `max_log_size`. The failure mode was described in open-telemetry#32100 but was apparently only fixed in some circumstances. I believe this is a more robust fix. I'll articulate the exact failure mode again here: 1. During a poll cycle, `reader.ReadToEnd` is called. Within this, a scanner is created which starts with a default buffer size. The buffer is filled, but no terminator is found. Therefore the scanner resizes the buffer to accommodate more data, hoping to find a terminator. Eventually, the buffer is large enough to contain all content until EOF, but still no terminator was found. At this time, the flush timer has not expired, so `reader.ReadToEnd` returns without emitting anything. 2. During the _next_ poll cycle, `reader.ReadToEnd` creates a new scanner, also with default buffer size. The first time is looks for a terminator, it of course doesn't find one, but at this time the flush timer has expired. Therefore, instead of resizing the buffer and continuing to look for a terminator, it just emits what it has. What should happen instead is the scanner continues to resize the buffer to find as much of the unterminated token as possible before emitting it. Therefore, this fix introduces a simple layer into the split func stack which allows us to reason about unterminated tokens more carefully. It captures the length of unterminated tokens and ensures that when we recreate a scanner, we will start with a buffer size that is appropriate to read the same content as last time, plus one additional byte. The extra byte allows us to check if new content has been added, in which case we will resume resizing. If no new content is found, the flusher will emit the entire unterminated token as one.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
Flush could have sent partial input before EOF was reached, this PR fixes it.
Link to tracking Issue: #31512, #32170
Testing: Added unit test
TestFlushPeriodEOFDocumentation: Added a note to
force_flush_periodoption