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.

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

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

Merged
merged 7 commits into from May 14, 2021

Conversation

cosmo0920
Copy link
Contributor

@cosmo0920 cosmo0920 commented May 11, 2021

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
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
lib/fluent/plugin/in_tail.rb Outdated Show resolved Hide resolved
lib/fluent/plugin/in_tail.rb Show resolved Hide resolved
cosmo0920 and others added 2 commits May 11, 2021 15:56
Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
Signed-off-by: Takuro Ashie <ashie@clear-code.com>
@ashie
Copy link
Member

ashie commented May 11, 2021

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.

Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
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
Copy link
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
Copy link
Member

ashie commented May 13, 2021

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
Copy link
Member

ashie commented May 13, 2021

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)

ashie added 2 commits May 14, 2021 09:14
* 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>
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
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants