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

Fix default value of rotate_age #446

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

daipom
Copy link
Contributor

@daipom daipom commented Mar 11, 2023

It seems that the old value came from ServerEngine.

However, actually, ServerEngine has nothing to do with the rotation, and this value 5 is not used at all.

Actually, Logger::LogDevice manages the rotation, and the default value is 7.

In Fluentd code, the parameters of Fluent::LogDeviceIO decides the behavior of the rotation.

The parameters of ServerEngine::DaemonLogger is meaningless.

Reproduce

  • Env
    • Ubuntu Focal
    • Ruby 3.2
    • Fluentd 1.15.3
  • Config
    • default rotate_size
    <system>
      <log>
        rotate_size 500
      </log>
    </system>
    
    <source>
      @type sample
      tag test.hoge
      sample {"message": "hoge"}
    </source>
    
    <match test.**>
      @type stdout
    </match>
    • rotate_size 5
    <system>
      <log>
        rotate_size 500
        rotate_age 5
      </log>
    </system>
    
    <source>
      @type sample
      tag test.hoge
      sample {"message": "hoge"}
    </source>
    
    <match test.**>
      @type stdout
    </match>
  • Result
    • default rotate_size: 7 files are created
    fluentd.log
    fluentd.log.0
    fluentd.log.1
    fluentd.log.2
    fluentd.log.3
    fluentd.log.4
    fluentd.log.5
    
    • rotate_size 5: 5 files are created
    fluentd.log
    fluentd.log.0
    fluentd.log.1
    fluentd.log.2
    fluentd.log.3
    

The last index of 7th file is 5.
I think it was hard to notice this mistake because of this.

It seems that the old value came from `ServerEngine`.

https://github.com/treasure-data/serverengine/blob/v2.3.1/lib/serverengine/daemon_logger.rb#L33

However, actually, `ServerEngine` has nothing to do with the
rotation, and this value `5` is not used at all.

Actually, `Logger::LogDevice` manages the rotation, and the default
value is `7`.

https://github.com/ruby/logger/blob/v1.5.3/lib/logger/log_device.rb#L20

In Fluentd code, the parameters of `Fluent::LogDeviceIO` decides
the behavior of the rotation.

https://github.com/fluent/fluentd/blob/v1.15.3/lib/fluent/supervisor.rb#L563

The parameters of `ServerEngine::DaemonLogger` is meaningless.

https://github.com/fluent/fluentd/blob/v1.15.3/lib/fluent/supervisor.rb#L580-L582

Signed-off-by: Daijiro Fukuda <[email protected]>
@daipom daipom requested a review from ashie March 17, 2023 02:44
@ashie ashie merged commit c6bd8fb into fluent:1.0 Mar 17, 2023
@ashie
Copy link
Member

ashie commented Mar 17, 2023

Thanks!

@daipom daipom deleted the fix-rotate_age-default-value branch March 17, 2023 08:15
@daipom
Copy link
Contributor Author

daipom commented Mar 17, 2023

Thanks for your review!

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