Add log throttling in files based on group rules

[Pranjal-Gupta2]

Which issue(s) this PR fixes:
None

What this PR does / why we need it:
Fluentd running in a big cluster with a high volume of logs has to deal with a lot of applications logging at different rates. High log generation rates for low priority applications can lead to log loss for high priority applications. We need to have a data flow control in the log collector so that we can limit the log collection rate of applications based on the priority of application workloads in the cluster.

Tail Plugin is watching a file and reads logs till it reaches the end of the file. Earlier accepted PR #3185 introduced throttling at the source level which applies a limit to the number of bytes collected by the in_tail plugin every second. However, it uniformly applies the same limit to every log file.

This PR enables us to form different groups based on configuration rules provided by the user. Within each group, one can set the rate limits for log collection. The set rate limit gets uniformly applied to each member (log file) of the group. If the group line limit of the log is reached within the rate_period time interval, Fluentd stops reading logs from that group. Once, rate_period (time) is over, Fluentd resets its group counters and restarts the reading process.

I strongly believe group level rate limiting can really help one to control low priority applications vs high priority applications log flow. Currently based on the namespace and app name (application name) extracted from file names, one can form groups for rate limiting.

Docs Changes:
fluent/fluentd-docs-gitbook#376

Release Note:
Same as Title

[ashie]

The concept would be nice, but implementing it as another plugin isn't good idea.
Please imagine several tail plugins that are focused on each features.

• in_tail_feature1
• in_tail_feature2
• in_tail_feature3
• ...

If a user wants feature1 and feature3, which plugin the user should select?
We should integrate it into existing tail plugin.

On the other hand, current in_tail’s code is complicated, I think refactoring might be needed before adding new major features.
Until doing it, providing it as an external plugin would be nice.

[Pranjal-Gupta2]

The concept would be nice, but implementing it as another plugin isn't good idea. Please imagine several tail plugins that are focused on each features.

• in_tail_feature1
• in_tail_feature2
• in_tail_feature3
• ...

If a user wants feature1 and feature3, which plugin the user should select? We should integrate it into existing tail plugin.

Thank you for all your valuable feedback. In the in_tail plugin, one can rate limit using read_bytes_limit_per_second, but in this PR, in_tail_with_throttle provides a grouping mechanism for rate limiting files. These two features might seem mutually exclusive from an end-user point of view. That’s why we prepared a separate plugin that extends the functionality of the in_tail plugin. So, if one is interested in limiting each file by bytes per second, then one can use the in_tail plugin. Otherwise, you can add group rules in in_tail_with_throttle.

On the other hand, current in_tail’s code is complicated, I think refactoring might be needed before adding new major features. Until doing it, providing it as an external plugin would be nice.

Do you want to keep a separate plugin in this repository until in_tail is refactored and then merge it later?

[pmoogi-redhat]

@ashie can you further guide on this PR merging ? please note both in_tail and in_tail_with_throttle can co-exists and one can use both of them as set of . we didn't want to disturb the base line of in_tail hence we created a new plugin with this group based rate limiting functionality. let us know what step we need to take next.

[ashie]

My additional impressions:

• Equivalent feature might be able to realized by multiple <source> and adding regexp based path parameter to in_tail. Could you describe the advantage of your patch compared with other ideas?
  • I think one of the strength of your idea is fallback mechanism, it makes easy to configure unmatched paths.
• Again, implementing as an additional plugin isn't good idea, please consider integrating it into in_tail if you want to merge it into fluentd.
  • If you can integrate it cleanly, we might not need waiting refactoring.
• The config format seems too rely on k8s's concept. It would be nice if you can generalize it.
• We don't want maintaining Copy & Pasted codes. For example detach_watcher_after_rotate_wait and TailWatcher::IOHandler#rate_limit_handle_notify are almost same with parent's one. When parent one is modified, we have to modify both codes. Please consider integrating them.
• Although in_tail_with_throttle might change the behaviour of in_tail, in_tail's tests aren't executed against in_tail_with_throttle.

[Pranjal-Gupta2]

My additional impressions:

• Equivalent feature might be able to realized by multiple <source> and adding regexp based path parameter to in_tail. Could you describe the advantage of your patch compared with other ideas?

  • I think one of the strength of your idea is fallback mechanism, it makes easy to configure unmatched paths.

@ashie Our plugin has the following advantages :

1. Using multiple threads -> context switching and cpu usage

Adding regexp based path parameter to in_tail will add multiple source plugins which will create additional threads for reading from files. Hence cpu cycles will be wasted in context switching.

2. Paths can be overlapped when using multiple source plugins

