Skip to content
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.

Sign up for GitHub

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

Jump to bottom

enhancement(file source): Better multi-line support #1852

Merged
merged 18 commits into from
Feb 22, 2020
Merged

enhancement(file source): Better multi-line support #1852

merged 18 commits into from
Feb 22, 2020

Conversation

MOZGIII
Copy link
Contributor

@MOZGIII MOZGIII commented Feb 19, 2020 •
edited by binarylogic
Loading

Closes #1431.
Ref fluent/fluent-bit#337.

TODO:

  • docs
  • behavior tests? no behavior tests for sources
  • test harness tests? probably would track this one separately

@MOZGIII
Extracted LineAgg to a separate mod, mostly as is (except for visibil…
deb5b4c 
…ity)

Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII MOZGIII force-pushed the better-line-agg branch 3 times, most recently from b179c3d to 7a685d0 Compare February 19, 2020 12:00
@MOZGIII
Prepare the structure to implement new multiline logic
a4eaec4 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Implement handling lines with conditions
a3c510e 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Change tests for legacy mode to go through a different constructor
c55644e 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Remove Mode::Legacy and reimplement it via new config
689a6c2 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Represent timeout as Duration
bb76980 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Switch to implementing legacy support at the Config level
b574013 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Tailor public interface visibility and derivations
2b2c5d5 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Add support for using new LineAgg interface at file source
954c400 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Update CODEOWNERS
754ec83 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII MOZGIII marked this pull request as ready for review February 19, 2020 14:51
@MOZGIII MOZGIII requested a review from LucioFranco as a code owner February 19, 2020 14:51
@binarylogic binarylogic requested review from bruceg and removed request for LucioFranco February 20, 2020 00:22
@binarylogic
Copy link
Contributor

@bruceg mind reviewing this?

bruceg
bruceg requested changes Feb 20, 2020
View reviewed changes
Copy link
Member

@bruceg bruceg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly pretty good, particularly the internal documentation. I just have a few questions I'd like to see answered before approving this. The tests look like they cover a good number of cases, which is reassuring.

src/sources/file.rs Outdated Show resolved Hide resolved
src/sources/file.rs Show resolved Hide resolved
src/sources/file.rs Outdated Show resolved Hide resolved
src/sources/file.rs Show resolved Hide resolved
src/sources/file.rs Outdated Show resolved Hide resolved
src/sources/file/line_agg.rs Show resolved Hide resolved
src/sources/file/line_agg.rs Outdated Show resolved Hide resolved
src/sources/file/line_agg.rs Outdated Show resolved Hide resolved
src/sources/file/line_agg.rs Outdated Show resolved Hide resolved
src/sources/file/line_agg.rs Show resolved Hide resolved
@MOZGIII MOZGIII requested a review from binarylogic as a code owner February 20, 2020 13:24
@MOZGIII
Update documentation
e716ecf 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Add snafu display to regex parsing errors
d3876f9 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Use simpler try_into invocations
13cfa64 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Make LineAgg generic over line source identifiers (buffer keys)
1062684 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Use hash_map::Entry
93a5a26 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Add utility function to add next line to existing buffer
274eb9d 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Move src/sources/file.rs to src/sources/file/mod.rs
4a01b5b 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII MOZGIII mentioned this pull request Feb 20, 2020
Open
@MOZGIII
Update CODEOWNERS
67a2c7f 
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII MOZGIII requested a review from bruceg February 20, 2020 15:47
bruceg
bruceg approved these changes Feb 20, 2020
View reviewed changes
Copy link
Member

@bruceg bruceg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one question, but that shouldn't hold up merging.

src/sources/file/mod.rs
if let Some(ref multiline_config) = multiline_config {
Box::new(LineAgg::new(
rx,
multiline_config.try_into().unwrap(), // validated in build
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thought. Is there any way for this created line_agg::Config to be stored where it is parsed in build and used here instead of re-parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried going that route - but ended up wanting to redo the whole file source configuration approach. I think we need to address this systematically - validate and update all the units. In short, the idea I had is to have two layers - a serializable/deserializeable config and a "processed" config, that's been validated and has values converted into the proper types.
So, in short - I see how ugly the current approach is - and we definitely should fix this, but even if we'll do it just with the file source - this is one of the part where I deliberately decided to leave the current approach unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #1895

@MOZGIII MOZGIII merged commit 89026d0 into master Feb 22, 2020
This was referenced Feb 22, 2020
Closed
Closed
@binarylogic binarylogic deleted the better-line-agg branch April 24, 2020 20:39
rcollette
rcollette reviewed Oct 12, 2020
View reviewed changes
.meta/sources/file.toml
required = false
description = """\
Multiline parsing configuration. \
If not speicified, multiline parsing is disabled.\
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling issue. Should be "specified."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rcollette That spelling error has been addressed in later code. Thanks for letting us know though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcollette rcollette rcollette left review comments

@jamtur01 jamtur01 jamtur01 left review comments

@bruceg bruceg bruceg approved these changes

@binarylogic binarylogic Awaiting requested review from binarylogic

Assignees

@bruceg bruceg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

message_stop_indicator for file source
5 participants
@MOZGIII @binarylogic @jamtur01 @rcollette @bruceg