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

Add log throttling per file #1

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

rewiko
Copy link
Owner

@rewiko rewiko commented Oct 29, 2019

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 force-pushed the add-log-throttling-per-file branch from 8a1d6ae to f9eef2b Compare November 4, 2019 13:57
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

@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.

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
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.

2 participants