in_tail_with_throttle allows you to have generic and context-specifc rules for log rate limit. It will be difficult to manage with in_tail's path parameter based grouping and can lead to files falling In more than one group.

For example, consider the following requirement:
1. Rate limit all files with a particular namespace (Generic rule).
2. Give priority to specific app name in that namespace with a different limit (Context Specific Rule)

Configuration for such requirement will be like: 
<group_rule>
	...
	<rule>
		## rule1
		namespace ABC
		limit 1000 ## Generic rules for all files in a namespace
	</rule>	
	<rule>
		## rule2
		namespace ABC
		appname DEF  ## Specific rules with adjustable limit
		limit 1000
	</rule>	
	...
</group_rule>

in_tail_with_throttle will add a file with DEF app name and ABC namespace to rule 2 (Higher precedence over rule1). Using in_tail's path parameter will add the file in two groups and hence repeated log lines will be collected.

3. Our plugin will take care of unexpected files.

In cases where we forget to add rules for some files, in_tail_with_throttle will add them to the default group and still collect logs. in_tail plugin with regex based path parameter grouping will fail in these situations.

4. Config will get pretty cluttered.

Adding multiple source configs for in_tail's path based grouping will make your configuration file big in size. It will be difficult to manage and update configurations in case of very large number of groups.

[Pranjal-Gupta2]

My additional impressions:

• The config format seems too rely on k8s's concept. It would be nice if you can generalize it.

@ashie can you please elaborate on what this generalisation means? Should we replace our namespace and app-name fields with something else?

For example,

<rule> 
    ## level1 instead of namespace
    level1 ABC

    ## level2 instead of appname
    level2 DEF

    limit 1000
</rule>

[Pranjal-Gupta2]

@ashie Could you please comment?

[agup006]

@cosmo0920 would you also be able to take a look?

[cosmo0920]

I reviewed and added a few concerns. Could you take a look about them?

[ashie]

Thanks for revising the patch, it's much better than before!
I'm sorry for my late response, I was thinking how to resolve remaining issues.
I commented about it above, please check it.

[cosmo0920]

Implementation idea is good. But, I found a nitpick issue for a constant name.

[ashie]

The big picture seems good, I'll check further more details, please wait for a few days...

[ashie]

A test always fails:

2022-02-07T07:27:29.7369178Z Error: test_shorter_than_rotate_wait(TailInputTest::singleline::log throttling per file::EOF with reads_bytes_per_second): Fluent::Test::Driver::TestTimedOut: Test case timed out with hard limit.
2022-02-07T07:27:29.7398448Z /home/runner/work/fluentd/fluentd/lib/fluent/test/driver/base.rb:201:in `rescue in run_actual'
2022-02-07T07:27:29.7398936Z /home/runner/work/fluentd/fluentd/lib/fluent/test/driver/base.rb:196:in `run_actual'
2022-02-07T07:27:29.7399358Z /home/runner/work/fluentd/fluentd/lib/fluent/test/driver/base.rb:95:in `run'
2022-02-07T07:27:29.7399753Z /home/runner/work/fluentd/fluentd/lib/fluent/test/driver/base_owner.rb:130:in `run'
2022-02-07T07:27:29.7400214Z /home/runner/work/fluentd/fluentd/test/plugin/test_in_tail.rb:714:in `test_shorter_than_rotate_wait'
2022-02-07T07:27:29.7400553Z      711:             tw
2022-02-07T07:27:29.7400872Z      712:           end.twice
2022-02-07T07:27:29.7401240Z      713: 
2022-02-07T07:27:29.7401498Z   => 714:           d.run(timeout: 10) do
2022-02-07T07:27:29.7403168Z      715:             until detached do
2022-02-07T07:27:29.7403680Z      716:               if d.events.size > 0 && !rotated
2022-02-07T07:27:29.7405460Z      717:                 cleanup_file("#{TMP_DIR}/tail.txt")
2022-02-07T07:27:29.7406123Z ===============================================================================
2022-02-07T07:29:08.6385152Z ...............................................................................
2022-02-07T07:29:08.6500370Z ...............................................................................
2022-02-07T07:29:08.6725292Z ...............................................................................

[cosmo0920]

Looks good with a nitpick comment.

[cosmo0920]

Should we return nil on remove_path_from_group_watcher as well?
This is just nitpick and not important.

[ashie]

Hmm, it takes too long time to run tests, more than 1 minutes longer than before:

master branch:

$ time bundle exec rake test TEST=test/plugin/test_in_tail.rb
...
real	2m33.910s
user	0m3.410s
sys	0m0.322s

this branch:

$ time bundle exec rake test TEST=test/plugin/test_in_tail.rb
...
real	3m36.044s
user	0m3.807s
sys	0m0.335s

We should shorten it if it's possible since it's affect to whole debugging efficiency.

[ashie]

Hmm, it takes too long time to run tests, more than 1 minutes longer than before:

Trying to improve it by efa7d2d
It seems fine on my local environment but not sure on CI yet.

[ashie]

Hmm, it takes too long time to run tests, more than 1 minutes longer than before:

Trying to improve it by efa7d2d It seems fine on my local environment but not sure on CI yet.

It's also fine on CI.

[ashie]

I'll merge this after checking group_watch.rb once more.

[ashie]

Hmm, it takes too long time to run tests, more than 1 minutes longer than before:

Trying to improve it by efa7d2d It seems fine on my local environment but not sure on CI yet.

$ time bundle exec rake test TEST=test/plugin/test_in_tail.rb
...
real	2m45.993s
user	0m3.642s
sys	0m0.266s

[ashie]

Merged.
I'm sorry for taking long time to merge this.
And thank you for contributing a nice feature 😃

[PranjalGupta2199]

Thanks @ashie @cosmo0920 @agup006 for your help and support! I am glad that it got merged! 🎉

[yuzs2]

looks this is a very useful feature, it would be even greater if you could provide more detailed user-guide for this. I'm new to fluentd, and the current example looks too simple for me: https://docs.fluentd.org/input/tail#less-than-group-greater-than-section, thank you

[yuzs2]

Hi folk, I tried to add the group section in my fluent.conf, but it was giving parse error:

/usr/local/rvm/rubies/ruby-2.7.6/lib/ruby/gems/2.7.0/gems/fluentd-1.15.0/lib/fluent/config/basic_parser.rb:92:in `parse_error!': got incomplete JSON hash configuration at kubernetes.conf line 230,10 (Fluent::ConfigParseError)

