in_tail: Always read from head for new/rotated files after startup

[majimenez-stratio]

Signed-off-by: majimenez-stratio [email protected]

Which issue(s) this PR fixes:
None

What this PR does / why we need it:
I noticed that read_from_head affect files read during startup and new files equally. I see a few drawbacks with the current situation if read_from_head = false and path includes '*', namely:

• New files detected on refresh are not read from the beggining and initial lines are lost.
• Same as above for rotated files

As a result, I think we can improve the way read_from_head is applied, so that it only affects new files detected during startup. This PR changes the behavior so that:

• During startup, files are read from the position stored in the position file, if available. If not, they are read as specified in read_from_head.
• After startup, if a new file is detected (or rotated), it is always read from the beginning.
• If skip_refresh_on_startup = true, new files are always read from the beggining. I'm not sure about this side effect, but I think skip_refresh_on_startup only makes sense in combination with read_from_head = true, so I think we're fine here.

I tried to mimic the way fluent-bit tail input works as it makes more sense to me. However, I'm quite new to Ruby, fluentd and logging in general, so I'm open to discussion if you feel this change is not appropiate or should be approached from another angle.

Docs Changes:

• We need to update the document about read_from_head
  • https://docs.fluentd.org/input/tail#read_from_head
  • https://github.com/fluent/fluentd-docs-gitbook/blob/1.0/input/tail.md#read_from_head

Release Note:
Always read from head for new/rotated files after startup

[ashie]

Good catch!
Your patch seems almost reasonable, thanks!

• If skip_refresh_on_startup = true, new files are always read from the beggining. I'm not sure about this side effect, but I think skip_refresh_on_startup only makes sense in combination with read_from_head = true, so I think we're fine here.

We should care this case too to keep consistency.
Otherwise people who reading the code later will be confused.
How about using an instance variable to check the first setup_watchers is already called or not.
By this, additional argument aren't needed anymore.

[majimenez-stratio]

Sounds good, I’ll change the code to address this case

[majimenez-stratio]

I followed your suggestions and refactored the code so that startup is tracked with an instance variable instead of parameters. It looks much nicer now, thanks for the tip.

Also, with the latest changes, the behavior when skip_refresh_on_startup = true seems more consistent, as it is basically the same as when it's false: first refresh works as specified in read_from_head, but will always read from the beginning for new/rotated files.

Additionally, this is the code that resets @startup instance var:

fluentd/lib/fluent/plugin/in_tail.rb

Line 381 in 98feddc

@startup = false if @startup

I'm not sure it is the best/safer way to do it or the most idiomatic, I'll take your guidance here in case there's a better way.

[ashie]

Could you resolve conflict?

[ashie]

Additionally, this is the code that resets @startup instance var:

fluentd/lib/fluent/plugin/in_tail.rb

Line 381 in 98feddc

@startup = false if @startup

I'm not sure it is the best/safer way to do it or the most idiomatic, I'll take your guidance here in case there's a better way.

LGTM

[kenhys]

@majimenez-stratio could you resolve the conflict again, please?

[majimenez-stratio]

@ashie Is there anything else I can do here so this can be merged? There's no rush, just to know :)

[ashie]

Various memo:

• We need to update the document about read_from_head
  • https://docs.fluentd.org/input/tail#read_from_head
  • https://github.com/fluent/fluentd-docs-gitbook/blob/1.0/input/tail.md#read_from_head
• I had a concern about incompatibility of the behaviour. But now I think that adding a notice to above document is enough, the revised behaviour seems reasonable for most users.
• I considered to add a test for rotation case but probably it's not needed
  • Rotation is usually processed by TailWatcher#on_rorate, not by refresh_watcher. So that read position is correctly determined by it.
  • In actual, I tried to add a test for it but both previous code & new code are passed the test.

[ashie]

Thanks for your contribution!

[majimenez-stratio]

My pleasure!