Add possibility to force flush logs after certain period of time

[sumo-drosiek]

Fixes #215

This is work in progress PR. General idea is to keep information about when last log has been properly flushed and if it was long ago, buffered line should be flushed as we do not except more lines

I'm going to make minor refactor, add unit tests and more comments to code

[codecov]

Codecov Report

Merging #216 (b2faa2c) into main (2ffe032) will increase coverage by 0.1%.
The diff coverage is 94.3%.

@@           Coverage Diff           @@
##            main    #216     +/-   ##
=======================================
+ Coverage   75.6%   75.8%   +0.1%     
=======================================
  Files         95      95             
  Lines       4408    4464     +56     
=======================================
+ Hits        3334    3384     +50     
- Misses       747     751      +4     
- Partials     327     329      +2     

Impacted Files	Coverage Δ
operator/builtin/input/file/file.go	72.2% <55.5%> (-1.3%)	⬇️
operator/builtin/input/file/config.go	77.3% <100.0%> (+0.3%)	⬆️
operator/builtin/input/file/reader.go	63.5% <100.0%> (-1.0%)	⬇️
operator/builtin/input/tcp/tcp.go	74.5% <100.0%> (ø)
operator/builtin/input/udp/udp.go	71.1% <100.0%> (ø)
operator/helper/multiline.go	89.5% <100.0%> (+5.2%)	⬆️

[djaglowski]

Perhaps this is just a naming issue, but I'll have a difficult time accepting that the splitfunc is being delegated to a Multiline struct. Put another way, we need a split func regardless of whether or not we are using multiline splitting.

I suspect that we can accomplish the same functionality as you are implementing, but by composing things in a better way.

Would it make sense to create a new struct which would pair the concept of a split func with the concept of a flush? Maybe something like:

type Splitter struct {
    splitFunc // can be simple like '\n' or multiline config
    flusher     // forces a flush based on timing
}

[djaglowski]

After looking at this further, I'm not convinced we even need to do what I've suggested here. Please see my related comment on reader.go. I'll leave this comment in place as a possible alternative, but I think we need to start by justifying the need to alter splitFunc behavior in any way.

[djaglowski]

For consistency, I think we should use helper.Duration instead of time.Duration. We can then link to https://github.com/open-telemetry/opentelemetry-log-collection/blob/main/docs/types/duration.md, which explains formatting sufficiently.

This is basically the same thing, but with a default unit of seconds, since nanoseconds is basically never reasonable in this context.

[sumo-drosiek]

I updated documentation

[djaglowski]

I believe this is the only place we are using the split func, so I'm not sure why it needs to be part of the multiline config.

We have two concepts here:

1. How to split lines
2. When to force a flush

I'm not convinced these are dependent on each other, or that either is necessarily a concern of multiline behavior. (Of course a split func may implement multiline splitting, but it does not always.)

Can we leave splitFunc as it was and just add a new config that controls flushing?

[sumo-drosiek]

There is some relations I was unsure how to resolve, so I put both functionality into one struct:

• splitFunc needs to be aware when to force a flush (so new structure is created and pass to the splitFunc)
• every file should be flushed separately (so splitFunc is created for every unique reader)

Regarding line splitting. Of course it is not always a multiline behavior, but looking at it from user perspective it is intuitive to use multline config for it, so Multiline struct underneath, than mixing Splitter struct with multiline config, or using splitter config.

I will exclude force a flush to separate config, as logic of it is mostly out of the splitFunc, but not sure how to resolve other dependencies in nice way

[sumo-drosiek]

I made some changes in requested direction, but after all I think Splitter struct seems to be reasonable consensus.

You can see current implementation without Splitter. There are two dependent entities: splitFunc and ForceFlush and I feel that they should be connected together via additional struct. WDYT?

[djaglowski]

I think consolidating into one struct makes sense

[sumo-drosiek]

See the latest code, please

[djaglowski]

Thanks @sumo-drosiek, nice improvement!