Skip to content

Handle both of stat and timer watchers are enabled and steady growing file case#3364

Merged
ashie merged 7 commits into
masterfrom
follow-up-log-throttling-per-file-feature
May 14, 2021
Merged

Handle both of stat and timer watchers are enabled and steady growing file case#3364
ashie merged 7 commits into
masterfrom
follow-up-log-throttling-per-file-feature

Conversation

@cosmo0920

@cosmo0920 cosmo0920 commented May 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Signed-off-by: Hiroshi Hatake hatake@calyptia.com

Which issue(s) this PR fixes:
Follows up #3185

What this PR does / why we need it:
In this case, timer watcher tells file contents every 1 seconds and file
contents changes is also notified from stats watcher.

In such circumstances, if some tailing files are steadily growing,
previous implementation does not prevent log ingestion for written
contents which is notified by stat watcher.

This commit also prevents log ingestion in such cases.

Docs Changes:

No needed.

Release Note:

Same as title.

Additional context

I can implement this edge case patch, but how can we write down into test cases...?

@cosmo0920 cosmo0920 requested a review from ashie May 11, 2021 06:29
@cosmo0920 cosmo0920 self-assigned this May 11, 2021
@cosmo0920
Handle both of stat and timer watchers are enabled and steady growing
27bb805 
files case

In this case, timer watcher tells file contents every 1 seconds and file
contents changes is also notified from stats watcher.

In such circumstances, if some tailing files are steadily growing,
previous implementation does not prevent log ingestion for written
contents which is notified by stat watcher.

This commit also prevents log ingestion in such cases.

Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
@cosmo0920 cosmo0920 force-pushed the follow-up-log-throttling-per-file-feature branch from 3dc9367 to 27bb805 Compare May 11, 2021 06:30
ashie
ashie requested changes May 11, 2021
View reviewed changes
Comment thread lib/fluent/plugin/in_tail.rb Outdated
Comment thread lib/fluent/plugin/in_tail.rb
cosmo0920 and others added 2 commits May 11, 2021 15:56
@cosmo0920
Handle bytes limits more elastically
34fcda1 
Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
@ashie
in_tail: Simplify checking read_bytes_limit_per_second
4f921a3 
Signed-off-by: Takuro Ashie <ashie@clear-code.com>
@ashie

ashie commented May 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

but how can we write down into test cases...?

@cosmo0920

An idea is that adding a hook to touch the test log file to limit_bytes_per_second_reached? by
rr's Proxy.
By this, it will make sure to update the file and emit inotify event on appropriate timing.

Please see the following example:
1c1ef8e#diff-b8d2266d24d2fe9459a77854ada3159ca968433b87c23b5b716ba3c132b57714R1931
Probably other test code can be almost same with existing one in #3185.

cosmo0920 added 2 commits May 13, 2021 15:43
@cosmo0920
in_tail: test: Add testcases for already reached bytes limits
bc11e6c 
Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
@cosmo0920
in_tail: test: Add writing files before checking bytes limits case
bee9125 
Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
@cosmo0920 cosmo0920 force-pushed the follow-up-log-throttling-per-file-feature branch from 1757297 to bee9125 Compare May 13, 2021 07:23
@cosmo0920

cosmo0920 commented May 13, 2021

Copy link
Copy Markdown
Contributor Author

An idea is that adding a hook to touch the test log file to limit_bytes_per_second_reached? by
rr's Proxy.
By this, it will make sure to update the file and emit inotify event on appropriate timing.

Please see the following example:
1c1ef8e#diff-b8d2266d24d2fe9459a77854ada3159ca968433b87c23b5b716ba3c132b57714R1931
Probably other test code can be almost same with existing one in #3185.

I'd considered this and I decided to add two testing scenarios.
One is already throttled scenario:
bc11e6c
The other is glitched writing timing scenario:
bee9125

Both scenarios return same number of events per emits.

@ashie

ashie commented May 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

I'm checking the tests, I feel some curious points.
In addition, I found that the test in #3185 is still inappropriate.
I'm now trying to fix them.

@ashie

ashie commented May 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

I'm now trying to fix them.

I've added a commit but probably it will fail on mac & windows.
(because it checks inotify events are surely occurred)

cosmo0920
cosmo0920 commented May 13, 2021
View reviewed changes
Comment thread test/plugin/test_in_tail.rb Outdated
ashie added 2 commits May 14, 2021 09:14
@ashie
test_in_tail: Fix some inappropriate tests for throttling feature
0d0e116 
* Check elapsed time
* Check exact number of emitted events
* Check inotify is surely emitted
* Remove a needless test.

Signed-off-by: Takuro Ashie <ashie@clear-code.com>
@ashie
test_in_tail: Fix a failed test on windows & mac
d4d0118 
Because they doesn't support inotify.

Signed-off-by: Takuro Ashie <ashie@clear-code.com>
@ashie ashie force-pushed the follow-up-log-throttling-per-file-feature branch from 81162e9 to d4d0118 Compare May 14, 2021 00:15
@ashie ashie merged commit e2e51d1 into master May 14, 2021
@ashie ashie deleted the follow-up-log-throttling-per-file-feature branch May 14, 2021 01:19
@cosmo0920

cosmo0920 commented May 14, 2021

Copy link
Copy Markdown
Contributor Author

Thanks!

This was referenced May 20, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashie ashie ashie approved these changes

Assignees

@cosmo0920 cosmo0920

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cosmo0920 @ashie