Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 in files based on group rules #3535
Add log throttling in files based on group rules #3535
Changes from 14 commits
e3a4ff6
0fed6bd
10603da
87e7426
c76ffbd
41d348f
8c466a5
fe4f915
aefde97
7b1bbac
a2d9526
433883b
46e898f
e8d893a
d311670
e291dae
0de4e66
dc680be
07ef562
efa7d2d
bd5a704
c6c5989
5f54dd0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return
nil
onremove_path_from_group_watcher
as well?This is just nitpick and not important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this great feature!
I have a question about this code line.
The
@limit
controls the total number of lines collected for a group, right? (https://docs.fluentd.org/input/tail#less-than-group-greater-than-section)That judgment seems to be done by this code line, but this line judges it by the average number of lines collected for a group, not the total number.
Either the code or the documentation is wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's right. It should be total.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! For now, I made an issue for this:
in_tail
:group
setting judges the limit by the average number, not the total number #4184There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daipom @ashie
The PR description states that the limit set in the configuration distributes the rate uniformly to all the members in a group. That's why the condition checks for average rate limit for a particular group member. I get why this is confusing and we may either change the documentation with an example or change the code itself.
The reason we are using average limit (for a group member) is because it puts a hard upper limit on the rate of logs emitted by a group. For example, a two groups with same limit but one having 1000 containers and the other having 10 containers, both will emit the same number of log lines.
However, if you apply the same limit to all the group members (without having a hard upper limit control) then there's no limit on the amount of data that is sent via fluentd. Considering the previous example, the rate of data sent from one group will be 100x larger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pranjal-Gupta2 Thanks!
If we apply the limit to each file independently, it will be 100 times larger, right?
I think the preferred specification is to calculate the total number in the group in #4184.
In this way, it will be the same upper amount independent of the number of files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! We can write a better articulation of this point in the documentation.
@ashie Can I take this up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will think some more about what to do with #4184.
If it looks good to fix the document, I will fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry. I missed this comment.
At this point, fixing the document would be a good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We welcome the document modifications of course!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @daipom @ashie!
Will start working on this.