Skip to content

Add log throttling per file #1

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
Open
rewiko wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add log throttling per file #1

rewiko wants to merge 5 commits into from

Conversation

@rewiko
Copy link
Owner

@rewiko rewiko commented Oct 29, 2019 •
edited
Loading

What this PR does / why we need it:
Running in a big cluster with high volume of log, it would be nice to throttle the log shipping to avoid network saturation and make it easier to calculate the max throughput per node for example in a Kubernetes cluster.

Tail plugin is watching files and every second reading from the last pointer to the end of the file.
This change allow to stop reading the file after X number of logs lines read and update the pointer in the pos file as usual.

Docs Changes:

  • adding read_lines_limit_per_notify which by default is set to -1, so no throttling involve by default.

@rewiko rewiko force-pushed the add-log-throttling-per-file branch 2 times, most recently from bd39c23 to 8a1d6ae Compare November 1, 2019 11:36
@rewiko rewiko mentioned this pull request Nov 4, 2019
Open
@rewiko rewiko force-pushed the add-log-throttling-per-file branch from 8a1d6ae to f9eef2b Compare November 4, 2019 13:57
domleb
domleb suggested changes Nov 12, 2019
View reviewed changes
Copy link

@domleb domleb left a comment

Choose a reason for hiding this comment

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

I have a question and comment around the design:

  1. The throttling is done based on lines but would it be better to throttle based on bytes so we can handle varying line sizes more predictably / reliably?
  2. The solution in place only works if the watch_timer is enabled and the stat_watcher (inotify) is disabled, because if we enable stat_watcher it will trigger more lines to be read (if the app is still logging) and therefore defeat the throttle. However, the preferred (by fluentd) config is the opposite for both. Therefore I don't think this approach would be acceptable for fluentd and as the plan is to merge these change back in I think we should start with a design that is compatible with stat_watcher. We should also be aiming to move back to stat_watcher as we only disabled this because fluentd was getting stuck, but it still does get stuck and we have a liveliness probe to handle that.

@rewiko
Copy link
Owner Author

rewiko commented Nov 16, 2019 •
edited
Loading

@domleb I've updated the code with a new commit adding throttling per bytes and also it should work with any config now: watch_timer, stat_watcher. With inotify setup it will keep reading with the first notify until the end of the file, same behavior without the throttling. If it reaches the bytes limit it will sleep 1 second - the reading time and will continue to read until the end of the file. If other notify are getting involved for the same file they will be queued because of the mutex/synchronise.

The tail plugin is single threaded so having sleep on the notify was stopping the tail plugin and no log from other files were getting read. I've added a thread array to avoid this issue.

@rewiko rewiko force-pushed the add-log-throttling-per-file branch from f3ac9b8 to a4ec3f2 Compare November 18, 2019 21:11
@rewiko rewiko mentioned this pull request Nov 18, 2019
Closed
Add log throttling per file
9f402c1 
Remove debug log

Signed-off-by: Anthony Comtois <[email protected]>
@rewiko rewiko force-pushed the add-log-throttling-per-file branch from a4ec3f2 to 8ab733c Compare November 18, 2019 21:26
@rewiko rewiko force-pushed the add-log-throttling-per-file branch from 8ab733c to 416693c Compare November 18, 2019 21:31
@cosmo0920 cosmo0920 mentioned this pull request Nov 30, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@domleb domleb domleb requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rewiko @domleb