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

Append plugin name and id to log #860

Merged

Conversation

cosmo0920
Copy link
Contributor

If users use following config:

<source>
  type forward
  @id hoge
  log_level debug
</source>
<match test>
  type stdout
  @id test
  log_level debug
</match>

Fluentd outputs following log which includes optional headers:

2016-03-18 14:00:55 +0900 [info]: fluent/supervisor.rb:518:read_config: reading config file path="example/in_forward.conf"
2016-03-18 14:00:55 +0900 [info]: fluent/supervisor.rb:351:supervise: starting fluentd-0.14.0.pre.1
2016-03-18 14:00:55 +0900 [info]: fluent/engine.rb:117:block in configure: gem 'fluentd' version '0.14.0.pre.1'
2016-03-18 14:00:55 +0900 [info]: fluent/agent.rb:128:add_match: adding match pattern="test" type="stdout"
2016-03-18 14:00:55 +0900 [info]: fluent/root_agent.rb:147:add_source: [Fluent::StdoutOutput(test)] adding source plugin_type="Fluent::StdoutOutput" plugin_id="test" type="forward"
2016-03-18 14:00:55 +0900 [info]: fluent/engine.rb:124:configure: [Fluent::ForwardInput(hoge)] using configuration file: <ROOT>
  <source>
    type forward
    @id hoge
    log_level debug
  </source>
  <match test>
    type stdout
    @id test
    log_level debug
  </match>
</ROOT> plugin_type="Fluent::ForwardInput" plugin_id="hoge"
2016-03-18 14:00:55 +0900 [info]: plugin/in_forward.rb:88:listen: [Fluent::ForwardInput(hoge)] listening fluent socket on 0.0.0.0:24224 plugin_type="Fluent::ForwardInput" plugin_id="hoge"
^C2016-03-18 14:01:03 +0900 [debug]: fluent/supervisor.rb:431:block in install_supervisor_signal_handlers: fluentd supervisor process get SIGINT
2016-03-18 14:01:03 +0900 [debug]: fluent/supervisor.rb:576:block in install_main_process_signal_handlers: [Fluent::ForwardInput(hoge)] fluentd main process get SIGINT plugin_type="Fluent::ForwardInput" plugin_id="hoge"
2016-03-18 14:01:03 +0900 [debug]: fluent/supervisor.rb:580:block in install_main_process_signal_handlers: [Fluent::ForwardInput(hoge)] getting start to shutdown main process plugin_type="Fluent::ForwardInput" plugin_id="hoge"
2016-03-18 14:01:03 +0900 [debug]: fluent/supervisor.rb:576:block in install_main_process_signal_handlers: [Fluent::ForwardInput(hoge)] fluentd main process get SIGINT plugin_type="Fluent::ForwardInput" plugin_id="hoge"
2016-03-18 14:01:03 +0900 [info]: fluent/engine.rb:202:run: [Fluent::ForwardInput(hoge)] shutting down fluentd plugin_type="Fluent::ForwardInput" plugin_id="hoge"
2016-03-18 14:01:03 +0900 [info]: fluent/root_agent.rb:125:block (2 levels) in shutdown: [Fluent::ForwardInput(hoge)] shutting down input plugin_type="Fluent::ForwardInput" plugin_id="hoge" type=:forward
2016-03-18 14:01:03 +0900 [info]: fluent/agent.rb:98:block (2 levels) in shutdown: [Fluent::ForwardInput(hoge)] shutting down output plugin_type="Fluent::ForwardInput" plugin_id="test" type=:stdout
2016-03-18 14:01:03 +0900 [info]: fluent/supervisor.rb:388:supervise: process finished code=0

@cosmo0920
Copy link
Contributor Author

Please review after #859.

@cosmo0920 cosmo0920 force-pushed the append-plugin-name-and-id-to-log branch from 71b2363 to e4bc190 Compare March 18, 2016 05:53
@tagomoris
Copy link
Member

It looks too much for me to add plugin type and id as attributes on all log lines.
How about to add these only for headers?

@log.optional_attrs = {
'plugin_type' => self.class.name,
}
if @_id_configured
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use plugin_id_configured? method instead of referring @_id_configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to use it.

@cosmo0920
Copy link
Contributor Author

It looks too much for me to add plugin type and id as attributes on all log lines.

They are added in only optional headers in log.

In practice, optional headers is not appended in stdout plugin output like:

