Fix logger initialization by daipom · Pull Request #4091 · fluent/fluentd

daipom · 2023-03-12T15:17:55Z

Which issue(s) this PR fixes:

What this PR does / why we need it:
This fix prevents the logger from outputting some initial log messages with some settings not applied, such as format. (Please see #4037).

In addition, the logic of initializing the logger is so difficult and has caused many bugs.
This fix simplifies it.

This fix consists of the following points.

All logger parameters are now set in the first initialization.
- Previously, only parameters related to rotation were applied first, but it is preferable to apply all parameters on that point.
Remove LoggerInitializer
- This class was a source of confusion because its role is difficult to understand.
Reflect this fix in tests.

Docs Changes:
Not needed.

Release Note:
Fix bug that the logger outputs some initial log messages with some settings not applied, such as format.

Additional Context

Currently, there is a bug in overwriting of build_system_config().
It must prefer the options explicitly specified in the command line.

So --log-rotate-age and --log-rotate-size should be merged into system_config in build_system_config()

fluentd/lib/fluent/supervisor.rb

Lines 1050 to 1061 in ee796ef

    
           def build_system_config(conf) 
        
             system_config = SystemConfig.create(conf, @cl_opt[:strict_config_value]) 
        
             # Prefer the options explicitly specified in the command line 
        
             opt = {} 
        
             Fluent::SystemConfig::SYSTEM_CONFIG_PARAMETERS.each do |param| 
        
               if @cl_opt.key?(param) && !@cl_opt[param].nil? 
        
                 opt[param] = @cl_opt[param] 
        
               end 
        
             end 
        
             system_config.overwrite_variables(**opt) 
        
             system_config 
        
           end

However, the log options in log directives are not in Fluent::SystemConfig::SYSTEM_CONFIG_PARAMETERS, so --rotate_age and --rotate_size are not merged into system_config.

So I have no choice but to give special treatment to the rotation parameters.
(I left comments for now)

ashie · 2023-03-14T04:17:31Z

Basically it looks good to me. I'm still checking details.

daipom · 2023-03-14T04:20:54Z

Thanks!

daipom · 2023-03-14T04:32:56Z

    def reopen!
-      @logger.reopen! if @logger
+      @out.reopen(@path, "a") if @path && @path != "-"
      nil
    end


Note: The previous logic is meaningless.

This method was not used, and even if this is called, it will do nothing.

https://github.com/treasure-data/serverengine/blob/v2.3.2/lib/serverengine/daemon_logger.rb#L116-L119

https://github.com/treasure-data/serverengine/blob/v2.3.2/lib/serverengine/daemon_logger.rb#L43-L65

filedev is always nil here.

daipom · 2023-03-14T07:05:29Z

I'll rebase this to apply

Make TestFluentdCommand tests stable on Windows #4096

daipom · 2023-03-14T08:26:30Z

All tests on Windows succeed!! Yay!!
(Excluding with Ruby head)

The logic of initializing logger is so difficult and has caused many bugs. This fix simplifies it and prevents logger from outputting some initial log messages with some settings not applied, such as `format`. This fix consists of the following 2 points. * All logger parameters are now set in the first initialization. * Previously, only parameters related to rotation were applied first, but it is preferable to apply all parameters on that point. * Remove LoggerInitializer * This class was a source of confusion because its role is difficult to understand. TODO: Fix tests. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

The previous one was almost testing nothing. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

* Overwritten `DamonLogger#level=` is not used now, since Fluentd doesn't use ServerEngine's reloading feature. * The parameters about rotate is in `Fluent::LogDeviceIO`, not in `ServerEngine::DaemonLogger`. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

This test should be fixed more..., but it should be done in another branch. Currently, `TestSystemConfig` tests some Supervisor logic too. This is not good, and it should be moved to SupervisorTest. We should remove `FakeSupervisor` class. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

Just add `sub_test_case "init logger"`. Other all diff is re-indent. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

Removed `rotate`, `rotate to max age` tests since `LogTest::test_log_rotates_specified_size_with_logdevio` already exists. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

ashie

LGTM

ashie · 2023-03-16T08:09:26Z

Thanks!

daipom · 2023-03-16T08:12:21Z

Thanks for your review!

Fix a bug of f8b73a5 (fluent#4091). Before that fix, Fluentd can start if rotation is enabled but the log file path is not specified. After that fix, the logger setup starts to fail on Windows. This fix solves it. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

daipom force-pushed the fix-logger-initialization branch from b37532b to d2dfdd9 Compare March 12, 2023 15:27

ashie self-requested a review March 13, 2023 05:03

daipom commented Mar 14, 2023

View reviewed changes

daipom force-pushed the fix-logger-initialization branch from d2dfdd9 to 879eeec Compare March 14, 2023 07:12

abetomo reviewed Mar 14, 2023

View reviewed changes

Comment thread lib/fluent/log.rb Outdated

daipom added 9 commits March 14, 2023 17:28

Test: Refactor tests about --inline-config

e21fc3a

The previous one was almost testing nothing. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

Test: Move per_process_path test

cf2abb7

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

Test: Fix SupervisorTest

fd77c43

* Overwritten `DamonLogger#level=` is not used now, since Fluentd doesn't use ServerEngine's reloading feature. * The parameters about rotate is in `Fluent::LogDeviceIO`, not in `ServerEngine::DaemonLogger`. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

SupervisorTest: Summarize logger tests in one sub_test

156274f

Just add `sub_test_case "init logger"`. Other all diff is re-indent. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

SupervisorTest: Refactor some tests

f1ce1d2

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

Test: Move tests in LoggerInitializerTest

d909dc8

Removed `rotate`, `rotate to max age` tests since `LogTest::test_log_rotates_specified_size_with_logdevio` already exists. Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

LogTest: Make sure to close LogDeviceIO

a9f5113

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>

daipom force-pushed the fix-logger-initialization branch from 879eeec to a9f5113 Compare March 14, 2023 08:29

ashie added this to the v1.16.0 milestone Mar 15, 2023

ashie approved these changes Mar 16, 2023

View reviewed changes

ashie merged commit d5df992 into fluent:master Mar 16, 2023

daipom deleted the fix-logger-initialization branch March 16, 2023 08:12

daipom mentioned this pull request May 29, 2023

Logger: Fails to launch Fluentd without specifying the path in the command line options on Windows with rotation #4187

Closed

daipom mentioned this pull request May 29, 2023

Fix failure to launch Fluentd on Windows with log rotation but not specifying the log path in the command line #4188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix logger initialization - #4091

Fix logger initialization#4091
ashie merged 9 commits into
fluent:masterfrom
daipom:fix-logger-initialization

daipom commented Mar 12, 2023

Uh oh!

ashie commented Mar 14, 2023

Uh oh!

daipom commented Mar 14, 2023

Uh oh!

daipom Mar 14, 2023

Uh oh!

daipom commented Mar 14, 2023

Uh oh!

Uh oh!

daipom commented Mar 14, 2023 •

edited

Loading

Uh oh!

ashie left a comment

Uh oh!

ashie commented Mar 16, 2023

Uh oh!

daipom commented Mar 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	def build_system_config(conf)
	system_config = SystemConfig.create(conf, @cl_opt[:strict_config_value])
	# Prefer the options explicitly specified in the command line
	opt = {}
	Fluent::SystemConfig::SYSTEM_CONFIG_PARAMETERS.each do \|param\|
	if @cl_opt.key?(param) && !@cl_opt[param].nil?
	opt[param] = @cl_opt[param]
	end
	end
	system_config.overwrite_variables(**opt)
	system_config
	end

Uh oh!

Conversation

daipom commented Mar 12, 2023

Uh oh!

ashie commented Mar 14, 2023

Uh oh!

daipom commented Mar 14, 2023

Uh oh!

daipom Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

daipom commented Mar 14, 2023

Uh oh!

Uh oh!

daipom commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashie left a comment

Choose a reason for hiding this comment

Uh oh!

ashie commented Mar 16, 2023

Uh oh!

daipom commented Mar 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daipom commented Mar 14, 2023 •

edited

Loading