below is what my fluent.conf be like, removing group section it will be working fine:

<source>
  @type tail
  @id in_tail_container_logs
  path /var/log/containers/*.log
  pos_file /var/log/{{.ID}}-fluentd-containers.log.pos
  pos_file_compaction_interval 1h
  tag kubernetes.*
  read_from_head true
  read_bytes_limit_per_second 8192
  <parse>
    @type multiline
    # cri-o
    format1 /^(?<partials>([^\n]+ (stdout|stderr) P [^\n]+\n)*)/
    format2 /(?<time>[^\n]+) (?<stream>stdout|stderr) F (?<log>[^\n]*)/
    # docker
    format3 /|^(?<json>{.*})/
    time_format %Y-%m-%dT%H:%M:%S.%N%:z
  </parse>
  <group>
    rate_period 600

    <rule>
      match {
          namespace: [/gulel-core/],
          podname: [/redis-master-0/]
      }
      limit 2
    </rule>
  </group>
</source>

could you please help? thanks @ashie @Pranjal-Gupta2 @cosmo0920 @agup006 @pmoogi-redhat

[ashie]

match value should be like this:

  match {
-      namespace: [/gulel-core/],
-      podname: [/redis-master-0/]
+      "namespace": "/gulel-core/",
+      "podname": "/redis-master-0/"
  }

The usage is describe in the document but it's also wrong, sorry for that. I'll fix it.

[ashie]

The usage is describe in the document but it's also wrong, sorry for that. I'll fix it.

fluent/fluentd-docs-gitbook#426

[daipom]

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?

[ashie]

Hmm, that's right. It should be total.

[daipom]

Thanks! For now, I made an issue for this:

• in_tail: group setting judges the limit by the average number, not the total number #4184

[Pranjal-Gupta2]

@daipom @ashie

This PR enables us to form different groups based on configuration rules provided by the user. Within each group, one can set the rate limits for log collection. The set rate limit gets uniformly applied to each member (log file) of the group.

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.

[daipom]

@Pranjal-Gupta2 Thanks!

Considering the previous example, the rate of data sent from one group will be 100x larger.

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.

[Pranjal-Gupta2]

Yup! We can write a better articulation of this point in the documentation.
@ashie Can I take this up?

[daipom]

I will think some more about what to do with #4184.
If it looks good to fix the document, I will fix it.

[daipom]

Yup! We can write a better articulation of this point in the documentation. @ashie Can I take this up?

Oh, sorry. I missed this comment.
At this point, fixing the document would be a good idea!

[daipom]

We welcome the document modifications of course!

[Pranjal-Gupta2]

Thanks @daipom @ashie!
Will start working on this.