2016-03-24 16:33:09 +0900 [info]: reading config file path="example/in_forward.conf"
2016-03-24 16:33:09 +0900 [info]: starting fluentd-0.14.0.pre.1
2016-03-24 16:33:09 +0900 [info]: gem 'fluentd' version '0.14.0.pre.1'
2016-03-24 16:33:09 +0900 [info]: adding match pattern="test" type="stdout"
2016-03-24 16:33:09 +0900 [info]: [Fluent::StdoutOutput(stdout)] adding source plugin_type="Fluent::StdoutOutput" plugin_id="stdout" type="forward"
2016-03-24 16:33:09 +0900 [info]: [Fluent::ForwardInput(forward)] using configuration file: <ROOT>
  <source>
    type forward
    @log_level debug
    @id forward
  </source>
  <match test>
    type stdout
    @log_level info
    @id stdout
  </match>
</ROOT> plugin_type="Fluent::ForwardInput" plugin_id="forward"
2016-03-24 16:33:09 +0900 [info]: [Fluent::ForwardInput(forward)] listening fluent socket on 0.0.0.0:24224 plugin_type="Fluent::ForwardInput" plugin_id="forward"
2016-03-24 16:33:13 +0900 test: {"test":"message"}
2016-03-24 16:33:15 +0900 test: {"test":"message"}
2016-03-24 16:33:19 +0900 test: {"test":"message"}

Or, we should reduce output against optional headers in log?

@tagomoris
Copy link
Member

@cosmo0920 Could you rebase on current master? I want to check this change doesn't break anything.

@cosmo0920 cosmo0920 force-pushed the append-plugin-name-and-id-to-log branch from dc622b6 to 6034412 Compare March 28, 2016 04:29
@cosmo0920
Copy link
Contributor Author

Rebased!

@tagomoris
Copy link
Member

2016-03-24 16:33:09 +0900 [info]: [Fluent::ForwardInput(forward)] listening fluent socket on 0.0.0.0:24224 plugin_type="Fluent::ForwardInput" plugin_id="forward"

For example, this log line has attributes: plugin_type="Fluent::ForwardInput" plugin_id="forward"

Are these attributes added to all lines from this plugin instance in spite of this line's header [Fluent::ForwardInput(forward)]?
Adding these information only to header looks work for me.

@cosmo0920 cosmo0920 force-pushed the append-plugin-name-and-id-to-log branch from 6034412 to 181f48c Compare March 30, 2016 08:00
@cosmo0920
Copy link
Contributor Author

How about latest patch?

This patch makes to add plugin inspection into only headers like this:

2016-04-04 11:47:51 +0900 [info]: reading config file path="example/in_forward.conf"
2016-04-04 11:47:51 +0900 [info]: starting fluentd-0.14.0.pre.1
2016-04-04 11:47:51 +0900 [info]: gem 'fluentd' version '0.14.0.pre.1'
2016-04-04 11:47:51 +0900 [info]: adding match pattern="test" type="stdout"
2016-04-04 11:47:51 +0900 [info]: [Fluent::StdoutOutput] adding source
2016-04-04 11:47:51 +0900 [info]: [Fluent::ForwardInput(forward)] using configuration file: <ROOT>
  <source>
    @type forward
    @id forward
    @log_level info
  </source>
  <match test>
    @type stdout
    @log_level info
  </match>
</ROOT>
2016-04-04 11:47:51 +0900 [info]: [Fluent::ForwardInput(forward)] listening fluent socket on 0.0.0.0:24224

@tagomoris
Copy link
Member

Along with your patch, logger looks not to dump attributes (map) when optional header exists.
@cosmo0920 Is it right and intentional? (it's wrong behavior if it's right.)

@cosmo0920
Copy link
Contributor Author

@cosmo0920 Is it right and intentional? (it's wrong behavior if it's right.)

It's intentional: :<

@@ -371,6 +387,13 @@ def configure(conf)
@log = PluginLogger.new($log)
end
@log.level = @log_level
@log.optional_header = "[#{self.class.name}#{plugin_id_configured? ? "(" + @id + ")" : ""}] "
@log.optional_attrs = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed to set plugin_type and plugin_id into optional_attrs?
This is ignored in actual logging by unless condition in L287. What's other purpose of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, this code seems to be useless....

@tagomoris
Copy link
Member

OK, current change is good to me, and CI is green now.
@cosmo0920 Could you squash commits into a commit?

@cosmo0920 cosmo0920 force-pushed the append-plugin-name-and-id-to-log branch from 78b46bc to c86cfe9 Compare April 4, 2016 07:34
@cosmo0920 cosmo0920 force-pushed the append-plugin-name-and-id-to-log branch from c86cfe9 to 750e571 Compare April 4, 2016 07:35
@cosmo0920
Copy link
Contributor Author

Squashed!

@tagomoris tagomoris merged commit c8845d4 into fluent:master Apr 4, 2016
@tagomoris
Copy link
Member

Merged. Thanks!

@cosmo0920 cosmo0920 deleted the append-plugin-name-and-id-to-log branch April 4, 2016 08:17
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